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

proposal: testing: remove or improve T.FailNow #24680

Closed
dsnet opened this issue Apr 4, 2018 · 9 comments

Comments

Projects
None yet
7 participants
@dsnet
Copy link
Member

commented Apr 4, 2018

The "fatal only for main goroutine" nature of T.FailNow is a source of bugs when writing tests. We should either remove it or improve it's usability for Go 2. See issues #24678, #15758.

Option 0:
Make misuse complain loudly. Rejected in #15758.

Option 1:
Just remove it (along with its friends Fatal and Fatalf). Whether SkipNow, Skip, Skipf are removed is possible too. With them gone, there is no temptation to misuse it.

Option 2:
Expand the functionality of FailNow to be callable from any goroutine. The behavior would be such that it tears down only the goroutine it is called form and also cancels a Context. Other goroutines can synchronize of the context for their own termination. This presumes that Contexts are added to testing. See #18368 (comment). This option can be done in Go 1.

\cc @neild @bcmills @ash2k

@gopherbot gopherbot added this to the Proposal milestone Apr 4, 2018

@gopherbot gopherbot added the Proposal label Apr 4, 2018

@dsnet dsnet added the Go2 label Apr 4, 2018

@cespare

This comment has been minimized.

Copy link
Contributor

commented Apr 4, 2018

I vote for option 0.

How would (1) work, in practice? Every t.Fatal() becomes t.Fatal(); return or something?

It sounds like (2) would require action from users to opt into the correct thing, so it wouldn't directly prevent the same misuse that happens today.

I still think we should do #15758. AFAICT that thread doesn't actually contain any reasons why it's infeasible. I don't see why the testing package can't use horrible hacks as long as they're hidden in its implementation.

I've seen this mistake many, many times while reviewing Go code.

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

How would (1) work, in practice?

Personally, I would probably use panic instead. In a top-level test function it has the same effect, and in a background goroutine it more obviously terminates the entire binary.

It sounds like (2) would require action from users to opt into the correct thing, so it wouldn't directly prevent the same misuse that happens today.

Agreed.

@bcmills

This comment has been minimized.

Copy link
Member

commented Apr 4, 2018

Option (2) is still missing some sort of synchronization. If we terminate the test, how do we prevent it from interfering with other tests? In particular, how do we distinguish goroutines started by lazy library initialization from goroutines associated with the test function itself?

@balshetzer

This comment has been minimized.

Copy link

commented Apr 6, 2018

We should revisit #15758 and complain loudly when FailNow is called from the wrong goroutine.

If we're willing to go further then we should just do os.Exit(1) on such an illegal invocation. The test is basically unsalvageable at this point and the whole thing should just die. If that's too far for Go 1 then it should be considered for Go 2.

Either way we should still make FailNow complain in Go 1. It'll save a lot of pain.

@neild

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

In #15758 @rsc stated that there's no way to tell if t.Fatal is called from the "right" goroutine, but I don't think that's true. Fatal could examine the stack to see if it has an appropriate function from the testing package in its ancestry.

@balshetzer

This comment has been minimized.

Copy link

commented Apr 6, 2018

Exactly, that doesn't even require exposing goroutine IDs, which is what stalled the original discussion.

@neild

This comment has been minimized.

Copy link
Contributor

commented Apr 6, 2018

I believe there are two independent questions that this issue conflates:

  • T.FailNow is documented as being required to be called from the main test goroutine. Nothing enforces this, and users frequently ignore this requirement. The requirement should either be enforced or removed.

  • Many, many tests suffer from race conditions caused by test goroutines outliving the test. (e.g., @dsnet's example in #15758.) Perhaps something can be done to make these errors less common. If so, the solution might address tests in particular or concurrency in general.

These questions can be addressed independently. We could just permit T.FailNow to be called from any goroutine with a simple documentation change, acknowledging the current state of affairs.

@balshetzer

This comment has been minimized.

Copy link

commented Apr 6, 2018

Such a documentation change won't fix any of the problems caused by calling FailNow outside of the test. This seems like a bad option.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented May 1, 2018

Let's just do #15758.

@golang golang locked and limited conversation to collaborators May 1, 2019

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.