rivertest: implement a Worker type to simplify testing #753

bgentry · 2025-02-11T02:43:56Z

Since the early days of the project, we've talked about potentially building some APIs to make it easier to test River workers in a realistic execution environment. This is an attempt to dramatically simplify this process, partly derived from testing implementations I wrote for River Pro's API backend code.

The rivertest.Worker type makes it much simpler to test River workers with a realistic execution environment without needing to touch the database at all. The type leverages a river.Config to allocate an inner Client and to enable jobs to be easily created with the normal default values, along with full access to features like middleweare. The execution context also contains all the normal stuff that a real context in River might, such as the client and a timeout if configured.

Here's a simple example of how to use this:

worker := river.WorkFunc(func(ctx context.Context, job *river.Job[testArgs]) error {
	return nil
})
testWorker := rivertest.NewWorker(t, driver, config, worker)

// Now that the `rivertest.Worker` has been initialized, it can be used to run as many different jobs as you'd like:
require.NoError(t, testWorker.Work(context.Background(), t, testArgs{Value: "test"}))

// An opts variant allows customizing insert opts:
require.NoError(t, testWorker.Work(context.Background(), t, testArgs{Value: "test"}), &river.InsertOpts{Metadata: `{"some": "value"}`))

// Want to totally customize the input job to test it in any desired state? See `WorkJob` or `WorkJobOpts`.

This is missing a lot of test coverage and there are some open TODOs like extracting the insertParamsFromConfigArgsAndOptions from the main river package so rivertest can also use it, but this should hopefully illustrate what I think is a pretty smooth API for easier testing of workers than what it otherwise takes today.

Related issue: #605

Currently based on #754.

brandur

Cool. Yep, idea looks good to me. Maybe finish it up and add some niceties like docs on the public-facing functions and maybe an example test to demonstrate usage.

rivertest/worker.go

bgentry · 2025-02-12T19:21:58Z

rivertest/worker.go

+		return nil, err
+	}
+
+	// TODO: use insertParamsFromConfigArgsAndOptions instead of reimplementing it here?


@brandur looked at this one a bit, and insertParamsFromConfigArgsAndOptions can't really be extracted from the main package because there are a number of dependencies on its own types, including interfaces whose methods return structs in that package. There's no way to avoid circular dependencies.

Options I see:

Live with the ugliness of exporting this API even though it's not useful or intended for public usage

Essentially reimplement its logic here to create realistic test jobs

Instead use rivershared/testfactory for building test jobs (wrap testfactory.Job_Build to return a *Job[T] instead of *riverdriver.JobInsertFullParams.

Went for option (3) here

rivertest/worker.go

bgentry · 2025-02-13T04:25:45Z

@brandur ~~this is still missing documentation and an example but~~ I think the implementation is essentially there. Want to take a look?

bgentry · 2025-02-13T19:23:30Z

rivertest/worker.go

+
+// WorkOpts allocates a synthetic job with the provided arguments and insert
+// options, then works it.
+func (w *Worker[T, TTx]) WorkOpts(ctx context.Context, tb testing.TB, args T, opts river.InsertOpts) error {


I opted to make a separate helper method for working a job with custom insert opts because (1) I expect the vast majority of use cases won't need to care about this and will only care about worker + args, and (2) the much more common use case will benefit from not needing to specify , nil at the end of every call.

In doing so I also opted to make the opts arg just a plain struct instead of a pointer, because it should always be provided when calling the more specialized helper method.

I opted to make a separate helper method for working a job with custom insert opts because (1) I expect the vast majority of use cases won't need to care about this and will only care about worker + args, and (2) the much more common use case will benefit from not needing to specify , nil at the end of every call.

Works for me I suppose, but just a note that you could state that exact same rationale as to why Insert should've been broken into Insert + InsertOpts.

You're right and I had a similar thought. Consolidated to just Work (mirroring client Insert method) + WorkJob for a full job struct.

bgentry · 2025-02-13T19:29:53Z

rivertest/worker.go

+// In all cases the underlying [river.Worker] will be called with the synthetic
+// job. The execution environment has a realistic River context with access to
+// all River features, including [river.ClientFromContext] and worker
+// middleware.


I realized there's a gap in this statement: river.JobCompleteTx won't be able to succeed if the job isn't actually real / isn't in the database. I see two options for working around this:

Tell users who run into this that they need to first insert a real job, and then use WorkJob to test working it.

Embed a value in the work context to indicate to JobCompleteTx that it's running within a special test helper and it shouldn't worry about whether the job exists.

Discussed a bit on Slack. Option (2) seems to be the most plausible, although stubbing out real functionality that'd otherwise hit a DB or do so something else in the real world is always a little icky. Hopefully though this is a special case where river.JobCompleteTx is already tested so well that it's not as necessary to do it here.

This turned out to be more complicated so we decided to leverage the context value to panic if JobCompleteTx is called within a test worker environment and when the job doesn't exist in the database.

bgentry · 2025-02-13T19:31:05Z

job_complete_tx.go

+	if len(rows) == 0 {
+		return nil, rivertype.ErrNotFound
+	}


Uncovered this bug while thinking about how JobCompleteTx would function if called from the new test helpers. Will add it to the changelog.

Make this util more useful with variadic args, functioning more similarly to `valutil.FirstNonZero` by returning the first non-empty value or else `nil`.

The `rivertest.Worker` type makes it much simpler to test River workers with a realistic execution environment without needing to touch the database at all. The type leverages a `river.Config` to allocate an inner `Client` and to enable jobs to be easily created with the normal default values, along with full access to features like middleweare. The execution context also contains all the normal stuff that a real context in River might, such as the client and a timeout if configured.

brandur · 2025-02-14T23:44:33Z

rivertest/worker.go

+//
+// When relying on features that require a database record (such as JobCompleteTx),
+// the job must be inserted into the database first and then executed with
+// WorkJob.


We can probably punt on this, but it'd probably make sense to have a test or something that'd make sure users have a sane way to do this.

JobCompleteTx will probably refuse to complete the job unless it's running, and there might not be that easy of a way to manipulate a job from available to running right now. They could Client.Insert, but then it'd get stuck there.

A couple possibilities:

We could provide a test helper for inserting jobs in any state similar to our internal factories.

We could have the test helpers here check to see if a job exists in the DB when running. If so, transition the job to running.

Yeah, I just ran into that while writing the test for this! 😆 I think it would make sense as a fast follow so we can refine the API a little more.

This can't succeed if the record isn't in the database, so panic if this situation occurs within a test worker environment.

Follows up #753 to make it possible to call `JobCompleteTx` inside test worker implementations. The test worker tries to update an equivalent job in the database's state to `running` so `JobCompleteTx` will be able to find it when called (it only considers jobs in `running` state). We also augment the API so that it's workable for users who want to use test transactions in their tests (which is hopefully the vast majority of users) by adding transaction variants `WorkTx` and `WorkJobTx`.

) * allow setting created_at on insert for clock overrides While the client code was attempting to set `created_at` when the clock was stubbed, the driver code was not actually utilizing this at all. This commit makes it possible to set the `created_at` (similar to `scheduled_at`) using a mocked clock. It also removes the ability to set `finalized_at`, which has no need to be set at insert (other than for tests utilizing `JobInsertFull`). * Let test workers call `JobCompleteTx` + test work transaction variants Follows up #753 to make it possible to call `JobCompleteTx` inside test worker implementations. The test worker tries to update an equivalent job in the database's state to `running` so `JobCompleteTx` will be able to find it when called (it only considers jobs in `running` state). We also augment the API so that it's workable for users who want to use test transactions in their tests (which is hopefully the vast majority of users) by adding transaction variants `WorkTx` and `WorkJobTx`. * always take tx in rivertest.Worker methods * always insert jobs for testworker * use real executor for rivertest worker, alter APIs Leverage the actual job executor for the `rivertest.Worker` type so that it closely matches real job executions. Reduce the exposed APIs so it only has `Work` and `WorkJob`, both of which require a transaction (but the latter works on existing jobs). Rather than returning only an error, also return a `WorkResult` struct that contains additional metadata. Populate it with the final updated job and the event kind to indicate whether the job completed, errored, snoozed, cancelled, etc. --------- Co-authored-by: Brandur <brandur@brandur.org>

bgentry requested a review from brandur February 11, 2025 02:43

bgentry force-pushed the bg-rivertest-work-job branch from d47c2a3 to f3226b9 Compare February 11, 2025 04:15

brandur reviewed Feb 12, 2025

View reviewed changes

rivertest/worker.go Outdated Show resolved Hide resolved

bgentry commented Feb 12, 2025

View reviewed changes

bgentry force-pushed the bg-rivertest-work-job branch from f3226b9 to ae4ee95 Compare February 13, 2025 04:23

bgentry commented Feb 13, 2025

View reviewed changes

rivertest/worker.go Outdated Show resolved Hide resolved

bgentry requested a review from brandur February 13, 2025 04:25

bgentry force-pushed the bg-rivertest-work-job branch from ae4ee95 to 1f0f526 Compare February 13, 2025 19:18

bgentry marked this pull request as ready for review February 13, 2025 19:19

bgentry force-pushed the bg-rivertest-work-job branch 2 times, most recently from d6f92fc to 52b9190 Compare February 13, 2025 19:23

bgentry commented Feb 13, 2025

View reviewed changes

bgentry force-pushed the bg-rivertest-work-job branch from 52b9190 to 958c2c2 Compare February 13, 2025 19:30

bgentry commented Feb 13, 2025

View reviewed changes

bgentry force-pushed the bg-rivertest-work-job branch 3 times, most recently from 8398517 to be5fdb6 Compare February 14, 2025 17:00

bgentry changed the base branch from master to bg-expose-test-clock February 14, 2025 17:07

bgentry force-pushed the bg-expose-test-clock branch from da869ef to 6ec16c2 Compare February 14, 2025 17:14

bgentry force-pushed the bg-rivertest-work-job branch 2 times, most recently from 4bccc25 to 12f2aaf Compare February 14, 2025 20:27

Base automatically changed from bg-expose-test-clock to master February 14, 2025 23:14

bgentry added 4 commits February 14, 2025 17:14

sliceutil.DefaultIfEmpty -> FirstNonEmpty

521905e

Make this util more useful with variadic args, functioning more similarly to `valutil.FirstNonZero` by returning the first non-empty value or else `nil`.

remove unused internal/clientconfig pkg

c743885

testfactory / insert full: allow setting AttemptedBy

37bf61d

fix panic on non-existent job in JobCompleteTx

a8569d1

brandur approved these changes Feb 14, 2025

View reviewed changes

bgentry force-pushed the bg-rivertest-work-job branch from 12f2aaf to 46ec75e Compare February 14, 2025 23:37

bgentry force-pushed the bg-rivertest-work-job branch from 46ec75e to 1a7e5ac Compare February 14, 2025 23:43

brandur approved these changes Feb 14, 2025

View reviewed changes

panic if JobCompleteTx called within a test worker

8b36c26

This can't succeed if the record isn't in the database, so panic if this situation occurs within a test worker environment.

bgentry force-pushed the bg-rivertest-work-job branch from 1a7e5ac to 8b36c26 Compare February 14, 2025 23:47

bgentry enabled auto-merge (squash) February 14, 2025 23:48

bgentry merged commit 0132e53 into master Feb 14, 2025
10 checks passed

bgentry deleted the bg-rivertest-work-job branch February 14, 2025 23:50

bgentry mentioned this pull request Feb 14, 2025

Any advice on testing worker code that calls: JobCompleteTx #605

Open

brandur mentioned this pull request Feb 15, 2025

Let test workers call JobCompleteTx + test work transaction variants #765

Closed

bgentry mentioned this pull request Feb 17, 2025

rivertest.Worker: Use job executor, always insert jobs, require tx #766

Merged

bgentry mentioned this pull request Feb 21, 2025

Job executor: Unmarshal job args late, after middlewares have run #783

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rivertest: implement a Worker type to simplify testing #753

rivertest: implement a Worker type to simplify testing #753

bgentry commented Feb 11, 2025 •

edited

Loading

brandur left a comment

bgentry Feb 12, 2025

bgentry Feb 13, 2025

bgentry commented Feb 13, 2025 •

edited

Loading

bgentry Feb 13, 2025

brandur Feb 14, 2025

bgentry Feb 14, 2025

bgentry Feb 13, 2025

brandur Feb 14, 2025

bgentry Feb 14, 2025

bgentry Feb 13, 2025

brandur Feb 14, 2025

bgentry Feb 14, 2025

rivertest: implement a Worker type to simplify testing #753

rivertest: implement a Worker type to simplify testing #753

Conversation

bgentry commented Feb 11, 2025 • edited Loading

brandur left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgentry commented Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgentry commented Feb 11, 2025 •

edited

Loading

bgentry commented Feb 13, 2025 •

edited

Loading