From c45f7bc88d0faa8fa83ab343c3fe55af91f0c88e Mon Sep 17 00:00:00 2001 From: Simon Sapin Date: Wed, 29 May 2024 08:11:01 +0200 Subject: [PATCH] Fix running `cargo test` without nextest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Reproduction: `cargo test -p apollo-federation -- -q` ## Background https://github.com/apollographql/router/pull/5157 introduced a new kind of snapshot test for the Rust query planner. Test schemas are specified as sets of subgraph schemas, so composing them is needed. Since we don’t have composition in Rust yet, the tests rely on JS composition through Rover. To avoid a dependency on Rover on CI and for most contributors, the composed supergraph are cached in the repository. Rover use is opt-in and only required when a cached file is missing or out of date. (Composition inputs are hashed.) The file name is derived (through a macro) from the test function name. To detect name conflicts, a static `OnceLock>>` is used to ensure no name is used more than once. ## The problem The static hash set relies on all snapshot tests running in the same process. This is the case with `cargo test`, but `cargo nextest run` as used on CI isolates every test in its own process. This breaks conflict detection of cache file names for composed supergraph schemas, since each test only "sees" itself in the static hash set. https://github.com/apollographql/router/pull/5240 introduced a name conflict: composition is used in a function called twice with different arguments. Because nextest was used both locally and on CI, the conflict went undectected. As a result, running `cargo test` on dev fails because the conflict is detected. ## This PR This PR fixes this case of cache file name conflict, but conflict detection is still broken with nextest. As a result it’s possible that this kind of conflict could happen again and be merged undectected. ## Non-solutions tried * Nextest has a notion of [test groups](https://nexte.st/book/test-groups), but they don’t appear to let multiple tests run in the same process * Instead of relying on the runtime side effect of tests, could conflict detection rely on enumerating tests at compile time? The [`linkme` crate](https://crates.io/crates/linkme) is a building block Router used to register Rust plugins. It could be used here in all composition inputs can be made const, but `std::any::type_name` used in a macro to extract the current function name is not `const fn` [yet](https://github.com/rust-lang/rust/issues/63084) ## Potential solutions * Remove the cache and accept the dependency on Rover for testing. This impacts CI and all contributors. * Require every `planner!` macro invocation to specify an explicit cache file name instead of relying on the function name. Then conflict detection can use `linkme`. * Move conflict detection to a separate test that something something parses Rust source files of other tests something --- .../named_fragments_preservation.rs | 78 +++++++++---------- ...ption_reuse_query_fragments_false.graphql} | 2 +- ..._option_reuse_query_fragments_true.graphql | 61 +++++++++++++++ 3 files changed, 101 insertions(+), 40 deletions(-) rename apollo-federation/tests/query_plan/supergraphs/{respects_query_planner_option_reuse_query_fragments.graphql => respects_query_planner_option_reuse_query_fragments_false.graphql} (95%) create mode 100644 apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_true.graphql diff --git a/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs b/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs index 231609b845..f5f1a0cf52 100644 --- a/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs +++ b/apollo-federation/tests/query_plan/build_query_plan_tests/named_fragments_preservation.rs @@ -290,25 +290,7 @@ fn it_avoid_fragments_usable_only_once() { ); } -#[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure - -fn respects_query_planner_option_reuse_query_fragments_true() { - respects_query_planner_option_reuse_query_fragments(true) -} -#[test] -#[should_panic(expected = "not yet implemented")] -// TODO: investigate this failure - -fn respects_query_planner_option_reuse_query_fragments_false() { - respects_query_planner_option_reuse_query_fragments(false) -} - -fn respects_query_planner_option_reuse_query_fragments(reuse_query_fragments: bool) { - let planner = planner!( - config = QueryPlannerConfig {reuse_query_fragments, ..Default::default()}, - Subgraph1: r#" +const SUBGRAPH: &str = r#" type Query { t: T } @@ -322,9 +304,9 @@ fn respects_query_planner_option_reuse_query_fragments(reuse_query_fragments: bo x: Int y: Int } - "#, - ); - let query = r#" +"#; + +const QUERY: &str = r#" query { t { a1 { @@ -340,12 +322,21 @@ fn respects_query_planner_option_reuse_query_fragments(reuse_query_fragments: bo x y } - "#; - if reuse_query_fragments { - assert_plan!( - &planner, - query, - @r#" +"#; + +#[test] +#[should_panic(expected = "not yet implemented")] +// TODO: investigate this failure + +fn respects_query_planner_option_reuse_query_fragments_true() { + let planner = planner!( + config = QueryPlannerConfig { reuse_query_fragments: true, ..Default::default()}, + Subgraph1: SUBGRAPH, + ); + assert_plan!( + &planner, + QUERY, + @r#" QueryPlan { Fetch(service: "Subgraph1") { { @@ -365,13 +356,23 @@ fn respects_query_planner_option_reuse_query_fragments(reuse_query_fragments: bo } }, } - "# - ); - } else { - assert_plan!( - &planner, - query, - @r#" + "# + ); +} + +#[test] +#[should_panic(expected = "not yet implemented")] +// TODO: investigate this failure + +fn respects_query_planner_option_reuse_query_fragments_false() { + let planner = planner!( + config = QueryPlannerConfig { reuse_query_fragments: false, ..Default::default()}, + Subgraph1: SUBGRAPH, + ); + assert_plan!( + &planner, + QUERY, + @r#" QueryPlan { Fetch(service: "Subgraph1") { { @@ -388,9 +389,8 @@ fn respects_query_planner_option_reuse_query_fragments(reuse_query_fragments: bo } }, } - "# - ); - } + "# + ); } #[test] @@ -464,7 +464,7 @@ fn it_works_with_nested_fragments_when_only_the_nested_fragment_gets_preserved() #[test] #[should_panic( - expected = "Error: variable `$if` of type `Boolean` cannot be used for argument `if` of type `Boolean!`" + expected = "variable `$if` of type `Boolean` cannot be used for argument `if` of type `Boolean!`" )] // TODO: investigate this failure fn it_preserves_directives_when_fragment_not_used() { diff --git a/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments.graphql b/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_false.graphql similarity index 95% rename from apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments.graphql rename to apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_false.graphql index 8831cd4888..11c5d8c279 100644 --- a/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments.graphql +++ b/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_false.graphql @@ -1,4 +1,4 @@ -# Composed from subgraphs with hash: 4b0a2b41b9cbccb8bde234dc0285c3372e220fa1 +# Composed from subgraphs with hash: ca8bc5b745ab72ecb5d72159ed30d089d2084d77 schema @link(url: "https://specs.apollo.dev/link/v1.0") @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) diff --git a/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_true.graphql b/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_true.graphql new file mode 100644 index 0000000000..11c5d8c279 --- /dev/null +++ b/apollo-federation/tests/query_plan/supergraphs/respects_query_planner_option_reuse_query_fragments_true.graphql @@ -0,0 +1,61 @@ +# Composed from subgraphs with hash: ca8bc5b745ab72ecb5d72159ed30d089d2084d77 +schema + @link(url: "https://specs.apollo.dev/link/v1.0") + @link(url: "https://specs.apollo.dev/join/v0.3", for: EXECUTION) +{ + query: Query +} + +directive @join__enumValue(graph: join__Graph!) repeatable on ENUM_VALUE + +directive @join__field(graph: join__Graph, requires: join__FieldSet, provides: join__FieldSet, type: String, external: Boolean, override: String, usedOverridden: Boolean) repeatable on FIELD_DEFINITION | INPUT_FIELD_DEFINITION + +directive @join__graph(name: String!, url: String!) on ENUM_VALUE + +directive @join__implements(graph: join__Graph!, interface: String!) repeatable on OBJECT | INTERFACE + +directive @join__type(graph: join__Graph!, key: join__FieldSet, extension: Boolean! = false, resolvable: Boolean! = true, isInterfaceObject: Boolean! = false) repeatable on OBJECT | INTERFACE | UNION | ENUM | INPUT_OBJECT | SCALAR + +directive @join__unionMember(graph: join__Graph!, member: String!) repeatable on UNION + +directive @link(url: String, as: String, for: link__Purpose, import: [link__Import]) repeatable on SCHEMA + +type A + @join__type(graph: SUBGRAPH1) +{ + x: Int + y: Int +} + +scalar join__FieldSet + +enum join__Graph { + SUBGRAPH1 @join__graph(name: "Subgraph1", url: "none") +} + +scalar link__Import + +enum link__Purpose { + """ + `SECURITY` features provide metadata necessary to securely resolve fields. + """ + SECURITY + + """ + `EXECUTION` features provide metadata necessary for operation execution. + """ + EXECUTION +} + +type Query + @join__type(graph: SUBGRAPH1) +{ + t: T +} + +type T + @join__type(graph: SUBGRAPH1) +{ + a1: A + a2: A +}