UnitTest failures on main

[jlewi]

Running on main I'm seeing test failures.

git log
commit 61fc8785ab8adb4cec5f1135d278a9e8895f327c (HEAD, upstream/main, upstream/HEAD)
Author: Sebastian Tiedtke <sebastiantiedtke@gmail.com>
Date:   Thu May 23 16:41:17 2024 -0400

    Is python3 more likely than python? (#586)
    
    `python` is an alias for `python3` on my BMP which makes the test fail.

Here are the test failures

 logger.go:146: 2024-05-23T23:09:03.531Z	INFO	stream canceled after the process finished; ignoring	{"id": "01HYKVE2NV87J6ZQTMY69E6BYT"}
    --- FAIL: TestRunnerServiceServerExecute_WithInput/SimulateCtrlC (1.52s)
        service_execute_test.go:643: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:643
            	Error:      	"rpc error: code = Unknown desc = exit status 1" does not contain "exit status 130"
            	Test:       	TestRunnerServiceServerExecute_WithInput/SimulateCtrlC
        service_execute_test.go:644: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:644
            	Error:      	Not equal: 
            	            	expected: 130
            	            	actual  : 1
            	Test:       	TestRunnerServiceServerExecute_WithInput/SimulateCtrlC

    --- FAIL: TestRunnerServiceServerExecute_WithInput/ContinuousInput (0.04s)
        service_execute_test.go:600: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:600
            	Error:      	Not equal: 
            	            	expected: "a\r\nb\r\nc\r\nd\r\nA\r\nB\r\nC\r\nD\r\n"
            	            	actual  : "a\r\nb\r\nA\r\nB\r\nc\r\nC\r\nd\r\nD\r\n"
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -2,7 +2,7 @@
            	            	 b
            	            	-c
            	            	-d
            	            	 A
            	            	 B
            	            	+c
            	            	 C
            	            	+d
            	            	 D
            	Test:       	TestRunnerServiceServerExecute_WithInput/ContinuousInput

2024-05-23T23:09:03.740Z	DEBUG	runner/command.go:404	finished copying from pty to stdout	{"count": 32}
--- FAIL: Test_command (0.00s)
    --- FAIL: Test_command/JavaScript (0.03s)
        command_test.go:107: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runner/command_test.go:107
            	Error:      	Not equal: 
            	            	expected: ""
            	            	actual  : "(node:24760) ExperimentalWarning: The Node.js specifier resolution flag is experimental. It could change or be removed at any time.\n(Use `node --trace-warnings ...` to show where the warning was created)\n"
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1 +1,3 @@
            	            	+(node:24760) ExperimentalWarning: The Node.js specifier resolution flag is experimental. It could change or be removed at any time.
            	            	+(Use `node --trace-warnings ...` to show where the warning was created)
            	            	 
            	Test:       	Test_command/JavaScript
    --- FAIL: Test_command/JavaScriptEnv (0.03s)
        command_test.go:135: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runner/command_test.go:135
            	Error:      	Not equal: 
            	            	expected: ""
            	            	actual  : "(node:24765) ExperimentalWarning: The Node.js specifier resolution flag is experimental. It could change or be removed at any time.\n(Use `node --trace-warnings ...` to show where the warning was created)\n"
            	            	
            	            	Diff:
            	            	--- Expected
            	            	+++ Actual
            	            	@@ -1 +1,3 @@
            	            	+(node:24765) ExperimentalWarning: The Node.js specifier resolution flag is experimental. It could change or be removed at any time.
            	            	+(Use `node --trace-warnings ...` to show where the warning was created)
            	            	 
            	Test:       	Test_command/JavaScriptEnv
FAIL
FAIL	github.com/stateful/runme/v3/internal/runner	4.624s
ok  	github.com/stateful/runme/v3/internal/runner/client	0.255s
--- FAIL: TestRunnerServiceServerExecute_WithInput (1.61s)

Tests appear to have passed on main at this commit #586 .

Are others seeing test failures when running locally or is it just me?

[jlewi]

It looks like a bunch of tests including some of the ones the are failing aren't running in GHA.

There are two test steps in CI.
The first is test on docker

runme/.github/workflows/ci.yml

Line 60 in 61fc878

- name: Test

This ends up running tests with tag test_with_docker

TZ=UTC go test -ldflags="-X 'github.com/stateful/runme/v3/internal/version.BuildVersion=3.3.1-3-g61fc878-61fc878'" -run=".*" -tags="test_with_docker" -timeout=90s -covermode=atomic -coverprofile=cover.out -coverpkg=./... "./..."

So only a subset of tests get run. TestRunnerServiceServerExecute_WithInput/SimulateCtrlC for example is not included.

There is a CI step to run tests on windows

runme/.github/workflows/ci.yml

Line 67 in 61fc878

- name: Test

This doesn't include any tags to restrict the tests. However, a bunch of tests are configured not to get built on windows. e.g

runme/internal/runnerv2service/service_execute_test.go

Line 1 in 61fc878

//go:build !windows

So it looks like we may have tests running locally that don't run on CI.

[jlewi]

Here's my proposal

1. Create a new GHA step that runs tests with no tag constraints.

2. Skip any tests which are currently failing using a skip directive e.g.

   
    if !testutil.runFlaky() {
       t.Skipf("Skipping flaky test")
    }
   

3. Open up individual issues to resolve specific failing tests

If this sounds good I can take this on; @sourishkrout let me know.

[sourishkrout]

😮 this is actually a bug in how the vscode terminal connects back to the kernel server running inside of vscode. The env vars seem to be persistent when reloaded/revived. I can repro the issue. Attempting a fix here: stateful/vscode-runme#1379

 logger.go:146: 2024-05-23T23:09:03.531Z	INFO	stream canceled after the process finished; ignoring	{"id": "01HYKVE2NV87J6ZQTMY69E6BYT"}
    --- FAIL: TestRunnerServiceServerExecute_WithInput/SimulateCtrlC (1.52s)
        service_execute_test.go:643: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:643
            	Error:      	"rpc error: code = Unknown desc = exit status 1" does not contain "exit status 130"
            	Test:       	TestRunnerServiceServerExecute_WithInput/SimulateCtrlC
        service_execute_test.go:644: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:644
            	Error:      	Not equal: 
            	            	expected: 130
            	            	actual  : 1
            	Test:       	TestRunnerServiceServerExecute_WithInput/SimulateCtrlC

[adambabik]

Thanks for reporting!

• TestRunnerServiceServerExecute_WithInput/SimulateCtrlC -- it's flaky. One possible solution is to use a loop to send signals and validate responses. Sometimes input characters are sent too fast.
• TestRunnerServiceServerExecute_WithInput/ContinuousInput -- this is quite interesting. That output is completely valid and it's race condition.
• Test_command/JavaScript and Test_command/JavaScriptEnv are failing due to different node.js version. We should run these only in the CI as locally it does not make sense. It kinda worked so far because all contributors have had the same node.js version. We can hide it with a build tag similarly to tests that expect Docker.

[sourishkrout]

😮 this is actually a bug in how the vscode terminal connects back to the kernel server running inside of vscode. The env vars seem to be persistent when reloaded/revived. I can repro the issue. Attempting a fix here: stateful/vscode-runme#1379

 logger.go:146: 2024-05-23T23:09:03.531Z	INFO	stream canceled after the process finished; ignoring	{"id": "01HYKVE2NV87J6ZQTMY69E6BYT"}
    --- FAIL: TestRunnerServiceServerExecute_WithInput/SimulateCtrlC (1.52s)
        service_execute_test.go:643: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:643
            	Error:      	"rpc error: code = Unknown desc = exit status 1" does not contain "exit status 130"
            	Test:       	TestRunnerServiceServerExecute_WithInput/SimulateCtrlC
        service_execute_test.go:644: 
            	Error Trace:	/Users/jlewi/git_runme/internal/runnerv2service/service_execute_test.go:644
            	Error:      	Not equal: 
            	            	expected: 130
            	            	actual  : 1
            	Test:       	TestRunnerServiceServerExecute_WithInput/SimulateCtrlC

Almost fixed it. Still not quite done yet until #607 is merged/release.

• Test_command/JavaScript and Test_command/JavaScriptEnv are failing due to different node.js version. We should run these only in the CI as locally it does not make sense. It kinda worked so far because all contributors have had the same node.js version. We can hide it with a build tag similarly to tests that expect Docker:

These should just migrate to #605.

[sourishkrout]

While we still have occasional flakes, a lot of work's been done to produce stable test runs, especially locally.