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: (*B).Fatal should cause non-zero exit status #14307

Closed
davecheney opened this issue Feb 12, 2016 · 12 comments

Comments

Projects
None yet
7 participants
@davecheney
Copy link
Contributor

commented Feb 12, 2016

lucky(~/src/issue) % cat issue_test.go 
package issue

import "testing"

func BenchmarkFail(b *testing.B) {
        for n := 0; n < b.N; n++ {
                b.Fatal("non zero exit")
        }
}
lucky(~/src/issue) % go test issue_test.go -test.bench=.
testing: warning: no tests to run
PASS
BenchmarkFail-4 --- FAIL: BenchmarkFail-4
        issue_test.go:7: non zero exit
ok      command-line-arguments  0.003s
lucky(~/src/issue) % echo $?
0
@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2016

The subject of the issue does not match the behavior of the program, and there is no explanatory text. Please explain.

@davecheney

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2016

b.Fatal should terminate the test program like its counterpart t.Fatal

@ianlancetaylor ianlancetaylor changed the title b.Fatal does not terminate test testing: b.Fatal does not terminate test Feb 12, 2016

@ianlancetaylor ianlancetaylor added this to the Go1.7 milestone Feb 12, 2016

@robpike

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2016

t.Fatal does not terminate the program. It terminates the test function. b.Fatal seems to do that as well, or at least your example does not demonstrate otherwise.

It looks like there is an issue that b.Fatal does not cause the program to exit with a non-zero exit code. Is that what you're saying?

@davecheney

This comment has been minimized.

Copy link
Contributor Author

commented Feb 12, 2016

I am sorry, you are correct. But calling t.Fatal in a test will cause the
test binary to eventually exit with a non zero error code. I was surprised
that calling the counterpart on testing.B did not cause the same behaviour.

On Sat, 13 Feb 2016, 09:32 Rob Pike notifications@github.com wrote:

t.Fatal does not terminate the program.


Reply to this email directly or view it on GitHub
#14307 (comment).

@cespare

This comment has been minimized.

Copy link
Contributor

commented Feb 12, 2016

@davecheney want to change the issue title? Maybe "testing: (*B).Fatal should cause non-zero exit status".

I took a brief look at this. The code does this right now (irrelevant lines elided):

testOk := RunTests(m.matchString, m.tests)
exampleOk := RunExamples(m.matchString, m.examples)
if !testOk || !exampleOk {
    fmt.Println("FAIL")
    return 1
}
fmt.Println("PASS")
RunBenchmarks(m.matchString, m.benchmarks)
return 0

Unfortunately the exported function RunBenchmarks doesn't return failure status. Perhaps we could modify InternalBenchmark to smuggle failure status through.

If we fix this, should we also only print "PASS" if benchmarks pass, as well as tests and examples?

@davecheney davecheney changed the title testing: b.Fatal does not terminate test esting: (*B).Fatal should cause non-zero exit status Feb 12, 2016

@mikioh mikioh changed the title esting: (*B).Fatal should cause non-zero exit status testing: (*B).Fatal should cause non-zero exit status Feb 13, 2016

@cmarcelo

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2016

Would adding a return value to RunBenchmarks break the compatibility promise? It seems to me it keeps the source compatibility since ignoring the single return value doesn't require any change to calling code.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Feb 26, 2016

Would adding a return value to RunBenchmarks break the compatibility promise?

Yes.

@bradfitz

This comment has been minimized.

@cmarcelo

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2016

@bradfitz Thanks for the link.

I wrote a CL that adds an extra field to the struct InternalBenchmark to pass the information (a "failed bool" field). It does pass the API check in all.bash, and seems "acceptable" from reading https://golang.org/doc/go1compat document, and the Internal in the name makes me thing is not widely used. On the other hand it doesn't pass the criteria from the post you've linked. So I'm a bit unsure whether is OK or not.

Pushed the CL in https://go-review.googlesource.com/#/c/19889/.

@gopherbot

This comment has been minimized.

Copy link

commented Feb 26, 2016

CL https://golang.org/cl/19889 mentions this issue.

@cmarcelo

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2016

Ian had a better suggestion without having to add a field to the exported struct, it became patch set 3 in the CL.

I'm still curious about the compatibility: because of the embedding explanation mentioned in the link, is adding fields a no-go for exported structs?

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Feb 27, 2016

In the compatibility doc (https://golang.org/doc/go1compat) we explicitly permit adding fields to exported structs.

@gopherbot gopherbot closed this in 3cb870d Feb 28, 2016

@golang golang locked and limited conversation to collaborators Feb 28, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.