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

go test treats exited suite as overall "pass" #39107

Closed
grasparv opened this issue May 16, 2020 · 2 comments
Closed

go test treats exited suite as overall "pass" #39107

grasparv opened this issue May 16, 2020 · 2 comments

Comments

@grasparv
Copy link

@grasparv grasparv commented May 16, 2020

go version go1.14.3 linux/amd64

I maintain testie that uses go test -json.

Here is an example reproducing unexpected results:

$ ls -l
total 8.0K
-rw-r--r-- 1 js js 332 May 16 11:52 bad_test.go
-rw-r--r-- 1 js js  22 May 16 11:46 go.mod

bad_test.go

package bad

import (
        "os"
        "testing"
)

func productionFunction1() {
        os.Exit(0)
}

func productionFunction2() bool {
        return true
}

func TestHelloOne(t *testing.T) {
        productionFunction1()
        if productionFunction2() == true {
                t.Fail()
        }
}

func TestHelloTwo(t *testing.T) {
        if productionFunction2() == true {
                t.Fail()
        }
}
$ go test -json ./...
{"Time":"2020-05-16T11:52:37.303091346+02:00","Action":"run","Package":"hello","Test":"TestHelloOne"}
{"Time":"2020-05-16T11:52:37.303398456+02:00","Action":"output","Package":"hello","Test":"TestHelloOne","Output":"=== RUN   TestHelloOne\n"}
{"Time":"2020-05-16T11:52:37.303932387+02:00","Action":"output","Package":"hello","Test":"TestHelloOne","Output":"ok  \thello\t0.005s\n"}
{"Time":"2020-05-16T11:52:37.304042213+02:00","Action":"pass","Package":"hello","Test":"TestHelloOne","Elapsed":0.006}
$

Now it appears as if the test suite overall result was OK just because the production code (incorrectly) reached an os.Exit(0).

I would have expected go test to:

  1. Find all test cases
  2. Execute from that list of test cases

Rationale: At the point in time where the go test command has begun execution and established a list of all tests that should run, any failures to execute must be reported as such. By specifying ./..., I believe I am referring to all tests in this directory and sub-directories. It translates to the test specification: Run TestHelloOne and run TestHelloTwo. If go test fails to what I told it to, it violates the test specification. Go can not choose on its own accord to not execute the test due to behaviour in test subject.

Alternatives?: Saying that the production is not allowed to os.Exit(0) incorrectly in production code is obviously not a solution, and it seems unreasonable that users should have to add some secondary layer of tools on top of or inside the test runner to determine whether all tests were really executed (in this case I'd like to have a new flag for go test with this behaviour!) - if this is even possible with the limited information available (e.g. how does a test runner list all tests to execute for ./... for comparing it with go test -json output)?

The behaviour actually seems to be the same for go test without the -json switch, but I am reporting it here as a user of go test -json without trying to pinpoint where the issue lies.

Thank you!

@ALTree
Copy link
Member

@ALTree ALTree commented May 16, 2020

This is a dup of the proposal at #29062, which was already accepted. No one did the work for the 1.15 code freeze deadline, so now it'll have to wait for the 1.16 development phase. Contributions are welcome, so if you care about this and want to contribute, feel free to do so.

I'm closing here, since this is a dup of a proposal that was already accepted. Additional discussion about this should be placed in the thread at #29062. Thank you.

@ALTree ALTree closed this May 16, 2020
@grasparv
Copy link
Author

@grasparv grasparv commented May 16, 2020

Oh, I did not see it. Thanks.

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
2 participants
You can’t perform that action at this time.