evetest: return errors from device file and publication reads

evetest/README.md

@@ -302,10 +302,10 @@ stdout, stderr, err := device.RunShellScript("uptime", timeout, stdoutWatchdogTi
 
 // Read EVE's internal published state (pubsub)
 var dpcl pillartypes.DevicePortConfigList
-evetest.ReadPublication(device, "nim", true, "global", &dpcl)
+err := evetest.ReadPublication(device, "nim", true, "global", &dpcl)
 
 // Read all publications of a type
-items := evetest.ReadAllPublications[pillartypes.AppInstanceStatus](
+items, err := evetest.ReadAllPublications[pillartypes.AppInstanceStatus](
     device, "zedmanager", false)
 
 // Get the latest device info/metrics (or nil if not yet received)

evetest/VERSION

@@ -1,2 +1,2 @@
 # Evetest version. Increment this manually whenever changes are made to the evetest framework.
-1.1
+1.2

evetest/edgedevice.go

@@ -1830,29 +1830,29 @@ func (d *EdgeDevice) FileExists(fileName string) bool {
 }
 
 // ReadFile reads the contents of a file from the device.
-func (d *EdgeDevice) ReadFile(fileName string) []byte {
+func (d *EdgeDevice) ReadFile(fileName string) ([]byte, error) {
 	ctx, cancel := context.WithTimeout(d.th.ctx, fileTransferTimeout)
 	defer cancel()
 
 	tmpFile, err := os.CreateTemp("", "eve-file-*")
 	if err != nil {
-		d.th.t.Fatalf("ReadFile: failed to create temp file: %v", err)
+		return nil, fmt.Errorf("ReadFile: failed to create temp file: %w", err)
 	}
 	tmpPath := tmpFile.Name()
 	tmpFile.Close()
 	defer os.Remove(tmpPath)
 
 	err = d.th.scpFromEVE(ctx, d.devName, fileName, tmpPath, false)
 	if err != nil {
-		d.th.t.Fatalf("ReadFile: failed to copy %q from device %q: %v",
+		return nil, fmt.Errorf("ReadFile: failed to copy %q from device %q: %w",
 			fileName, d.devName, err)
 	}
 
 	data, err := os.ReadFile(tmpPath)
 	if err != nil {
-		d.th.t.Fatalf("ReadFile: failed to read temp file: %v", err)
+		return nil, fmt.Errorf("ReadFile: failed to read temp file: %w", err)
 	}
-	return data
+	return data, nil
 }
 
 // WriteFile writes content to a file on the device.
@@ -2782,12 +2782,11 @@ func (d *EdgeDevice) WatchClusterMetrics() (
 //   - key: identifies the specific message within the topic to fetch
 //   - output: pointer to a value of type T to unmarshal the message into
 //
-// Returns false if the topic or message does not exist yet (e.g. before the
-// agent has first published it) -- callers that need to wait for it should
-// poll on the returned bool instead of treating absence as an error. Calls
-// t.Fatalf on any other read or unmarshal failure.
+// Returns an error if the message does not exist yet (e.g. before the agent has
+// first published it), cannot be read, or does not unmarshal into T. Callers
+// waiting for a message to appear should poll until the error clears.
 func ReadPublication[T any](d *EdgeDevice, fromAgent string, persistent bool,
-	key string, output *T) bool {
+	key string, output *T) error {
 	fullName := fmt.Sprintf("%T", *new(T))
 	typeName := fullName[strings.LastIndex(fullName, ".")+1:]
 	var path string
@@ -2796,15 +2795,15 @@ func ReadPublication[T any](d *EdgeDevice, fromAgent string, persistent bool,
 	} else {
 		path = fmt.Sprintf("/run/%s/%s/%s.json", fromAgent, typeName, key)
 	}
-	if !d.FileExists(path) {
-		return false
+	data, err := d.ReadFile(path)
+	if err != nil {
+		return fmt.Errorf("ReadPublication: %w", err)
 	}
-	data := d.ReadFile(path)
 	if err := json.Unmarshal(data, output); err != nil {
-		d.th.t.Fatalf("ReadPublication: failed to unmarshal %q from device %q: %v",
+		return fmt.Errorf("ReadPublication: failed to unmarshal %q from device %q: %w",
 			path, d.devName, err)
 	}
-	return true
+	return nil
 }
 
 // ReadAllPublications retrieves all messages from a pub-sub topic published by
@@ -2816,7 +2815,8 @@ func ReadPublication[T any](d *EdgeDevice, fromAgent string, persistent bool,
 //
 // Returns a slice of values of type T representing all messages from the topic,
 // or an error if reading or unmarshaling fails.
-func ReadAllPublications[T any](d *EdgeDevice, fromAgent string, persistent bool) []T {
+func ReadAllPublications[T any](d *EdgeDevice, fromAgent string,
+	persistent bool) ([]T, error) {
 	fullName := fmt.Sprintf("%T", *new(T))
 	typeName := fullName[strings.LastIndex(fullName, ".")+1:]
 	var dir string
@@ -2830,20 +2830,29 @@ func ReadAllPublications[T any](d *EdgeDevice, fromAgent string, persistent bool
 		"find "+shellEscape(dir)+" -maxdepth 1 -name '*.json' -type f 2>/dev/null || true",
 		quickSSHCommandTimeout, 0)
 	if err != nil {
-		d.th.t.Fatalf("ReadAllPublications: failed to list %q on device %q: %v",
+		return nil, fmt.Errorf("ReadAllPublications: failed to list %q on device %q: %w",
 			dir, d.devName, err)
 	}
 	var results []T
-	for _, file := range strings.Fields(stdout) {
-		data := d.ReadFile(file)
+	// Pubsub keys become file names and may contain spaces.
+	for _, file := range strings.Split(stdout, "\n") {
+		file = strings.TrimRight(file, "\r")
+		if file == "" {
+			continue
+		}
+		data, err := d.ReadFile(file)
+		if err != nil {
+			return nil, fmt.Errorf("ReadAllPublications: %w", err)
+		}
 		var item T
 		if err := json.Unmarshal(data, &item); err != nil {
-			d.th.t.Fatalf("ReadAllPublications: failed to unmarshal %q from device %q: %v",
+			return nil, fmt.Errorf(
+				"ReadAllPublications: failed to unmarshal %q from device %q: %w",
 				file, d.devName, err)
 		}
 		results = append(results, item)
 	}
-	return results
+	return results, nil
 }
 
 // getDevUUID returns the device UUID, calling t.Fatalf if not found/onboarded.

evetest/ssh.go

@@ -210,19 +210,26 @@ func (th *TestHarness) scpFromEVE(ctx context.Context,
 	if recursive {
 		scpArgs = append(scpArgs, "-r")
 	}
-	// The path after the colon is interpreted by a remote shell, so it needs
-	// its own shell quoting independent of how this argv element is split
-	// locally (scp itself is invoked directly via exec, with no local shell
-	// involved) -- otherwise a remote path containing spaces (e.g. a pubsub
-	// key like "Application Data Store") gets split into multiple arguments
-	// remotely.
+	// Pass the remote path verbatim: scp speaks SFTP (the OpenSSH 9.x default,
+	// and no -O here selects the legacy protocol), so no remote shell splits
+	// it and quoting it would make the quotes part of the file name. Spaces
+	// are safe because this is a single argv element -- exec runs scp
+	// directly, with no local shell.
 	scpArgs = append(scpArgs,
 		"-i", "/root/.ssh/eve_rsa",
-		"root@"+eveIP+":"+shellEscape(remotePath),
+		"root@"+eveIP+":"+remotePath,
 		localPath,
 	)
 	cmd := exec.CommandContext(ctx, "scp", scpArgs...)
-	return cmd.Run()
+	var stderr bytes.Buffer
+	cmd.Stderr = &stderr
+	if err := cmd.Run(); err != nil {
+		if msg := strings.TrimSpace(stderr.String()); msg != "" {
+			return fmt.Errorf("%w: %s", err, msg)
+		}
+		return err
+	}
+	return nil
 }
 
 // scpToEVE copies a file (or, when recursive is true, a directory) from a
@@ -242,10 +249,18 @@ func (th *TestHarness) scpToEVE(ctx context.Context,
 	scpArgs = append(scpArgs,
 		"-i", "/root/.ssh/eve_rsa",
 		localPath,
-		"root@"+eveIP+":"+shellEscape(remotePath),
+		"root@"+eveIP+":"+remotePath,
 	)
 	cmd := exec.CommandContext(ctx, "scp", scpArgs...)
-	return cmd.Run()
+	var stderr bytes.Buffer
+	cmd.Stderr = &stderr
+	if err := cmd.Run(); err != nil {
+		if msg := strings.TrimSpace(stderr.String()); msg != "" {
+			return fmt.Errorf("%w: %s", err, msg)
+		}
+		return err
+	}
+	return nil
 }
 
 // getReachableEVEAddr finds a reachable IP for the given device at the specified

evetest/tests/networking/dns_test.go

@@ -319,22 +319,26 @@ func TestDNSFunctionality(test *testing.T) {
 	// ------------------------------------------------------------------
 	log.Infof("Phase 2: verifying /etc/resolv.conf and mgmt dnsmasq config...")
 
-	resolvConf := string(device.ReadFile("/etc/resolv.conf"))
-	t.Expect(resolvConf).To(ContainSubstring("nameserver 127.0.0.1"))
+	resolvConf, err := device.ReadFile("/etc/resolv.conf")
+	t.Expect(err).NotTo(HaveOccurred())
+	t.Expect(string(resolvConf)).To(ContainSubstring("nameserver 127.0.0.1"))
 
 	// The main config file contains static options only; upstream server
 	// entries live in the separate servers file re-read on SIGHUP.
 	const (
 		configFilePath  = "/run/nim/dnsmasq.mgmt.conf"
 		serversFilePath = "/run/nim/dnsmasq.mgmt.servers"
 	)
-	dnsmasqConf := string(device.ReadFile(configFilePath))
-	t.Expect(dnsmasqConf).To(ContainSubstring("servers-file=" + serversFilePath))
+	dnsmasqConf, err := device.ReadFile(configFilePath)
+	t.Expect(err).NotTo(HaveOccurred())
+	t.Expect(string(dnsmasqConf)).To(ContainSubstring("servers-file=" + serversFilePath))
 
 	// The servers file may still be updating after Phase 1 — wait for it
 	// to contain all expected servers in cost-ascending order.
 	t.Eventually(func(g Gomega) {
-		dnsmasqServers := string(device.ReadFile(serversFilePath))
+		serversFile, err := device.ReadFile(serversFilePath)
+		g.Expect(err).NotTo(HaveOccurred())
+		dnsmasqServers := string(serversFile)
 		// All expected upstream servers must be present.
 		g.Expect(dnsmasqServers).To(ContainSubstring(badDNS3IP),
 			"eth3 (cost=0) server must appear in mgmt dnsmasq servers file")

evetest/tests/networking/pciback_error_test.go

@@ -74,7 +74,8 @@ func portPciLong(t *WithT, device *evetest.EdgeDevice, phylabel string) string {
 	var pci string
 	t.Eventually(func() string {
 		var aa pillartypes.AssignableAdapters
-		if !evetest.ReadPublication(device, "domainmgr", false, "global", &aa) {
+		if err := evetest.ReadPublication(
+			device, "domainmgr", false, "global", &aa); err != nil {
 			return ""
 		}
 		for _, b := range aa.IoBundleList {

evetest/tests/storage/vault_trim_test.go

@@ -69,8 +69,8 @@ func TestVaultZvolTrimReclaimsBlocks(test *testing.T) {
 	// Wait for vaultmgr to report the default vault ConversionComplete.
 	t.Eventually(func() bool {
 		var status pillartypes.VaultStatus
-		if !evetest.ReadPublication(device, "vaultmgr", false,
-			pillartypes.DefaultVaultName, &status) {
+		if err := evetest.ReadPublication(device, "vaultmgr", false,
+			pillartypes.DefaultVaultName, &status); err != nil {
 			return false // status not published yet
 		}
 		return status.ConversionComplete

evetest/tests/storage/zvol_provisioned_size_test.go

@@ -155,9 +155,11 @@ func TestZVolProvisionedSizeReported(test *testing.T) {
 	// volumemgr pubsub object: AppDiskMetric.ProvisionedBytes is the value the
 	// fix populates. The zvol device path embeds the volume UUID, so match on
 	// it. Poll because volumemgr recomputes disk metrics on its own interval.
-	t.Eventually(func() uint64 {
-		for _, m := range evetest.ReadAllPublications[pillartypes.AppDiskMetric](
-			device, "volumemgr", false) {
+	t.Eventually(func(g Gomega) uint64 {
+		diskMetrics, err := evetest.ReadAllPublications[pillartypes.AppDiskMetric](
+			device, "volumemgr", false)
+		g.Expect(err).NotTo(HaveOccurred())
+		for _, m := range diskMetrics {
 			if strings.Contains(m.DiskPath, volUUID.String()) {
 				return m.ProvisionedBytes
 			}