Implement rivertest.RequireNotInserted test helper (inverse of RequireInserted) - #237
Conversation
| // specify `Priority: 3`, and a joke of the same kind was inserted, but it was | ||
| // `Priority: 2`, RequireNotInserted will not fail. If the inserted job was | ||
| // `Priority: 2` (therefore matching the job), RequireNotInserted does fail. | ||
| func RequireNotInserted[TDriver riverdriver.Driver[TTx], TTx any, TArgs river.JobArgs](ctx context.Context, tb testing.TB, driver TDriver, expectedJob TArgs, opts *RequireInsertedOpts) { |
There was a problem hiding this comment.
Okay, so I went back and forth a lot on what opts this function should accept.
You can see here that it takes RequireInsertedOpts, which is the same as RequireInserted. Initially, this felt a little wrong to me, but after implementing it as a different struct, having a totally duplicated set of fields on a different struct just felt wasteful.
I then thought about hacking it a bit with something like:
type RequireNotInsertedOpts RequireInsertedOptsWhich would let us futureproof a bit by changing the type definition to a full struct in the future in case we needed to. But it has the unfortunate downside of being worse for IDE mouseover definitions of the struct, because the properties of RequireInsertedOpts aren't directly visible.
Lastly, I considered just not giving this function an opts at all. The purpose of the opts is so you could do something like:
RequireNotInserted(..., &RequireInsertedOpts{Queue: "high-priority"})To verify that a job of that type did not go the high property queue. So if a job the kind went to the default queue, it's okay, but it fails if it went to high priority.
However, one could also imagine asserting on this affirmatively instead by inverting. Something like:
RequireInserted(..., &RequireInsertedOpts{Queue: "default"})Which might be clearer than the use of RequireNotInserted anyway.
That said, if we decide not to give RequireNotInserted options, then realize later that we need them for something, it's a breaking change to add them in, which would be nice to avoid.
Any immediate thoughts on all that?
There was a problem hiding this comment.
See my above comment, I think the behavior on this might be backwards.
But as far as the type thing, I would definitely avoid the aliasing option because it results in poorer usability. If we define these types right next to each other, it wouldn't be that hard to keep them lined up. Or alternatively if we can get away with just using the same type in both places, that's even better.
1bba7a8 to
136f0ea
Compare
36eb88c to
12f2369
Compare
| // When used with RequireNotInserted, multiple properties act like an OR instead | ||
| // of an AND. If an inserted job is found whose properties match any of the set | ||
| // opts properties, a test failure is triggered. |
There was a problem hiding this comment.
I don't this behavior is what I would expect here. My intuition (based on how I've used queue assertions for other libraries/ecosystems) is that I'm allowed to be as specific as I desire here, and that the assertion will ensure that no jobs have been inserted which match all of the conditions I specified. If I want, I can easily make sure that no jobs have been inserted on a certain queue, or if I want to be more specific, I can make sure that no jobs have been inserted on that queue and with that job type.
And in the affirmative case where I want to make sure a job was inserted, it likewise matches all of the criteria I specify, but ignores all those which I don't.
| // specify `Priority: 3`, and a joke of the same kind was inserted, but it was | ||
| // `Priority: 2`, RequireNotInserted will not fail. If the inserted job was | ||
| // `Priority: 2` (therefore matching the job), RequireNotInserted does fail. | ||
| func RequireNotInserted[TDriver riverdriver.Driver[TTx], TTx any, TArgs river.JobArgs](ctx context.Context, tb testing.TB, driver TDriver, expectedJob TArgs, opts *RequireInsertedOpts) { |
There was a problem hiding this comment.
See my above comment, I think the behavior on this might be backwards.
But as far as the type thing, I would definitely avoid the aliasing option because it results in poorer usability. If we define these types right next to each other, it wouldn't be that hard to keep them lined up. Or alternatively if we can get away with just using the same type in both places, that's even better.
12f2369 to
97fbdcc
Compare
|
@bgentry Alright I ended up flipping the conditions on |
97fbdcc to
53d9ed6
Compare
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Nice work! This should be super useful.
| // Multiple properties set on this struct increase the specifity on a job to | ||
| // match, acting like an AND condition on each. | ||
| // | ||
| // In the case of RequireInserted or RequireInsertdMany, if multiple properties |
There was a problem hiding this comment.
| // In the case of RequireInserted or RequireInsertdMany, if multiple properties | |
| // In the case of RequireInserted or RequireInsertedMany, if multiple properties |
| if jobRow.MaxAttempts == expectedOpts.MaxAttempts { | ||
| if requireNotInserted { | ||
| failures = append(failures, fmt.Sprintf("max attempts equal to excluded %d", expectedOpts.MaxAttempts)) | ||
| } | ||
| } else { | ||
| if requireNotInserted { | ||
| return true // any one property doesn't match; assertion passes | ||
| } else { | ||
| failures = append(failures, fmt.Sprintf("max attempts %d not equal to expected %d", jobRow.MaxAttempts, expectedOpts.MaxAttempts)) | ||
| } | ||
| } |
There was a problem hiding this comment.
There's definitely a lot of duplication in these attr comparisons. Do you think it'd be worth extracting a generic helper for these comparable attrs (int, string)?
There was a problem hiding this comment.
Looked into this one. I'm not against the idea, but the helper would have to take a lot of different parameters to replicate that functionality that's in here now, and even if it's DRYer, I think overall it might obscure rather than clarify. The special cases of ScheduledAt and Tags would also be much harder to shoehorn in there and would probably have to stay on there own, thereby making the overall value a bit more dubious again.
| A new `river_queue` table is introduced in the v4 migration for this purpose. Upon startup, every producer in each River `Client` will make an `UPSERT` query to the database to either register the queue as being active, or if it already exists it will instead bump the timestamp to keep it active. This query will be run periodically in each producer as long as the `Client` is alive, even if the queue is paused. A separate query will delete/purge any queues which have not been active in awhile (currently fixed to 24 hours). | ||
|
|
||
| `QueuePause` and `QueueResume` APIs have been introduced to `Client` pause and resume a single queue by name, or _all_ queues using the special `*` value. Each producer will watch for notifications on the relevant `LISTEN/NOTIFY` topic unless operating in poll-only mode, in which case they will periodically poll for changes to their queue record in the database. | ||
| - `RequireNotInserted` test helper (in addition to the existing `RequireInserted`) that verifies that a job with matching conditions was _not_ inserted. [PR #237](https://github.com/riverqueue/river/pull/237). |
There was a problem hiding this comment.
We missed the release on this so unfortunately this will need to get rebased and pushed to the unreleased section.
There was a problem hiding this comment.
Moved it out to "unreleased".
…uireInserted`) Here, introduce a new test helper for `rivertest.RequireNotInserted`, which fails in cases where a job matching in the input kind and (optional) properties _was_ inserted rather than was not. Its purpose is to invert the checks made by the existing `RequireInserted`. Unlike `RequireInserted`, I _didn't_ implement a batch version of the helper like `RequireNotInsertedMany`. I was going to do it, but after thinking about it for a while, I don't think it really makes sense. `RequireInsertedMany` is useful because it allows you to verify a sequence of job inserts that occurred in a particular order with particular options. There's no sequence for jobs that weren't inserted, so the only thing a `RequireNotInsertedMany` would do is verify that many different job kinds were all not inserted as part of one operation, which doesn't really seem that useful. I figure if there's demand we can think about adding a batch helper, but it may be better to not do so prospectively to keep the API smaller, and avoiding adding a function that doesn't end up getting any use.
53d9ed6 to
88ae276
Compare
|
Thx! |
|
Actually, I guess that wasn't an approval, but lemme know if you disagree with the above. |
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Meant to approve, sorry!
|
Oh cool. No worries! |
2 participants
Here, introduce a new test helper for
rivertest.RequireNotInserted,which fails in cases where a job matching in the input kind and
(optional) properties was inserted rather than was not. Its purpose is
to invert the checks made by the existing
RequireInserted.Unlike
RequireInserted, I didn't implement a batch version of thehelper like
RequireNotInsertedMany. I was going to do it, but afterthinking about it for a while, I don't think it really makes sense.
RequireInsertedManyis useful because it allows you to verify asequence of job inserts that occurred in a particular order with
particular options. There's no sequence for jobs that weren't inserted,
so the only thing a
RequireNotInsertedManywould do is verify thatmany different job kinds were all not inserted as part of one operation,
which doesn't really seem that useful. I figure if there's demand we can
think about adding a batch helper, but it may be better to not do so
prospectively to keep the API smaller, and avoiding adding a function
that doesn't end up getting any use.