Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

testing: tests that panic sometimes log "warning: no tests to run" instead #41479

Closed
dmitshur opened this issue Sep 18, 2020 · 5 comments
Closed

testing: tests that panic sometimes log "warning: no tests to run" instead #41479

dmitshur opened this issue Sep 18, 2020 · 5 comments

Comments

@dmitshur
Copy link
Member

@dmitshur dmitshur commented Sep 18, 2020

The TestScript/test_exit test in cmd/go failed on the linux-386-longtest builder in https://build.golang.org/log/6f34555f06c618921fe4b9e85fbe1522a1a8078e because a panic wasn't seen:

--- FAIL: TestScript (0.00s)
    --- FAIL: TestScript/test_exit (0.78s)
        script_test.go:211: 
            # Builds and runs test binaries, so skip in short mode. (0.000s)
            # If a test invoked by 'go test' exits with a zero status code,
            # it will panic. (0.777s)
            > ! go test ./zero
            [stdout]
            --- FAIL: TestExitZero (0.00s)
            testing: warning: no tests to run
            FAIL
            FAIL	m/zero	0.024s
            FAIL
            [exit status 1]
            > ! stdout ^ok
            > ! stdout 'exit status'
            > stdout 'panic'
            FAIL: testdata/script/test_exit.txt:11: no match for `(?m)panic` found in stdout
            
FAIL
FAIL	cmd/go	99.358s

The passing behavior for that test is something like:

# If a test invoked by 'go test' exits with a zero status code,
# it will panic. (0.418s)
> ! go test ./zero
[stdout]
--- FAIL: TestExitZero (0.00s)
panic: unexpected call to os.Exit(0) during test [recovered]
panic: unexpected call to os.Exit(0) during test

Another failure on linux-386-longtest builder around 20 commits ago in https://build.golang.org/log/63ec772b8d735f614581991b172dc91b86f64b5c also involved not finding panic in stdout:

--- FAIL: TestScript (0.00s)
    --- FAIL: TestScript/test_cleanup_failnow (0.79s)
        script_test.go:211: 
            # For issue 41355 (0.787s)
            > [short] skip
            > ! go test -v cleanup_failnow/panic_nocleanup_test.go
            [stdout]
            === RUN   TestX
            === RUN   TestX/x
            --- FAIL: TestX (0.00s)
                --- FAIL: TestX/x (0.00s)
            --- FAIL: TestX (0.00s)
            testing: warning: no tests to run
            FAIL
            FAIL	command-line-arguments	0.041s
            FAIL
            [exit status 1]
            > stdout '(?s)panic: die \[recovered\].*panic: die'
            FAIL: testdata/script/test_cleanup_failnow.txt:5: no match for `(?m)(?s)panic: die \[recovered\].*panic: die` found in stdout
            
FAIL
FAIL	cmd/go	86.983s

Not sure if the issue is in cmd/go or the runtime.

/cc @bcmills

@dmitshur dmitshur added this to the Backlog milestone Sep 18, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented Sep 18, 2020

Only those two in the logs. Hrm.

I suspect that this is more likely a bug in the testing package, or in runtime or even cmd/compile, than in cmd/go proper, although without a root cause of course I can't be sure.

2020-09-18T14:36:22-789d77a/linux-386-longtest
2020-09-17T15:51:14-07d5eb0/linux-386-longtest

@bcmills
Copy link
Member

@bcmills bcmills commented Sep 18, 2020

The first of these was observed Sept. 17, which isn't too long after CL 254637 (CC @ianlancetaylor @odeke-em @changkun), which changed the panic behavior of the testing package.

@bcmills bcmills modified the milestones: Backlog, Go1.16 Sep 18, 2020
@bcmills
Copy link
Member

@bcmills bcmills commented Sep 18, 2020

The testing: warning: no tests to run lines in the output are also very suspicious. I suspect that they indicate a race between the panic-propagation code and the test-cleanup code. (Probably the testing package is not waiting for whatever is supposed to propagate the panic to complete.)

Unfortunately, that error string is not unique. It has two possible origins:

fmt.Fprintln(os.Stderr, "testing: warning: no tests to run")

fmt.Fprintln(os.Stderr, "testing: warning: no tests to run")

@bcmills bcmills changed the title cmd/go: tests sometimes fail on linux-386-longtest builder due to no match for panic in stdout testing: tests that panic sometimes log "warning: no tests to run" instead Sep 18, 2020
@gopherbot
Copy link

@gopherbot gopherbot commented Sep 19, 2020

Change https://golang.org/cl/256098 mentions this issue: testing: send t.signal only if there is no panic

@changkun
Copy link
Contributor

@changkun changkun commented Sep 19, 2020

Sorry for making trouble again. I think the bug was indeed introduced in CL 254637.

The reason is CL 254637 sends the t.signal earlier before the panic statement.
Since t.Run calls runtime.Goexit() on panicking tests, if t.Run receives the signal earlier than invoking the panic. The callee runTests may not record the running test, which results in "warning: no tests to run."

I don't have an idea about how to reproduce the bug, but I think my suspicion is reasonable. Thus I just moved the signaling code a few lines down and sent a CL that should fix the bug.

Another option would be to revert the CL 254637, but this choice would lead to #41355 open again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants