Refactor feature flag definitions, add durable periodic jobs UI #435
Conversation
c493ad7 to
310a2c5
Compare
riverproui/endpoints_test.go
Outdated
| "riverqueue.com/riverui/internal/riverinternaltest" | ||
| ) | ||
|
|
||
| func TestProEndpointsExtensions(t *testing.T) { //nolint:paralleltest |
There was a problem hiding this comment.
FYI, alternatively these could use the schema test helpers + parallel as long as they're provided with the "no schema reuse" option (so schema changes don't accidentally land in a subsequent test).
There was a problem hiding this comment.
Oh good call. I went to look into this and there were some complications, including that this repo is still using a forked internal riverinternaltest (you can see from the diff you commented on). I had to refactor a few test setup bits to make this work, but I think everything is all converted over as of the last commit and this file no long disables parallel tests.
Move all Pro-related flags into the extensions setup so there's no need to check for these Pro artifacts in OSS. Also add a flag for durable periodic jobs.
Also fix Queue stories that depend on feature flags.
c3fdb97 to
7e4222e
Compare
|
@brandur Please take a look at the final commit here if you have time (you may have further suggestions to clean it up). I'll merge this meanwhile and am happy to follow up on any fixes. |
|
|
||
| pool := riversharedtest.DBPool(ctx, tb) | ||
| driver := riverpropgxv5.New(pool) | ||
| tx, schema := riverdbtest.TestTx(ctx, tb, driver, opts) |
There was a problem hiding this comment.
Hm, I'm not sure about these helpers like this that just abstract away setting like three variables. It doesn't save much code, adds a layer of indirection, and makes it such that if you ever need a more complex test case that needs access to something else like the database pool, you can't get to it.
There was a problem hiding this comment.
I think the issue is the underlying helper in riverdbtest.TestTxPgx isn't quite composable enough because it constructs its own (non-Pro) driver and doesn't return the driver. I have a cleanup in riverqueue/river#1048 that I think takes care of these concerns and avoids the need to duplicate nearly-identical helpers in a few places.
|
|
||
| // The same thing as the built-in riverdbtest.TestTxPgx does. | ||
| _, err := tx.Exec(ctx, "SET search_path TO '"+schema+"'") | ||
| require.NoError(tb, err) |
There was a problem hiding this comment.
Isn't this already in the test tx helper?
There was a problem hiding this comment.
Yes, but it uses a non-pro driver and thus doesn't apply pro migrations.
There was a problem hiding this comment.
Refactored version uses the more flexible helper exposed in riverqueue/river#1048 to avoid duplicating all of this.
Also, to keep naming more intuitive and consistent with riverdbtest, I renamed this riverprodbtest instead of protestinternal.
common_test.go
Outdated
| _, err := tx.Exec(ctx, "SET search_path TO '"+schema+"'") | ||
| require.NoError(tb, err) | ||
|
|
||
| return driver, tx |
There was a problem hiding this comment.
Isn't this the same as your other new test helper below?
There was a problem hiding this comment.
Removed thanks to reusable helper in riverqueue/river#1048
c88c99d to
e6f8860
Compare
| "riverqueue.com/riverpro/driver/riverpropgxv5" | ||
| ) | ||
|
|
||
| func DriverAndTestTx(ctx context.Context, tb testing.TB, opts *riverdbtest.TestTxOpts) (driver.ProDriver[pgx.Tx], pgx.Tx) { |
There was a problem hiding this comment.
One more small ask, but can we get rid of this? It's a helper that (1) defines one variable, and (2) calls another helper. It's mostly used by shared setup blocks anyway, so it's not like this code needs to be repeated in that many places (four places by my count).
There was a problem hiding this comment.
Yeah, done ✅ It was doing a lot more before upstreaming the flexible helper and no longer makes sense as-is.
Also refactor to leverage shared DB test TX helpers and parallelize endpoint detection tests which modify the schema (by disbling sharing on those tests).
e6f8860 to
aede13b
Compare
I wanted to refactor this so feature extensions are defined entirely in pro and not in the OSS Go code. I can still do a little more cleanup here to consolidate some semi-redundant flags (both "queries" vs "table" flags for some features) but will do that later.
I then added a flag for durable periodic jobs, and finally implemented a UI for listing durable periodic jobs.
In the future I'd like to link to jobs in the list produced by this periodic job but we don't have metadata filtering yet.