testing: unify context usage in tests

[nvanbenschoten]

See discussion in #14128.

Currently, we have a mixture of context.Background and context.TODO calls scattered throughout our tests. It's unclear which one to use, and no pattern has formed. There are good arguments for and against using both. context.Background is the standard recommended usage for tests and is the convention adopted by most Go project, but implies that the context usage is a resolved question. Meanwhile, context.TODO implies that future work is desired and helps keep unresolved context decisions separate from fully resolved ones, but is primarily meant for locations which we plan on fixing later by plumbing in a real context.

@RaduBerinde further elaborated on this:

I used TODO in tests because I thought there might be a good reason to some day have a
per-test context (like the proposed testutils.Context(t)) for capturing traces. The appdash
work mentioned is one potential reason for this. We may also want to run various tests
with Lighstep if it helps debugging. I think being able to look at a lightstep trace when
starting to debug a test failure could save a lot of time.

We should either fully adopt context.Background in tests as recommended by the Go documentation or replace all contexts in tests with some per-test context. Either way, context.TODO should not sit in our tests forever.

cc. @andreimatei @RaduBerinde @tamird

[benesch]

Is the only argument against ctx := testutils.Context(t) the diff noise? As an outsider to this debate, it seems like if the downside of either context.Background() or context.TODO() is that the name of the method doesn't match the context's purpose, then testutils.Context is the obvious solution.

I'll cast a vote for incurring the diff noise; I found the lack of context-creation consistency to be the most confusing part of our context system.

Also, it looks like there's a chance that testing.T.Context() returns in Go 1.9.

[RaduBerinde]

I think it would be a good change because it will give us the flexibility to do something smarter with testutil.Context() later. But making the change will probably be time-consuming (more so than just changing TODO to Background) - there are probably many testing helper functions which don't take either testing.TB or a Context; new args would have to be plumbed through).

[bdarnell]

In the 5 months since this issue was filed, no plan has emerged for doing anything smarter with contexts in tests (nor, as far as I've seen, has there been any need that might drive such a plan). testing.T.Context() did not appear in Go 1.9. Therefore, I'd like to propose that in order to put to rest questions like those in #17885, we do s/context.TODO/context.Background/g in all *_test.go files. When and if we develop a new plan for test contexts, we'll still be able to grep for context.Background in tests just as easily as we could for context.TODO.

For curiosity's sake, we currently have slightly more instances of context.Background than context.TODO in tests (and relatively few uses of either outside of tests):

[bdarnell@rowlf cockroach]$ git grep context.Background -- '*_test.go'|wc -l
1412
[bdarnell@rowlf cockroach]$ git grep context.TODO -- '*_test.go'|wc -l
1154
[bdarnell@rowlf cockroach]$ git grep context.Background -- '*.go'|wc -l
1577
[bdarnell@rowlf cockroach]$ git grep context.TODO -- '*.go'|wc -l
1355

[andreimatei]

I maintain my opinion that the TODO in context.TODO() should be read to mean "put something here that's better than Background()". If the alternative is Background(), I thing TODO() is better (so I'd unify them the other way if you feel like unifying).

[bdarnell]

But what's the "better than Background()" alternative that these TODOs are waiting for? It's silly to have a style rule that mandates the use of context.TODO in brand new code. And we don't really lose anything by switching to context.Background - when/if we decide on a better context strategy for tests, the choice of context we make today won't make any difference on how easy or hard that will be to adopt.

[benesch]

Plus, the documentation for Background() does recommend it for use in tests:

Background returns a non-nil, empty Context. It is never canceled, has no values, and has no deadline. It is typically used by the main function, initialization, and tests, and as the top-level Context for incoming requests.

[andreimatei]

But what's the "better than Background()" alternative that these TODOs are waiting for?

Let me turn the question around - why is Background() the right context? The work done by the tests is very much foreground, not background. Anything is better than Background() for them.

[benesch]

Background() is just poorly named. When you create a context for an incoming request, you're using Background() to create a context for foreground work, no?

[tbg]

My only contribution to what seems to me like a 🚲 🏚 is that we should just use context.Background() in all tests. context.TODO() is functionally identical, and whatever lofty plan we have in the future for our testing contexts is completely independent of the choice here. Reserving context.TODO() for use in production code that actually should have a real request context is more useful.

[andreimatei]

Background() is just poorly named. When you create a context for an incoming request, you're using Background() to create a context for foreground work, no?

Exactly, you are creating (deriving) a context, you're not just using Background().

I think TODO() is poorly named :)

To Tobi I'd say that reserving Background() for use by background server activity is the more useful thing.
Anyway, this is my last message on the topic. We can take a vote if I haven't impressed anyone :)

[RaduBerinde]

It doesn't matter much either way. I'm ok with Background FWIW

[andreimatei]

Well I guess it's likely I'll lose a vote, so if whoever wants to drive this still has the desire...

[tbg]

Seems like Background won.