Skip to content

Commit

Permalink
Fix running cargo test without nextest
Browse files Browse the repository at this point in the history
Reproduction: `cargo test -p apollo-federation -- -q`

## Background

#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<Mutex<HashSet<&'static str>>>`
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.

#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](rust-lang/rust#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
  • Loading branch information
SimonSapin committed May 29, 2024
1 parent 09bef5a commit c45f7bc
Show file tree
Hide file tree
Showing 3 changed files with 101 additions and 40 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand All @@ -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 {
Expand All @@ -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") {
{
Expand All @@ -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") {
{
Expand All @@ -388,9 +389,8 @@ fn respects_query_planner_option_reuse_query_fragments(reuse_query_fragments: bo
}
},
}
"#
);
}
"#
);
}

#[test]
Expand Down Expand Up @@ -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() {
Expand Down
Original file line number Diff line number Diff line change
@@ -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)
Expand Down
Original file line number Diff line number Diff line change
@@ -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
}

0 comments on commit c45f7bc

Please sign in to comment.