Skip to content

Allow explicit schema injection to rivertest.Require* test functions#926

Merged
brandur merged 1 commit into
masterfrom
brandur-require-insert-schema-normal-options
May 28, 2025
Merged

Allow explicit schema injection to rivertest.Require* test functions#926
brandur merged 1 commit into
masterfrom
brandur-require-insert-schema-normal-options

Conversation

@brandur

@brandur brandur commented May 24, 2025

Copy link
Copy Markdown
Contributor

Here, resolve #907 by letting an explicit schema be injected into
rivertest.Require* assertions in a similar way that one can be used in
a client.

This approach adds a schema in RequireInsertedOpts. This comment does
a good job of highlight all the potential approaches for adding a schema
[1], and unfortunately none of them are all that great. I implemented
one other version of this (a variant of option 2 in that list), which as
some advantages, but in the end it just ended up ballooning the API out
to an uncomfortable degree.

The worst part about adding schema to RequireInsertedOpts is its
interact with the RequireMany* functions, where each expectation can
set its own schema, and it's not clear what would happen if different
expectations set different schemas. I resolved this ambiguity by making
it an error to mix and match schemas. Assertions are allowed to send a
schema in only the first position like:

jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
    {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
    {Args: &Job1Args{String: "bar"}},
})

Or send the same schema in all positions:

jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
    {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
    {Args: &Job1Args{String: "bar"}, Opts: bundle.schemaOpts},
})

But they aren't allowed to set a schema only in position other than the
first, or mix and match schemas between expectations.

Fixes #907.

[1] #907 (comment)

Here, resolve #907 by letting an explicit schema be injected into
`rivertest.Require*` assertions in a similar way that one can be used in
a client.

This approach adds a schema in `RequireInsertedOpts`. This comment does
a good job of highlight all the potential approaches for adding a schema
[1], and unfortunately none of them are all that great. I implemented
one other version of this (a variant of option 2 in that list), which as
some advantages, but in the end it just ended up ballooning the API out
to an uncomfortable degree.

The worst part about adding schema to `RequireInsertedOpts` is its
interact with the `RequireMany*` functions, where each expectation can
set its own schema, and it's not clear what would happen if different
expectations set different schemas. I resolved this ambiguity by making
it an error to mix and match schemas. Assertions are allowed to send a
schema in only the first position like:

    jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
        {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
        {Args: &Job1Args{String: "bar"}},
    })

Or send the same schema in all positions:

    jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
        {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
        {Args: &Job1Args{String: "bar"}, Opts: bundle.schemaOpts},
    })

But they aren't allowed to set a schema only in position other than the
first, or mix and match schemas between expectations.

Fixes #907.

[1] #907 (comment)
@brandur brandur force-pushed the brandur-require-insert-schema-normal-options branch from 08a640d to 9049281 Compare May 24, 2025 06:10
@brandur brandur requested a review from bgentry May 24, 2025 06:14

@bgentry bgentry left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this seems like the best of the possible options and you explored those pretty thoroughly. Great pragmatic choice!

@brandur

brandur commented May 28, 2025

Copy link
Copy Markdown
Contributor Author

+1, wish there was a better option, but this should do it for now. Thanks!

@brandur brandur merged commit 5535ac7 into master May 28, 2025
10 checks passed
@brandur brandur deleted the brandur-require-insert-schema-normal-options branch May 28, 2025 14:50
brandur added a commit that referenced this pull request May 28, 2025
#926)

Here, resolve #907 by letting an explicit schema be injected into
`rivertest.Require*` assertions in a similar way that one can be used in
a client.

This approach adds a schema in `RequireInsertedOpts`. This comment does
a good job of highlight all the potential approaches for adding a schema
[1], and unfortunately none of them are all that great. I implemented
one other version of this (a variant of option 2 in that list), which as
some advantages, but in the end it just ended up ballooning the API out
to an uncomfortable degree.

The worst part about adding schema to `RequireInsertedOpts` is its
interact with the `RequireMany*` functions, where each expectation can
set its own schema, and it's not clear what would happen if different
expectations set different schemas. I resolved this ambiguity by making
it an error to mix and match schemas. Assertions are allowed to send a
schema in only the first position like:

    jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
        {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
        {Args: &Job1Args{String: "bar"}},
    })

Or send the same schema in all positions:

    jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
        {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
        {Args: &Job1Args{String: "bar"}, Opts: bundle.schemaOpts},
    })

But they aren't allowed to set a schema only in position other than the
first, or mix and match schemas between expectations.

Fixes #907.

[1] #907 (comment)
brandur added a commit that referenced this pull request May 28, 2025
#926)

Here, resolve #907 by letting an explicit schema be injected into
`rivertest.Require*` assertions in a similar way that one can be used in
a client.

This approach adds a schema in `RequireInsertedOpts`. This comment does
a good job of highlight all the potential approaches for adding a schema
[1], and unfortunately none of them are all that great. I implemented
one other version of this (a variant of option 2 in that list), which as
some advantages, but in the end it just ended up ballooning the API out
to an uncomfortable degree.

The worst part about adding schema to `RequireInsertedOpts` is its
interact with the `RequireMany*` functions, where each expectation can
set its own schema, and it's not clear what would happen if different
expectations set different schemas. I resolved this ambiguity by making
it an error to mix and match schemas. Assertions are allowed to send a
schema in only the first position like:

    jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
        {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
        {Args: &Job1Args{String: "bar"}},
    })

Or send the same schema in all positions:

    jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
        {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
        {Args: &Job1Args{String: "bar"}, Opts: bundle.schemaOpts},
    })

But they aren't allowed to set a schema only in position other than the
first, or mix and match schemas between expectations.

Fixes #907.

[1] #907 (comment)
brandur added a commit that referenced this pull request May 28, 2025
#926) (#933)

Here, resolve #907 by letting an explicit schema be injected into
`rivertest.Require*` assertions in a similar way that one can be used in
a client.

This approach adds a schema in `RequireInsertedOpts`. This comment does
a good job of highlight all the potential approaches for adding a schema
[1], and unfortunately none of them are all that great. I implemented
one other version of this (a variant of option 2 in that list), which as
some advantages, but in the end it just ended up ballooning the API out
to an uncomfortable degree.

The worst part about adding schema to `RequireInsertedOpts` is its
interact with the `RequireMany*` functions, where each expectation can
set its own schema, and it's not clear what would happen if different
expectations set different schemas. I resolved this ambiguity by making
it an error to mix and match schemas. Assertions are allowed to send a
schema in only the first position like:

    jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
        {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
        {Args: &Job1Args{String: "bar"}},
    })

Or send the same schema in all positions:

    jobs := requireManyInserted(ctx, bundle.mockT, bundle.driver, []ExpectedJob{
        {Args: &Job1Args{String: "foo"}, Opts: bundle.schemaOpts},
        {Args: &Job1Args{String: "bar"}, Opts: bundle.schemaOpts},
    })

But they aren't allowed to set a schema only in position other than the
first, or mix and match schemas between expectations.

Fixes #907.

[1] #907 (comment)
zmwangx added a commit to zmwangx/river that referenced this pull request May 31, 2026
…Worker`

`rivertest.Worker` works a job in three database steps: it inserts the job
through the client, builds an inline completer, and transitions the job to
`running` via `JobUpdateFull`. The first two already thread `config.Schema`
through — the client uses it internally, and it's passed explicitly to
`jobcompleter.NewInlineCompleter` — but the `JobUpdateFull` call omitted it.

With a non-default `Schema`, the insert lands the job in `<schema>.river_job`
correctly, then the running-state update runs unqualified and resolves only
through the connection's `search_path`. On a connection that doesn't include
the configured schema it fails one step later with:

    test worker internal error: failed to update job to running state: ERROR: relation "river_job" does not exist (SQLSTATE 42P01)

Pass `w.config.Schema` into `JobUpdateFullParams` so all three steps agree on
the schema. This finishes custom-schema support for `rivertest.Worker`; the
`rivertest.Require*` family received an analogous per-call `Schema` option in
riverqueue#926 (riverqueue#907).

The regression tests migrate River into an isolated named schema and work jobs
through a transaction whose `search_path` is empty, so the tables resolve only
via schema qualification — the exact condition that fails before this change.
They live as `CustomSchema` subtests of `TestWorker_Work` and
`TestWorker_WorkJob`, each building its own bundle inline since the schema setup
differs from those tests' shared `setup`.
zmwangx added a commit to zmwangx/river that referenced this pull request May 31, 2026
…Worker`

`rivertest.Worker` works a job in three database steps: it inserts the job
through the client, builds an inline completer, and transitions the job to
`running` via `JobUpdateFull`. The first two already thread `config.Schema`
through — the client uses it internally, and it's passed explicitly to
`jobcompleter.NewInlineCompleter` — but the `JobUpdateFull` call omitted it.

With a non-default `Schema`, the insert lands the job in `<schema>.river_job`
correctly, then the running-state update runs unqualified and resolves only
through the connection's `search_path`. On a connection that doesn't include
the configured schema it fails one step later with:

    test worker internal error: failed to update job to running state: ERROR: relation "river_job" does not exist (SQLSTATE 42P01)

Pass `w.config.Schema` into `JobUpdateFullParams` so all three steps agree on
the schema. This finishes custom-schema support for `rivertest.Worker`; the
`rivertest.Require*` family received an analogous per-call `Schema` option in
riverqueue#926 (riverqueue#907).

The regression tests migrate River into an isolated named schema and work jobs
through a transaction whose `search_path` is empty, so the tables resolve only
via schema qualification — the exact condition that fails before this change.
They live as `CustomSchema` subtests of `TestWorker_Work` and
`TestWorker_WorkJob`, each building its own bundle inline since the schema setup
differs from those tests' shared `setup`.
brandur pushed a commit that referenced this pull request May 31, 2026
…Worker` (#1262)

`rivertest.Worker` works a job in three database steps: it inserts the job
through the client, builds an inline completer, and transitions the job to
`running` via `JobUpdateFull`. The first two already thread `config.Schema`
through — the client uses it internally, and it's passed explicitly to
`jobcompleter.NewInlineCompleter` — but the `JobUpdateFull` call omitted it.

With a non-default `Schema`, the insert lands the job in `<schema>.river_job`
correctly, then the running-state update runs unqualified and resolves only
through the connection's `search_path`. On a connection that doesn't include
the configured schema it fails one step later with:

    test worker internal error: failed to update job to running state: ERROR: relation "river_job" does not exist (SQLSTATE 42P01)

Pass `w.config.Schema` into `JobUpdateFullParams` so all three steps agree on
the schema. This finishes custom-schema support for `rivertest.Worker`; the
`rivertest.Require*` family received an analogous per-call `Schema` option in
#926 (#907).

The regression tests migrate River into an isolated named schema and work jobs
through a transaction whose `search_path` is empty, so the tables resolve only
via schema qualification — the exact condition that fails before this change.
They live as `CustomSchema` subtests of `TestWorker_Work` and
`TestWorker_WorkJob`, each building its own bundle inline since the schema setup
differs from those tests' shared `setup`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

rivertest.RequireX for alternative schema

2 participants