Drop client Schema configuration
#18
Merged
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
brandur
commented
Nov 13, 2023
| @@ -141,7 +142,7 @@ func newTestClient(ctx context.Context, t *testing.T, config *Config) *Client[pg | |||
| return client | |||
| } | |||
|
|
|||
| func runClient(ctx context.Context, t *testing.T, client *Client[pgx.Tx]) { | |||
| func startClient(ctx context.Context, t *testing.T, client *Client[pgx.Tx]) { | |||
There was a problem hiding this comment.
Sorry for the spam, but renamed this since we renamed Run -> Start.
brandur
commented
Nov 13, 2023
| @@ -1896,7 +1967,7 @@ func Test_Client_UnknownJobKindErrorsTheJob(t *testing.T) { | |||
| require.NoError(client.Stop(ctx)) | |||
| } | |||
|
|
|||
| func Test_Client_Run_Error(t *testing.T) { | |||
| func Test_Client_Start_Error(t *testing.T) { | |||
There was a problem hiding this comment.
Same idea here. Run -> Start.
brandur
commented
Nov 13, 2023
| return client | ||
| } | ||
|
|
||
| func Test_Client(t *testing.T) { |
There was a problem hiding this comment.
Started a new branch of client tests because it's not clear what should go where in here anymore, and I'm trying to put in something where new tests can be added without starting a fresh test every time. We'll try to take advantage of JobArgsReflectKind and WorkFunc in subtests so that args and work definition can be colocated right inside the test rather than being floating around in global state somewhere.
StartInsertAndWork below is a basic client test (not related to the Schema change) added to show roughly what this look like.
bgentry
approved these changes
Nov 13, 2023
As discussed in #15, the `Schema` configuration parameter doesn't actually do anything, and we realized that it's possible to support this feature by using an alternate configuration for pgxpool in `search_path`. Here, drop `Schema` and add a test that verifies that this technique works. So as to not have to create and migrate an alternate schema, we cheat a little bit by repointing the schema and then just verifying that we can't find a `river_job` table there. I'm finding the test hierarchy in `client_test.go` to be pretty confusing in determining on what best practice is for what should go where, so I'm starting out with a new `Test_Client` which I'm hoping we can start using as a base for most of the general tests in here in a more reusable way. I leverage `JobArgsReflectKind` and `WorkFunc` in its subtests so that it's not dependent on our various global job args that are defined all over the place. Instead, args and the work routine are colocated right inside the test. Configuring an alternate schema is definitely going to need its own documentation page somewhere because it's not obvious, but that'll come separately. Fixes #15.
brandur
force-pushed
the
brandur-drop-schema-config
branch
from
e0ffce5
to
32b8d3e
Compare
|
TY TY. |
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.
As discussed in #15, the
Schemaconfiguration parameter doesn'tactually do anything, and we realized that it's possible to support this
feature by using an alternate configuration for pgxpool in
search_path.Here, drop
Schemaand add a test that verifies that this techniqueworks. So as to not have to create and migrate an alternate schema, we
cheat a little bit by repointing the schema and then just verifying that
we can't find a
river_jobtable there.I'm finding the test hierarchy in
client_test.goto be prettyconfusing in determining on what best practice is for what should go
where, so I'm starting out with a new
Test_Clientwhich I'm hoping wecan start using as a base for most of the general tests in here in a
more reusable way. I leverage
JobArgsReflectKindandWorkFuncin itssubtests so that it's not dependent on our various global job args that
are defined all over the place. Instead, args and the work routine are
colocated right inside the test.
Configuring an alternate schema is definitely going to need its own
documentation page somewhere because it's not obvious, but that'll come
separately.
Fixes #15.