rivertest.Worker: Use job executor, always insert jobs, require tx #766
Conversation
There was a problem hiding this comment.
@bgentry Yep, this looks great man! I've been wanting to packag-ify job executor forever anyway, and this is a good excuse to get it done.
You mentioned potentially just removing the non-tx work variants — IMO that's a good idea given that it'd be kind of weird to leave side effects like a new row on a raw pool.
And looking at the API again, returning like a result struct like you mentioned might not be a bad idea regardless of what we put in it for future compatibility if nothing else.
| exec := w.client.Driver().UnwrapExecutor(tx) | ||
| subscribeCh := make(chan []jobcompleter.CompleterJobUpdated, 1) | ||
| archetype := riversharedtest.BaseServiceArchetype(tb) | ||
| completer := jobcompleter.NewInlineCompleter(archetype, exec, w.client.Pilot(), subscribeCh) |
There was a problem hiding this comment.
Would be a very minor saving, but I wonder if we should let this tolerate a nil subscribe channel so we can skip the allocation and the frontmatter code.
There was a problem hiding this comment.
I actually ended up leveraging this in the updated version in order to get the post-completion job row for the new return struct. We might consider refactoring it as there's a handful of channels at play, lmk once you get to review the new version (coming shortly).
bgentry
force-pushed
the
bg-test-worker-always-tx
branch
from
0ee76ca to
f6dccab
Compare
bgentry
changed the base branch from
brandur-test-worker-job-complete
to
master
bgentry
changed the title
bgentry
force-pushed
the
bg-test-worker-always-tx
branch
3 times, most recently
from
f7da1d9 to
2deeb9e
Compare
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`).
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`.
bgentry
force-pushed
the
bg-test-worker-always-tx
branch
from
2deeb9e to
87de760
Compare
|
@brandur aside from missing a changelog entry, this should be ready to go! Flaky test DB issues are still on my radar and I think are probably fixable even with the current design. |
rivertest/worker_test.go
Outdated
| tw := NewWorker(t, driver, config, worker) | ||
| require.NoError(t, tw.Work(context.Background(), t, testArgs{Value: "test"}, nil)) | ||
| tw := NewWorker(t, bundle.driver, bundle.config, worker) | ||
| res, err := tw.Work(context.Background(), t, bundle.tx, testArgs{Value: "test"}, nil) |
There was a problem hiding this comment.
Maybe replace this context.Background() with ctx since it's broadly defined now, and it cleans up a lot of visual noise.
There was a problem hiding this comment.
Done, did that in the whole file 👍
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.
bgentry
force-pushed
the
bg-test-worker-always-tx
branch
from
87de760 to
d2607ac
Compare
This updates the
rivertest.Workertype (#753) to leverage the extracted job executor logic from #768 and to avoid some of the downsides with the first pass of that design (partly outlined in #765), particularly the fact that certain database-dependent operations likeJobCompleteTxcouldn't be accurately simulated without hitting the database.As part of these changes, test jobs are always inserted into the database and the test worker methods always require a transaction to be used for additional operations on that job (such as completions). The return types were also altered so that an extensible result struct is returned in addition to any legitimate worker errors that occur. The struct contains the kind of event that occurred from execution (completion, snoozing, cancellation, etc) so that it's easy to assert against the result, even if a special not-really-an-error like snooze was returned.
To make the tests line up for this, I had to make corrections to the job insert queries to allow them to properly set the created time in a similar fashion to how they were already setting the scheduled time. This allows stub clocks to be used in tests and ensures they're properly respected throughout the execution process.