Wait for browser process to exit before returning first error from stderr

This ensures we don't return on the first error we see, since they could
be non-fatal, but actually wait for the browser process to exit.

Fixes #614

Author: Ivan Mirić

common/browser_process.go

@@ -179,41 +179,51 @@ func execute(
 	return command{cmd, done, stdout, stderr}, nil
 }
 
-// parseDevToolsURL grabs the websocket address from chrome's output and returns it.
-func parseDevToolsURL(ctx context.Context, cmd command) (wsURL string, _ error) {
-	type result struct {
-		devToolsURL string
-		err         error
-	}
-	c := make(chan result, 1)
+// parseDevToolsURL grabs the WebSocket address from Chrome's output and returns
+// it. If the process ends abruptly, it will return the first error from stderr.
+func parseDevToolsURL(ctx context.Context, cmd command) (string, error) { //nolint: cyclop
+	var (
+		lines = make(chan string)
+		errCh = make(chan error)
+	)
 	go func() {
-		const urlPrefix = "DevTools listening on "
 		scanner := bufio.NewScanner(cmd.stderr)
-
 		for scanner.Scan() {
-			line := scanner.Text()
+			lines <- scanner.Text()
+		}
+		if err := scanner.Err(); err != nil {
+			errCh <- err
+		}
+	}()
+
+	const urlPrefix = "DevTools listening on "
+	var stdErr error
+
+	preferStdErr := func(e error) error {
+		if stdErr != nil {
+			return stdErr
+		}
+		return e
+	}
+
+	for {
+		select {
+		case line := <-lines:
 			if strings.HasPrefix(line, urlPrefix) {
-				c <- result{
-					strings.TrimPrefix(strings.TrimSpace(line), urlPrefix),
-					nil,
-				}
-				return
+				return strings.TrimPrefix(strings.TrimSpace(line), urlPrefix), nil
 			}
-			if strings.Contains(line, ":ERROR:") {
+			// TODO: Capture more than one error?
+			if stdErr == nil && strings.Contains(line, ":ERROR:") {
 				if i := strings.Index(line, "] "); i > 0 {
-					c <- result{"", errors.New(line[i+2:])}
-					return
+					stdErr = errors.New(line[i+2:])
 				}
 			}
+		case err := <-errCh:
+			return "", preferStdErr(err)
+		case <-cmd.done:
+			return "", preferStdErr(errors.New("browser process ended unexpectedly"))
+		case <-ctx.Done():
+			return "", preferStdErr(fmt.Errorf("%w", ctx.Err()))
 		}
-		if err := scanner.Err(); err != nil {
-			c <- result{"", err}
-		}
-	}()
-	select {
-	case r := <-c:
-		return r.devToolsURL, r.err
-	case <-ctx.Done():
-		return "", fmt.Errorf("%w", ctx.Err())
 	}
 }

common/browser_process_test.go

@@ -0,0 +1,187 @@
+package common
+
+import (
+	"context"
+	"io"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+type mockCommand struct {
+	*command
+	cancelFn context.CancelFunc
+}
+
+type mockReader struct {
+	lines []string
+	hook  func()
+	err   error
+}
+
+func (r *mockReader) Read(p []byte) (n int, err error) {
+	if r.hook != nil {
+		// Allow some time for the read to be processed
+		time.AfterFunc(100*time.Millisecond, r.hook)
+		r.hook = nil // Ensure the hook only runs once
+	}
+	if len(r.lines) == 0 {
+		return 0, io.EOF
+	}
+	n = copy(p, []byte(r.lines[0]+"\n"))
+	r.lines = r.lines[1:]
+
+	return n, r.err
+}
+
+func TestParseDevToolsURL(t *testing.T) {
+	t.Parallel()
+
+	testCases := []struct {
+		name     string
+		stderr   []string
+		readErr  error
+		readHook func(c *mockCommand)
+		assert   func(t *testing.T, wsURL string, err error)
+	}{
+		{
+			name: "ok/no_error",
+			stderr: []string{
+				`DevTools listening on ws://127.0.0.1:41315/devtools/browser/d1d3f8eb-b362-4f12-9370-bd25778d0da7`,
+			},
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.NoError(t, err)
+				assert.Equal(t, "ws://127.0.0.1:41315/devtools/browser/d1d3f8eb-b362-4f12-9370-bd25778d0da7", wsURL)
+			},
+		},
+		{
+			name: "ok/non-fatal_error",
+			stderr: []string{
+				`[23400:23418:1028/115455.877614:ERROR:bus.cc(399)] Failed to ` +
+					`connect to the bus: Could not parse server address: ` +
+					`Unknown address type (examples of valid types are "tcp" ` +
+					`and on UNIX "unix")`,
+				"",
+				`DevTools listening on ws://127.0.0.1:41315/devtools/browser/d1d3f8eb-b362-4f12-9370-bd25778d0da7`,
+			},
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.NoError(t, err)
+				assert.Equal(t, "ws://127.0.0.1:41315/devtools/browser/d1d3f8eb-b362-4f12-9370-bd25778d0da7", wsURL)
+			},
+		},
+		{
+			name: "err/fatal-eof",
+			stderr: []string{
+				`[6497:6497:1013/103521.932979:ERROR:ozone_platform_x11` +
+					`.cc(247)] Missing X server or $DISPLAY` + "\n",
+			},
+			readErr: io.ErrUnexpectedEOF,
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "Missing X server or $DISPLAY")
+			},
+		},
+		{
+			name:    "err/fatal-eof-no_stderr",
+			stderr:  []string{""},
+			readErr: io.ErrUnexpectedEOF,
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "unexpected EOF")
+			},
+		},
+		{
+			// Ensure any error found on stderr is returned first.
+			name: "err/fatal-premature_cmd_done-stderr",
+			stderr: []string{
+				`[6497:6497:1013/103521.932979:ERROR:ozone_platform_x11` +
+					`.cc(247)] Missing X server or $DISPLAY` + "\n",
+			},
+			readHook: func(c *mockCommand) { close(c.done) },
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "Missing X server or $DISPLAY")
+			},
+		},
+		{
+			// If there's no error on stderr, return a generic error.
+			name:     "err/fatal-premature_cmd_done-no_stderr",
+			stderr:   []string{""},
+			readHook: func(c *mockCommand) { close(c.done) },
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "browser process ended unexpectedly")
+			},
+		},
+		{
+			name: "err/fatal-premature_ctx_cancel-stderr",
+			stderr: []string{
+				`[6497:6497:1013/103521.932979:ERROR:ozone_platform_x11` +
+					`.cc(247)] Missing X server or $DISPLAY` + "\n",
+			},
+			readHook: func(c *mockCommand) { c.cancelFn() },
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "Missing X server or $DISPLAY")
+			},
+		},
+		{
+			name:     "err/fatal-premature_ctx_cancel-no_stderr",
+			stderr:   []string{""},
+			readHook: func(c *mockCommand) { c.cancelFn() },
+			assert: func(t *testing.T, wsURL string, err error) {
+				t.Helper()
+				require.Empty(t, wsURL)
+				assert.EqualError(t, err, "context canceled")
+			},
+		},
+	}
+
+	for _, tc := range testCases {
+		tc := tc
+		t.Run(tc.name, func(t *testing.T) {
+			t.Parallel()
+
+			ctx, cancel := context.WithCancel(context.Background())
+			t.Cleanup(cancel)
+
+			var cmd *mockCommand
+			mr := mockReader{lines: tc.stderr, err: tc.readErr}
+			if tc.readHook != nil {
+				mr.hook = func() { tc.readHook(cmd) }
+			}
+			cmd = &mockCommand{&command{done: make(chan struct{}), stderr: &mr}, cancel}
+
+			timeout := time.Second
+			timer := time.NewTimer(timeout)
+			t.Cleanup(func() { _ = timer.Stop() })
+
+			var (
+				done  = make(chan struct{})
+				wsURL string
+				err   error
+			)
+
+			go func() {
+				wsURL, err = parseDevToolsURL(ctx, *cmd.command)
+				close(done)
+			}()
+
+			select {
+			case <-done:
+				tc.assert(t, wsURL, err)
+			case <-timer.C:
+				t.Errorf("test timed out after %s", timeout)
+			}
+		})
+	}
+}