Assorted bug fixes in new QP, found by a newly-ported test #5157
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SimonSapin
requested review from
dariuszkuc,
sachindshinde and
goto-bus-stop
as code owners
|
@SimonSapin, please consider creating a changeset entry in |
|
CI performance tests
|
duckki
approved these changes
May 14, 2024
SimonSapin
added a commit
that referenced
this pull request
May 29, 2024
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
SimonSapin
added a commit
that referenced
this pull request
May 29, 2024
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
lrlna
pushed a commit
that referenced
this pull request
Jun 3, 2024
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
Geal
pushed a commit
that referenced
this pull request
Jun 10, 2024
Reproduction: `cargo test -p apollo-federation -- -q` #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 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 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. * 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) * 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
theJC
pushed a commit
to theJC/router
that referenced
this pull request
Jun 10, 2024
Reproduction: `cargo test -p apollo-federation -- -q` ## Background apollographql#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. apollographql#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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Part of FED-221.
Checklist
Complete the checklist (and note appropriate exceptions) before the PR is marked ready-for-review.
Exceptions
Note any exceptions here
Notes
Footnotes
It may be appropriate to bring upcoming changes to the attention of other (impacted) groups. Please endeavour to do this before seeking PR approval. The mechanism for doing this will vary considerably, so use your judgement as to how and when to do this. ↩
Configuration is an important part of many changes. Where applicable please try to document configuration examples. ↩
Tick whichever testing boxes are applicable. If you are adding Manual Tests, please document the manual testing (extensively) in the Exceptions. ↩