fix: Fix cleanup in test helpers, prefer `defer` in tests #3113

mafredri · 2022-07-22T12:18:36Z

This PR was sparked by #3109 (comment).

Taking a closer look at uses of t.Cleanup there were some places where require interrupts cleanup.

Motivation for preferring defer outside of helpers is that mixed use of t.Cleanup and defer result in unexpected execution order, ideally we should strive to only use t.Cleanup in helper functions.

fix: Change uses of t.Cleanup -> defer in test bodies
fix: Ensure t.Cleanup is not aborted by require
chore: Add helper annotations

mafredri · 2022-07-22T12:19:55Z

cli/logout_test.go

 			if runtime.GOOS == "windows" {
 				// Closing the opened files for cleanup.
 				err = urlFile.Close()
-				require.NoError(t, err)
+				assert.NoError(t, err)


Assert here, to avoid interrupting Close below.

Should comment this in the file so it doesn't get reverted

I kind of agree, but I'm less worried about this old code changing than new (incorrect) code going in. This is something everyone using require must be very aware of (it halts execution flow).

mafredri · 2022-07-22T13:47:52Z

Looks like I overlooked the fact that sometimes t.Cleanup is used to clean up after parallel sub-tests, will revert those changes.

kylecarbs · 2022-07-22T19:32:30Z

We should probably create a linter for this. cc @Emyrk our in-house linting DSL expert

deansheather

👍 Linter would be cool and should be easy to detect all instances of t.Cleanup, with nolint for every exception

deansheather · 2022-07-22T20:17:19Z

coderd/coderd_test.go

@@ -94,6 +94,8 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
 		t.Cleanup(func() { close(tickerCh) })

 		ctx, cancelFunc := context.WithCancel(context.Background())
+		defer t.Cleanup(cancelFunc) // Defer to ensure cancelFunc is executed first.


Does defer cancelFunc() not work here? I would expect defer to run before cleanups.

See comment on the other instance: https://github.com/coder/coder/pull/3113/files#r928922910.

deansheather · 2022-07-22T20:17:44Z

coderd/coderdtest/coderdtest.go

@@ -143,6 +143,8 @@ func newWithCloser(t *testing.T, options *Options) (*codersdk.Client, io.Closer)
 	}

 	ctx, cancelFunc := context.WithCancel(context.Background())
+	defer t.Cleanup(cancelFunc) // Defer to ensure cancelFunc is executed first.


Since this is a helper function, defer cancelFunc() would run before the calling test gets a chance to do anything with it. And a t.Cleanup without defer would cancel the context too late, it would not trigger teardown. So I realize this is a bit of a sneaky way to do it, but this way t.Cleanup(cancelFunc) will be left on top of the cleanup stack when this function exits, triggering the teardown we want (and same order as before the refactor).

Mixing t.Cleanup and defer can lead to unexpected order of execution.

mafredri · 2022-07-25T15:09:45Z

Writing the rule would be fairly straightforward:

func testCleanup(m dsl.Matcher) {
	m.Import("testing")

	m.Match(`t.Cleanup($*fn)`).
		Where(m["fn"].Type.Is("func()")).
		Report("Only call t.Cleanup in helpers.").
		Suggest("defer $fn()")
}

But I'm not sure demanding a //nolint for every cleanup in every helper function is a great developer experience.

Is there a way we could improve upon this @Emyrk? For instance, if t.Helper() is called before t.Cleanup == OK?

EDIT: Looks the above ruleguard also only works for single lines, if it's a multi-line function it won't be tagged.

Emyrk

Interesting. I always kinda wondered if t.Cleanup vs defer actually made an impact.

Emyrk · 2022-07-25T15:54:48Z

Writing the rule would be fairly straightforward:
func testCleanup(m dsl.Matcher) {
	m.Import("testing")

	m.Match(`t.Cleanup($*fn)`).
		Where(m["fn"].Type.Is("func()")).
		Report("Only call t.Cleanup in helpers.").
		Suggest("defer $fn()")
}
But I'm not sure demanding a //nolint for every cleanup in every helper function is a great developer experience.

Is there a way we could improve upon this @Emyrk? For instance, if t.Helper() is called before t.Cleanup == OK?

EDIT: Looks the above ruleguard also only works for single lines, if it's a multi-line function it won't be tagged.

Yea matching negative cases is really tricky, but I think we can dial the Cleanup match to prevent false positives (helper funcs). We can expand the match to match in a whole test function, as I think helper functions have different signatures.

	m.Import("testing")

	m.Match(`
	$f(t *testing.T) {
		$*_
		t.Cleanup($*_)
		$*_
	}
	`).
		Where(m["f"].Text.Matches("^Test")).
		Report("Only call t.Cleanup in helpers.").
		At(m["c"])

The downside to this is that if the function calls t.Cleanup() twice, only the 1st will match. You fix it, then run the linter, and the second matches. So you don't get both matches in 1 go 🤷‍♂️

mafredri · 2022-07-25T16:02:51Z

@Emyrk Yeah, that may be the best approach. Thanks for the sample. We might want to add a separate case for t.Run functions as well since those won't match ^Test.

mafredri · 2022-07-25T16:21:33Z

Created #3181 to track the linting request and get this merged.

mafredri self-assigned this Jul 22, 2022

mafredri requested a review from a team July 22, 2022 12:18

mafredri commented Jul 22, 2022

View reviewed changes

deansheather approved these changes Jul 22, 2022

View reviewed changes

mafredri added 7 commits July 25, 2022 17:45

fix: Change uses of t.Cleanup -> defer in test bodies

9a5db33

Mixing t.Cleanup and defer can lead to unexpected order of execution.

fix: Ensure t.Cleanup is not aborted by require

628d9f2

chore: Add helper annotations

2334d8a

fix: Revert bad change

d263af2

fix: Bad placement

7b356b3

fix: Run ctx cancel first in t.Cleanup

f245f64

fix: Revert bad cleanup changes

b624871

mafredri force-pushed the mafredri/test-cleanup-defer-fixes branch from 193bb78 to b624871 Compare July 25, 2022 14:59

Emyrk reviewed Jul 25, 2022

View reviewed changes

mafredri mentioned this pull request Jul 25, 2022

Add linting rule for preferring defer over t.Cleanup in tests (except helpers) #3181

Closed

mafredri merged commit 6916d34 into main Jul 25, 2022

mafredri deleted the mafredri/test-cleanup-defer-fixes branch July 25, 2022 16:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Fix cleanup in test helpers, prefer `defer` in tests #3113

fix: Fix cleanup in test helpers, prefer `defer` in tests #3113

mafredri commented Jul 22, 2022

mafredri Jul 22, 2022

deansheather Jul 22, 2022

mafredri Jul 25, 2022

mafredri commented Jul 22, 2022

kylecarbs commented Jul 22, 2022

deansheather left a comment

deansheather Jul 22, 2022

mafredri Jul 25, 2022

deansheather Jul 22, 2022

mafredri Jul 25, 2022

mafredri commented Jul 25, 2022 •

edited

Loading

Emyrk left a comment

Emyrk commented Jul 25, 2022 •

edited

Loading

mafredri commented Jul 25, 2022

mafredri commented Jul 25, 2022

fix: Fix cleanup in test helpers, prefer defer in tests #3113

fix: Fix cleanup in test helpers, prefer defer in tests #3113

Conversation

mafredri commented Jul 22, 2022

mafredri Jul 22, 2022

Choose a reason for hiding this comment

deansheather Jul 22, 2022

Choose a reason for hiding this comment

mafredri Jul 25, 2022

Choose a reason for hiding this comment

mafredri commented Jul 22, 2022

kylecarbs commented Jul 22, 2022

deansheather left a comment

Choose a reason for hiding this comment

deansheather Jul 22, 2022

Choose a reason for hiding this comment

mafredri Jul 25, 2022

Choose a reason for hiding this comment

deansheather Jul 22, 2022

Choose a reason for hiding this comment

mafredri Jul 25, 2022

Choose a reason for hiding this comment

mafredri commented Jul 25, 2022 • edited Loading

Emyrk left a comment

Choose a reason for hiding this comment

Emyrk commented Jul 25, 2022 • edited Loading

mafredri commented Jul 25, 2022

mafredri commented Jul 25, 2022

fix: Fix cleanup in test helpers, prefer `defer` in tests #3113

fix: Fix cleanup in test helpers, prefer `defer` in tests #3113

mafredri commented Jul 25, 2022 •

edited

Loading

Emyrk commented Jul 25, 2022 •

edited

Loading