Implement `rivertest.RequireNotInserted` test helper (inverse of `RequireInserted`) #237

brandur · 2024-02-28T04:03:18Z

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.

brandur · 2024-02-28T04:09:10Z

rivertest/rivertest.go

+// 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) {


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 RequireInsertedOpts

Which 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?

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.

bgentry · 2024-03-29T04:09:29Z

rivertest/rivertest.go

+// 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.


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.

bgentry · 2024-03-29T04:14:52Z

rivertest/rivertest.go

+// 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) {


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.

brandur · 2024-05-03T09:35:09Z

@bgentry Alright I ended up flipping the conditions on RequireNotInserted options so they act like an AND instead of an OR. Also ended up improving the error messages so that if multiple properties on an inserted job row do or don't match, we return all of them instead of just the first. Added additional tests to check everything. Want to take another look?

bgentry

Nice work! This should be super useful.

bgentry · 2024-05-03T19:10:37Z

rivertest/rivertest.go

+// 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


Suggested change

// In the case of RequireInserted or RequireInsertdMany, if multiple properties

// In the case of RequireInserted or RequireInsertedMany, if multiple properties

Thx. Fixed.

bgentry · 2024-05-03T19:15:46Z

rivertest/rivertest.go

+		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'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)?

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.

bgentry · 2024-05-03T19:26:55Z

CHANGELOG.md

@@ -19,6 +19,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
  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).


We missed the release on this so unfortunately this will need to get rebased and pushed to the unreleased section.

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.

brandur · 2024-05-04T02:50:48Z

Thx!

brandur · 2024-05-04T02:51:28Z

Actually, I guess that wasn't an approval, but lemme know if you disagree with the above.

bgentry

Meant to approve, sorry!

brandur · 2024-05-04T04:54:12Z

Oh cool. No worries!

brandur commented Feb 28, 2024

View reviewed changes

brandur force-pushed the brandur-require-not-inserted branch from 1bba7a8 to 136f0ea Compare February 28, 2024 04:09

brandur requested a review from bgentry February 28, 2024 04:13

brandur force-pushed the brandur-require-not-inserted branch 2 times, most recently from 36eb88c to 12f2369 Compare March 19, 2024 02:36

bgentry reviewed Mar 29, 2024

View reviewed changes

brandur force-pushed the brandur-require-not-inserted branch from 12f2369 to 97fbdcc Compare May 3, 2024 09:27

brandur requested a review from bgentry May 3, 2024 09:35

brandur force-pushed the brandur-require-not-inserted branch from 97fbdcc to 53d9ed6 Compare May 3, 2024 09:36

bgentry reviewed May 3, 2024

View reviewed changes

brandur force-pushed the brandur-require-not-inserted branch from 53d9ed6 to 88ae276 Compare May 4, 2024 02:40

brandur requested a review from bgentry May 4, 2024 02:51

bgentry approved these changes May 4, 2024

View reviewed changes

brandur merged commit 105a830 into master May 4, 2024
10 checks passed

brandur deleted the brandur-require-not-inserted branch May 4, 2024 04:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `rivertest.RequireNotInserted` test helper (inverse of `RequireInserted`) #237

Implement `rivertest.RequireNotInserted` test helper (inverse of `RequireInserted`) #237

brandur commented Feb 28, 2024

brandur Feb 28, 2024

bgentry Mar 29, 2024

bgentry Mar 29, 2024

bgentry Mar 29, 2024

brandur commented May 3, 2024

bgentry left a comment

bgentry May 3, 2024

brandur May 4, 2024

bgentry May 3, 2024

brandur May 4, 2024

bgentry May 3, 2024

brandur May 4, 2024

brandur commented May 4, 2024

brandur commented May 4, 2024

bgentry left a comment

brandur commented May 4, 2024

	// In the case of RequireInserted or RequireInsertdMany, if multiple properties
	// In the case of RequireInserted or RequireInsertedMany, if multiple properties

Implement rivertest.RequireNotInserted test helper (inverse of RequireInserted) #237

Implement rivertest.RequireNotInserted test helper (inverse of RequireInserted) #237

Conversation

brandur commented Feb 28, 2024

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

brandur commented May 3, 2024

bgentry left a 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

brandur commented May 4, 2024

brandur commented May 4, 2024

bgentry left a comment

Choose a reason for hiding this comment

brandur commented May 4, 2024

Implement `rivertest.RequireNotInserted` test helper (inverse of `RequireInserted`) #237

Implement `rivertest.RequireNotInserted` test helper (inverse of `RequireInserted`) #237