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
Closed

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

dsnet opened this issue Apr 4, 2018 · 9 comments
Labels
FrozenDueToAge Proposal v2 A language change or incompatible library change
Milestone

Comments

@dsnet
Copy link
Member

dsnet 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
@dsnet dsnet added the v2 A language change or incompatible library change label Apr 4, 2018
@cespare
Copy link
Contributor

cespare 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
Copy link
Contributor

bcmills 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
Copy link
Contributor

bcmills 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
Copy link

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
Copy link
Contributor

neild 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
Copy link

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

@neild
Copy link
Contributor

neild 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 testing: complain loudly during concurrent use of T.FatalX and T.SkipX #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
Copy link

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
Copy link
Contributor

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.
Labels
FrozenDueToAge Proposal v2 A language change or incompatible library change
Projects
None yet
Development

No branches or pull requests

7 participants