Add rivertest.WorkContext for use testing JobArgs.Work implementations

[brandur]

Here, add a new test helper called rivertest.WorkContext. Its purpose
is to generate a realistic looking context for testing JobArgs.Work
implementation, particularly by adding a client to context that makes
river.ClientFromContext available in tests, but may also be used to
add any other context-related features that may be added in the future.

We've talked about new test helpers for running Work implementations
and may still do that, but this is a primitive that makes testing a
little better without having to add anything heavyweight.

Fixes #512.

[bgentry]

LGTM aside from comments ✌️

[bgentry]

Thoughts on exposing a function here for setting the client on the context, rather than exposing the key itself? That would still make it impossible for that key to be utilized in any way other than by calling the function. And it feels more inline with the common pattern recommended in the docs (see userKey).

To make this work you would also need the getter function to be exposed in this package though (withClient).

[brandur]

Hm, unfortunately I think this turns out to be difficult because to get/set a client, you need a reference to the Client type. Having that in rivercommon would be a circular dependency, and even if we were to bring in a separate package for it, it'd still create an import cycle with the main package.

You saw that this is the internal rivercommon right? Should maybe it impossible to reuse ContextKeyClient for anything else.

[bgentry]

Do you think this should also attempt to apply timeouts to the context or no? For example there's a default client-level job timeout, as well as the possibility of it being overridden by the worker's Timeout(job) method.

I did that with my internal job testing helper in the API, but since it's worker-specific maybe it makes more sense for the possible job-running helper instead of this one.

[brandur]

I definitely thought about it, but I think it might be out of scope for this particular helper.

Once added, a timeout is hard to remove (so I'd generally bias against it), but more importantly here, we don't have the right information to calculate what a timeout should be — to know what your timeout is, you need access to the client default, the job args to pull an InsertOpts off of, and the job itself in case of a job-specific override.

I think we could add something like this with a more elaborate rivertest.Work function of some kind, but it'd be a bit more of a project. I tried to prototype this quickly, but it's easier said than done because you'd need some back channel to pull a timeout out of an injected client, and possibly a way of getting an executor from job args.

We could experiment with that path a little more, but I suspect it might be easier to just have users explicitly call their own Work functions and add their own timeouts at that time if they want it. We'd supply a basic context with WorkContext, but nothing else.

I know I always beat this point home, but I'll just float again that use of context.WithTimeout in tests can be considered a major anti-pattern because in case one fails, you get very little information about that where a timeout happened. Compare that to using go test ./... -timeout 1m and you get an exact stack trace of where a slow test was when it hits its maximum time allowance.

tl;dr I think we should probably leave timeouts of the equation for now, and possible consider them for more elaborate test functions down the line.

[bgentry]

I know I always beat this point home, but I'll just float again that use of context.WithTimeout in tests can be considered a major anti-pattern because in case one fails, you get very little information about that where a timeout happened.

I agree generally but WithTimeoutCause does change the story here a little bit, particularly if the error is easily identifiable. Otherwise I agree with the rest of what you wrote 😄

[bgentry]

Related to my last, I checked and we do not actually use WithTimeoutCause anywhere—I think because we wanted to support Go 1.20 when we initially launched River. Maybe we should make use of that for River-controlled timeouts?

[brandur]

Yeah, agreed that it'd probably make sense to replace all our timeouts with a WithTimeoutCause and then have a single River timeout value that people could reference. Not sure how often people really need to check what kind of timeout occurred, but I'm sure it'll be useful to somebody.

TBC, are you saying that we should still add WithTimeoutCause here too, or just more broadly use it elsewhere?

[bgentry]

Thinking about it more, I think the test helper is probably best without applying its own timeouts. I don't think most people will want to be running unit tests with their real world timeouts applied anyway.

So yeah, more of a side comment, this is good to go 👍

[brandur]

Perfect, thx.