rivertest.RequireX for alternative schema

[advdv]

We have our River client use an alternative schema: "workers". And we go through great lengths to make our tests mirror the actual database setup so we can't really change this. But now that i've started working with the rivertest package this doesn't seem to be supported. I simply get:

    River assertion failure:
            Internal failure: error querying jobs: ERROR: relation "river_job" does not exist (SQLSTATE 42P01)

This seems like it is a known limitations because of the comment i've found here:

river/rivertest/rivertest.go

Line 23 in 487814e

const emptySchema = ""

Is there any workaround?

[brandur]

Ugh, I was hoping we'd get a little more time before someone ran into this.

This is definitely a missing feature. I would've added it already except that doing so on top of the current APIs would be pretty ugly, so I need to play around with few alternatives and hopefully find a good one.

There is a workaround, which is to set search_path in tests similar to how River worked before the addition of the Schema parameter. More docs here:

https://riverqueue.com/docs/alternate-schema#configuring-a-schema-in-search-path

[advdv]

Oops, sorry. If it helps, it is not a real blocker for us. We've settled on a testing utility that calls the Client instance directly to fetch jobs. It was not that hard to write. I did something thinking myself, but i agree the options for rivertest are not pretty:

• Add yet another arg to the RequireX functions (gets in the way for most, breaking change probably)
• Switch to functional param/options for the RequireX functions (breaking change, adds complexity, but future proof)
• Pass the schema via the context.Context (unintuitive/magic, but not a breaking change)
• Set it as a global variable (easy to add, but global state is a bad idea, especially for testing)
• Add to the RequireInsertedOpts (easy to add, no breaking change, but breaks the abstraction since its not an InsertOpt)

Another approach that may would help us if the Rivertest package exposed the job assertion code, separately from the "get a job from the client part". That way developers could provide any means they have to read jobs through their client themselves and pass it to the assertion functions that rivertest provides. But at that point i'm not sure if the package's public surface becomes to big and the implementations to shallow.

[brandur]

• Add yet another arg to the RequireX functions (gets in the way for most, breaking change probably)
• Switch to functional param/options for the RequireX functions (breaking change, adds complexity, but future proof)
• Pass the schema via the context.Context (unintuitive/magic, but not a breaking change)
• Set it as a global variable (easy to add, but global state is a bad idea, especially for testing)
• Add to the RequireInsertedOpts (easy to add, no breaking change, but breaks the abstraction since its not an InsertOpt)

Yes, exactly correct. I also thought about all these options, and there's multiple that would work, but none are obviously amazing.

Tonight I prototyped a variant of (2) which puts in a new set of *Opt functions that takes a new options struct, but leaves the old functions in place so we don't have a breaking change. It works, but I really hate how we double the current 6x exported functions to 12x.

I also prototyped the last option (5). It works okay, but somewhat breaks the abstraction like you said, and is not particularly friendly for the RequireInsertedMany variants which could theoretically specify multiple schemas. I'm leaning toward this option as most plausible, but still find it a little ugly.

[advdv]

Tonight I prototyped a variant of (2) which puts in a new set of *Opt functions that takes a new options struct, but leaves the old functions in place so we don't have a breaking change. It works, but I really hate how we double the current 6x exported functions to 12x.

Agreed,

I also prototyped the last option (5). It works okay, but somewhat breaks the abstraction like you said, and is not particularly friendly for the RequireInsertedMany variants which could theoretically specify multiple schemas. I'm leaning toward this option as most plausible, but still find it a little ugly.

Sounds like the least ugly one indeed.

Btw, another option is not fix this right now. Our setup is arguable a lot more "e2e" than what this package is designed for: we initiate a full empty database with schemas for each test (https://github.com/peterldowns/pgtestdb). Ignoring it for now gives others time to chime in.