Run datastore tests against multiple schemas
#1277
Conversation
|
Some additional notes here:
|
aggregator_core/src/datastore.rs
Outdated
| #[rstest] | ||
| // The version numbers in these cases must match SUPPORTED_SCHEMA_VERSIONS | ||
| #[case::version_20230405185602(ephemeral_datastore_max_schema_version(20230405185602))] | ||
| #[case::version_20230417204528(ephemeral_datastore_max_schema_version(20230417204528))] |
There was a problem hiding this comment.
Ideally this list of cases would be automatically stamped out from SUPPORTED_SCHEMA_VERSIONS, but I am not sure how to do that gracefully.
There was a problem hiding this comment.
As declarative macros cannot produce attributes, we'd have to write our own procedural macro in order to do that. We can either do so as a progressive enhancement later, or just make coupled changes to the template and SUPPORTED_SCHEMA_VERSIONS by hand.
There was a problem hiding this comment.
I think the main value of rstest is that we run each datastore test on each supported schema version. However, I don't like that we have to remember to manually add a case each time we add a new schema version -- we should have one canonical list of supported schemas, and that is SUPPORTED_SCHEMAS. Currently, if I add a version to SUPPORTED_SCHEMAS but forget to add a case here, the tests would pass even if the new schema versions was broken; I (or a reviewer) have to manually notice that I'm missing a case. IMO, that's a blocker.
A few potential solutions come to mind:
- I assume something like
#[values(SUPPORTED_SCHEMAS)]doesn't work, but if so, that's easy enough (docs). - We could write a macro to generate the necessary
#[case]attributes -- based on this StackOverflow question it looks like generating attributes from a macro is possible. - If the above aren't feasible/easy, another approach would be to avoid using
rstestat all. We could write a "wrapper function" likerun_with_ephemeral_datastorewhich would take aFn(EphemeralDatastore), then call that function in a loop with an ephemeral datastore at the proper version. Every place that currently uses the template would be changed to call this function instead, wrapping the current test body. This would work, but wouldn't separate different versions of the same test into separate cases or provide parallelization of tests across versions, and less importantly would induce an additional level of indentation in most test bodies.
I would say writing a quick macro is probably the best approach, assuming there's no syntax that would allow SUPPORTED_VERSIONS to be used directly.
There was a problem hiding this comment.
That inspires one way we could do this with a declarative macro: we could define and then use a macro that takes multiple schema versions as arguments, and then emits two top-level items: the const declaration of SUPPORTED_SCHEMA_VERSIONS, and the entire test template function, decorated with one #[case] attribute macro per macro argument.
There was a problem hiding this comment.
That's a great idea. I'll give the macro a try this afternoon.
There was a problem hiding this comment.
The macro was pretty easy to write (I did in the latest pushed commit). The downside is that because you can't construct identifiers with macro variables, we can't use rstest's case::label syntax to interpolate the schema version into test names.
|
Sample test output: Note we get distinct test cases per schema version: I like this because I believe it means |
| // We must import `rstest_reuse` at the top of the crate | ||
| // https://docs.rs/rstest_reuse/0.5.0/rstest_reuse/#use-rstest_reuse-at-the-top-of-your-crate | ||
| #[cfg(test)] | ||
| use rstest_reuse; |
There was a problem hiding this comment.
This is unusual, but we can live with it. They must be using ::crate::rstest_reuse in the macro expansion, expecting to hit this private imported name.
aggregator_core/src/datastore.rs
Outdated
| @@ -5439,6 +5441,8 @@ mod tests { | |||
| vdaf::prio3::{Prio3, Prio3Count}, | |||
| }; | |||
| use rand::{distributions::Standard, random, thread_rng, Rng}; | |||
| use rstest::rstest; | |||
| //use rstest_reuse::{self, template}; | |||
There was a problem hiding this comment.
This can be deleted.
aggregator_core/src/datastore.rs
Outdated
| #[rstest] | ||
| // The version numbers in these cases must match SUPPORTED_SCHEMA_VERSIONS | ||
| #[case::version_20230405185602(ephemeral_datastore_max_schema_version(20230405185602))] | ||
| #[case::version_20230417204528(ephemeral_datastore_max_schema_version(20230417204528))] |
There was a problem hiding this comment.
As declarative macros cannot produce attributes, we'd have to write our own procedural macro in order to do that. We can either do so as a progressive enhancement later, or just make coupled changes to the template and SUPPORTED_SCHEMA_VERSIONS by hand.
aggregator_core/src/datastore.rs
Outdated
| @@ -5439,6 +5441,8 @@ mod tests { | |||
| vdaf::prio3::{Prio3, Prio3Count}, | |||
| }; | |||
| use rand::{distributions::Standard, random, thread_rng, Rng}; | |||
| use rstest::rstest; | |||
| //use rstest_reuse::{self, template}; | |||
aggregator_core/src/datastore.rs
Outdated
| #[rstest] | ||
| // The version numbers in these cases must match SUPPORTED_SCHEMA_VERSIONS | ||
| #[case::version_20230405185602(ephemeral_datastore_max_schema_version(20230405185602))] | ||
| #[case::version_20230417204528(ephemeral_datastore_max_schema_version(20230417204528))] |
There was a problem hiding this comment.
I think the main value of rstest is that we run each datastore test on each supported schema version. However, I don't like that we have to remember to manually add a case each time we add a new schema version -- we should have one canonical list of supported schemas, and that is SUPPORTED_SCHEMAS. Currently, if I add a version to SUPPORTED_SCHEMAS but forget to add a case here, the tests would pass even if the new schema versions was broken; I (or a reviewer) have to manually notice that I'm missing a case. IMO, that's a blocker.
A few potential solutions come to mind:
- I assume something like
#[values(SUPPORTED_SCHEMAS)]doesn't work, but if so, that's easy enough (docs). - We could write a macro to generate the necessary
#[case]attributes -- based on this StackOverflow question it looks like generating attributes from a macro is possible. - If the above aren't feasible/easy, another approach would be to avoid using
rstestat all. We could write a "wrapper function" likerun_with_ephemeral_datastorewhich would take aFn(EphemeralDatastore), then call that function in a loop with an ephemeral datastore at the proper version. Every place that currently uses the template would be changed to call this function instead, wrapping the current test body. This would work, but wouldn't separate different versions of the same test into separate cases or provide parallelization of tests across versions, and less importantly would induce an additional level of indentation in most test bodies.
I would say writing a quick macro is probably the best approach, assuming there's no syntax that would allow SUPPORTED_VERSIONS to be used directly.
tgeoghegan
force-pushed
the
timg/startup-check-schema-version
branch
from
47264b0
to
9c1457a
Compare
docs/DEPLOYING.md
Outdated
| @@ -114,6 +114,12 @@ This will generate two new migration scripts. Fill the `*.up.sql` file with the | |||
| migration you want to run and the `*.down.sql` file with a script that reverses | |||
| the first script. | |||
|
|
|||
| After adding a migration, you must add its version number to | |||
| `datastore::SUPPORTED_SCHEMA_VERSIONS` as Janus will refuse to run if it does | |||
| not recognize the database schema version. You also must add a `#[case]` | |||
There was a problem hiding this comment.
nit: the docs suggesting that a #[case] attribute needs to be created can now be dropped.
We want to prove in tests that some Janus version can run safely on multiple schema versions to make database schema migrations safer. In this commit: - `aggregator_core::test_util::EphemeralDatastore` can now be constructed with a `max_schema_version` argument to control which migration scripts are applied during tests - `datastore::SUPPORTED_SCHEMA_VERSIONS` is now emitted by the `supported_schema_versions` macro, which also constructs a template for stamping out tests. - We adopt [`rstest`][1] to inject an `EphemeralDatastore` instance into tests in `aggregator_core::datastore::Datatore::tests`. Using [`rstest_reuse`][2], we can automatically stamp out versions of existing tests that run using multiple schema versions. We only use this dependency injection technique in the `datastore` module, because that should be the only module that's tightly coupled to the database schema. We could run all tests that use a datastore this way, at the cost of increasing overall test runtime. [1]: https://docs.rs/rstest [2]: https://docs.rs/rstest_reuse
We want to prove in tests that some Janus version can run safely on multiple schema versions to make database schema migrations safer. In this commit:
aggregator_core::test_util::EphemeralDatastorecan now be constructed with amax_schema_versionargument to control which migration scripts are applied during testsrstestto inject anEphemeralDatastoreinstance into tests inaggregator_core::datastore::Datatore::tests. Usingrstest_reuse, we can automatically stamp out versions of existing tests that run using multiple schema versions.We only use this dependency injection technique in the
datastoremodule, because that should be the only module that's tightly coupled to the database schema. We could run all tests that use a datastore this way, at the cost of increasing overall test runtime.