Do not hang Eventually on failed condition

[ubunatic]

Summary

Do not hang Eventually when condition does not return normally and fail fast instead.
Do not hang Never when condition does not return normally to prevent false positives.

Code Changes

• Unblock blocking ch <- condition() with a defer func in Eventually
• Unblock blocking ch <- condition() with a defer func in Never
• React on all runtime.Goexit() variants to fail accordingly in Eventually, Never, and EventuallyWithT
• Report any unclean condition goroutine exit as "Condition exited unexpectedly"

Details/Effect

• Previously hanging tests caused by unclean condition exits will fail fast
• Entirely failed tests will not wait for the full timeout anymore but fail fast
• Eventually and Never will behave more unsurprisingly and fail on exited conditions
• Never will not report false positives caused by unclean exits
• EventuallyWithT can distinguish between collect failure and outer failure and will fail on an exit if it was not recorded in the collect. The condition func must call collect.FailNow() (also indirectly) or return normally to not fail
• The sum of changes allow for safely calling require.* functions also on the outer/parent t inside conditions to fail the test immediately

Doc/Internal Changes

• Updated and linked code and repo documentation on the behavior
• Extended code examples to explain usage of Eventually, EventuallyWithT, and Never with details on best practices
  • general: Use atomic operations!
  • new: It's ok to call require.*
• Updated _codegen to respect source code links like in // Also see [Eventually] ... and to reuse the last string parameter as error message (updated a few code examples to comply)
• Changed MockT.Failed to MockT.Failed() to better comply with testing interfaces and for allowing to reuse the CollectT implementation for testing tests.

Motivation

• Currently when calling require.Fail, or t.FailNow(), or a similar function inside the Eventually condition, or when the condition does not return due to any indirect call to runtime.Goexit, the ch <- condition will block and cause the tick loop to hang until the timeout is reached.
• There was no way to detect the failed condition and hanging loop.

Related issues

• Eventually hangs on failed/aborted condition #1810

Tests

• added many test to test the new fast exit behavior
• added "demo" tests to show that panic still exit the runner (run with env: TestPanic=1)

Discussion

• I propose to use a new message "Condition exited unexpectedly"
• Alternatively we can use the same error message "Condition never satisfied" as used for the timeout case, to not break existing external test analysis approaches.
• (In the future) we could also detect panics and report a "Condition panicked" to educate developers what actually happened. Out of scope for this change, could be a v2 feature.

Impact

1. (Our) Go developers tend to write require.NoError or similar on the outer t inside the Eventually condition, causing the tests to hang. This regularly creates confusion. With this change the test behavior becomes more explicit. A require.Fail(t) or similar inside the Eventually condition will just behave as if called outside: make the test fail, but without hanging the test. "As a developer" this is how I would expect my test tests to fail.

2. Just looking at our internal codebase, I can see hundreds of tests that will fail fast with this change. Globally, I expect to save thousands of hours of wasted compute caused by the hanging tests.

3. The more explicit error will better educate developers what is wrong in their code. The fixed Never behavior will reveal hidden issues. And the safe use of require.* in the condition functions will make 1000s of existing test no longer violate the best practices.

Risk Assessment

• Panics stay as is. The behavior is the same -> low risk
• Eventually exits faster. This may cause some timing issues, but the core logic stays the same -> low risk
• EventuallyWithT detects external exits. Logic on the collect stays as is. External exits through the outer t will just fail fast. Before, EventuallyWithT would succeed (wrongly) and only the outer test would fail. Now both will fail. -> low risk
• Never reports more failures. It exits with an error now and does not block anymore. Before, it would block and never check the condition anymore, which may be satisfied in the meantime, and then Never would wrongly report a success. This will reveal hidden satisfy bugs -> low risk
• MockT.Failed(). MockT is not used outside of this repo (GH search + Sourcegraph search) -> low risk

[brackendawson]

This must be tested, you could do so like this:

func TestEventuallyFailsFast(t *testing.T) {
	mockT := &mockTestingT{}
	wait := make(chan struct{})
	go func() {

		Eventually(mockT, func() bool {
			// Emulate a call to testing.T.FailNow
			runtime.Goexit()
			return true
		}, time.Hour, time.Millisecond)
		close(wait)
	}()
	select {
	case <-wait:
	case <-time.After(time.Second):
		t.Fatal("Eventually did not fail fast")
	}
	// assert that eventually adds an error to mockT
	True(t, mockT.Failed())
}

The documentation of Eventually must be changed to explain the new failure behaviour when runtime.Goexit is called from the condition function.

The same change should be made in the other eventually/never functions where it makes sense.

The above being a review of the code, I'm hesitent about making a change like this. This relies on unsupported behavior in the standard library, namely the behavior of calling t.FailNow from a goroutine other than the one running the test. This behavior isn't guaranteed to remain the same, though with our particular case being synchronous I have no good reason to doubt it would change.

This is also inconsistent when compared with the T versions of this assertion, where a failing require function fails the condition and not the entire assertion. However that is already what happens, just more slowly.

[ubunatic]

@brackendawson I tried to address the issue overall, incl. WithT and Never.

• The mockTestingT and also CollectT work well for tests. Thanks for the hint.
• I could test most of the behavior, I think
• The new error message would be "Condition exited unexpectedly". Happy to change that as needed.
• Also added docs and code comments.

Please have a look again at the proposed changes. Thank you!

[ubunatic]

I pushed a few smaller improvements on the tests docs and updated the PR and GH Issue description. I think The PR now covers Eventually, Never, and EventuallyWithT comprehensively and holistically.

[ubunatic]

While writing more tests I found my initial CollectT result check not to be consistent.
I extended CollectT with a finished state (like in testing.T). This way we can clearly distinguish if FailNow was called on collect or on the parent t and safely fail fast.

I updated the docs and tests for this, please have a look. @brackendawson

Note that I changed MockT's Failed to Failed() and reused an embedded CollectT there, since it provides all what is needed to collect errors for tests. I don't think MockT is used outside of this repo (GH search did not show anything, also did a sourcegraph code search). Impact should be low, except for quite a few updated tests. If needed, I can make this a separate PR.

[brackendawson]

I'm not sure what has happened here? We're now handling painics which I expected to be left out of this PR. The code is full of badly written comments and emoji. There are changes in three (!) un-related packages. And the changes in the file I expected to see changed are now so large that they aren't initially shown in GitHub's diff viewer.

At the time of my last review I though this was close to ready to merge, I think it might be better if you return to that state (73ecd57) and implement the requested changes without allowing the scope to creep.

[ubunatic]

Since this got out of hand, I will restart from 73ecd57 as proposed and make new PR, as small as possible, touching as few files as possible. Thanks for taking the time to review.

---

Edit

I will still address all the comments to not leave them open.

If there is value in any of the extensive docs in this PR, we may cherry pick them later; without emojis, as I now understand. The obscurity of Eventually and frequent discussions with other Gophers brought me here. I though adding more elaborate docs and better examples would help other Gophers using the lib.

IMHO: With better tooling across all OSes, code + emojis is easy to do and provides better UX; if not used too wildly.

Other than that, changing scripts and unrelated files was really a mistake. Again, sorry for that, and for wasting your time.

Next

I now opened a new PR. There, I still fix the runtime.Goexit issue holistically for Eventually, Never, and EventuallyWithT to provide consistent behavior. To me this is not out of scope. For EventuallyWithT this required a small internal change to CollectT.