Attempt to reduce test flakiness on Windows

[pietern]

Changes

Test failures indicate that both stdout and stderr are consumed, yet the content of stdout doesn't end up in the intended output. This can happen if the goroutines responsible for writing to the combined output buffer attempt to write to the same underlying buffer concurrently.

Example failure:

=== RUN   TestBackgroundCombinedOutput
    background_test.go:65: 
        	Error Trace:	D:/a/cli/cli/libs/process/background_test.go:65
        	Error:      	elements differ
        	            	
        	            	extra elements in list A:
        	            	([]interface {}) (len=1) {
        	            	 (string) (len=1) "2"
        	            	}
        	            	
        	            	
        	            	listA:
        	            	([]string) (len=2) {
        	            	 (string) (len=1) "1",
        	            	 (string) (len=1) "2"
        	            	}
        	            	
        	            	
        	            	listB:
        	            	([]string) (len=1) {
        	            	 (string) (len=1) "1"
        	            	}
        	Test:       	TestBackgroundCombinedOutput

With the test body:

cli/libs/process/background_test.go

Lines 48 to 66 in ca45e53

	func TestBackgroundCombinedOutput(t *testing.T) {
	ctx := context.Background()
	buf := bytes.Buffer{}
	res, err := Background(ctx, []string{
	"python3", "-c", "import sys, time; " +
	`sys.stderr.write("1\n"); sys.stderr.flush(); ` +
	"time.sleep(0.001); " +
	"print('2', flush=True); sys.stdout.flush(); " +
	"time.sleep(0.001)",
	}, WithCombinedOutput(&buf))
	assert.NoError(t, err)
	assert.Equal(t, "2", strings.TrimSpace(res))
	// The order of stdout and stderr being read into the buffer
	// for combined output is not deterministic due to scheduling
	// of the underlying goroutines that consume them.
	// That's why this asserts on the contents and not the order.
	assert.ElementsMatch(t, []string{"1", "2"}, splitLines(buf.Bytes()))
	}

With the implementation of WithCombinedOutput:

cli/libs/process/opts.go

Lines 72 to 78 in ca45e53

	func WithCombinedOutput(buf *bytes.Buffer) execOption {
	return func(_ context.Context, c *exec.Cmd) error {
	c.Stdout = io.MultiWriter(buf, c.Stdout)
	c.Stderr = io.MultiWriter(buf, c.Stderr)
	return nil
	}
	}

Notice that c.Stdout does get the "2", or the test failure would have included the relevant assertion error. This leads me to believe that there is a race on writing to buf from the two goroutines writing to c.Stdout and c.Stderr.

Tests

The test passes. If this PR has the intended effect remains to be seen...

[shreyas-goenka]

Just curious, Why is this only a problem on windows? Indeed it does not seem to happen on macs (ran the test 1000 times on my machine).

[pietern]

@shreyas-goenka It is possible the goroutines for stdout and stderr are woken up at the same time on Windows due to some interactions at the I/O notification level. I expect that on other platforms they are woken up sequentially (due to the tiny sleep in the Python code).