Add missing integration-level job snooze test #31
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bgentry
approved these changes
Nov 16, 2023
Comment on lines
+226
to
+229
| AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { | ||
| return JobCancel(fmt.Errorf("a persisted internal error")) | ||
| })) |
There was a problem hiding this comment.
Should we check the job in the database after this is all done for completeness? i.e. make sure it is cancelled and the error is saved
Comment on lines
+250
to
+258
| AddWorker(client.config.Workers, WorkFunc(func(ctx context.Context, job *Job[JobArgs]) error { | ||
| return JobSnooze(15 * time.Minute) | ||
| })) |
There was a problem hiding this comment.
Likewise, should we check anything about the job in the DB after this executes? Since this is an integration test it might be a good safety check.
I noticed while trying to refactor something the other day that I had a problem around snoozing, but only the example snooze test was failing. Looking into it further, it turned out that it was because the snooze example was the only snooze test we had. Example tests aren't great because they do obnoxious things like swallow all output (making it harder to debug), but they also don't support some feature like `-count`, making it harder to debug intermittent problems. Here, add another client test for snooze. We use the new client test line that makes use of `JobArgsReflectKind` and `WorkFunc`, keeping the implementation clean and all local to the specific test case that needs it. And since snooze and cancel are quite similar, I added a test case for cancel as well. Cancel did have another test elsewhere, but I figure I might as well add one in this new line too to line up with the snooze test case. It should be a pretty fast test, so doesn't hurt to have a duplicate.
brandur
force-pushed
the
brandur-snooze-test
branch
from
7f94356 to
ce5f5da
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I noticed while trying to refactor something the other day that I had a
problem around snoozing, but only the example snooze test was failing.
Looking into it further, it turned out that it was because the snooze
example was the only snooze test we had.
Example tests aren't great because they do obnoxious things like swallow
all output (making it harder to debug), but they also don't support some
feature like
-count, making it harder to debug intermittent problems.Here, add another client test for snooze. We use the new client test
line that makes use of
JobArgsReflectKindandWorkFunc, keeping theimplementation clean and all local to the specific test case that needs
it.
And since snooze and cancel are quite similar, I added a test case for
cancel as well. Cancel did have another test elsewhere, but I figure I
might as well add one in this new line too to line up with the snooze
test case. It should be a pretty fast test, so doesn't hurt to have a
duplicate.