Bring TestTx helper into rivershared for use in other projects#814
Bring TestTx helper into rivershared for use in other projects#814
TestTx helper into rivershared for use in other projects#814Conversation
I found in another demo project that I wanted to have access to the `TestTx` helper for tests, but that we weren't exporting it anywhere. Here, bring it into `rivershared` for easy access everywhere. I put two variants in for now, the simpler `TestTx` that uses a pool to `TEST_DATABASE_URL` or `river_test`, and a `TestTxPool` that lets you inject your own pool. The former is in use for the time being by `riverinternaltest` so it can do its existing connection customization logic, but as we fully migrate it to `rivershared`, we should see if we can drop that along with `TestTxPool` maybe. Even if we don't drop the latter, it's not the worst thing to have around because it definitely increases flexibility. I also added some code that uses `context.WithoutCancel` to be able to issue a full rollback in `TestTx` cleanup. This isn't needed by the main River project yet, but probably will be soon. When switching to the new Go `t.Context` in tests, the context is cancelled before cleanup phases are called, so without these changes the rollback will fail.
|
|
||
| // DBPool gets a lazily initialized database pool for `TEST_DATABASE_URL` or | ||
| // `river_test` if the former isn't specified. | ||
| func DBPool(ctx context.Context, tb testing.TB) *pgxpool.Pool { |
There was a problem hiding this comment.
I wonder if this should take an arg for DB URL rather than assuming a particular env? It makes it slightly more verbose to use but also much more flexible if other projects want to use their own schemas and DBs instead of sharing those used by the main River test suite.
It could even require.NotEmpty on the URL so the caller doesn't need to use a mustGetEnv type util when calling it.
I guess the downside is that this arg would need to cascade down through TestTx and TestTxPool, whose APIs are more likely to be called in a lot of places.
There was a problem hiding this comment.
Yeah, good idea. I went back and forth on these quite a lot because it's hard to find a good balance between good customizability and encouraging good default convention everywhere.
Just since this is specifically in riversharedtest (strongly implying use in testing), why don't we see if we can get everything on the convention of TEST_DATABASE_URL/river_test. In case this is turns out to be harde than expected, we can swap that out early for a more injectable env version.
| // Tests may inerit context from `t.Context()` which is cancelled after | ||
| // tests run and before calling clean up. We need a non-cancelled | ||
| // context to issue rollback here, so use a bit of a bludgeon to do so | ||
| // with `context.WithoutCancel()`. | ||
| ctx := context.WithoutCancel(ctx) |
There was a problem hiding this comment.
This of course means this rollback could hang ~indefinitely since it no longer inherits cancellation from tb.Context(), but that's probably fine within a test suite. Worst case you could re-apply a moderate timeout here.
There was a problem hiding this comment.
Yeah I suppose it's technically possible, but I think it's another one of those things that doesn't really happen in practice. I've had something similar to this in place at work for many months now over which time we've done millions of test runs and I've never seen it happen even one time.
|
Thanks! |
2 participants
I found in another demo project that I wanted to have access to the
TestTxhelper for tests, but that we weren't exporting it anywhere.Here, bring it into
riversharedfor easy access everywhere.I put two variants in for now, the simpler
TestTxthat uses a pool toTEST_DATABASE_URLorriver_test, and aTestTxPoolthat lets youinject your own pool. The former is in use for the time being by
riverinternaltestso it can do its existing connection customizationlogic, but as we fully migrate it to
rivershared, we should see if wecan drop that along with
TestTxPoolmaybe. Even if we don't drop thelatter, it's not the worst thing to have around because it definitely
increases flexibility.
I also added some code that uses
context.WithoutCancelto be able toissue a full rollback in
TestTxcleanup. This isn't needed by the mainRiver project yet, but probably will be soon. When switching to the new
Go
t.Contextin tests, the context is cancelled before cleanup phasesare called, so without these changes the rollback will fail.