Fix running cargo test without nextest

[SimonSapin]

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, 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 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

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

[github-actions]

@SimonSapin, please consider creating a changeset entry in /.changesets/. These instructions describe the process and tooling.

[router-perf]

CI performance tests

• events_big_cap_high_rate_callback - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity using callback mode
• reload - Reload test over a long period of time at a constant rate of users
• large-request - Stress test with a 1 MB request payload
• events - Stress test for events with a lot of users and deduplication ENABLED
• const - Basic stress test that runs with a constant number of users
• step - Basic stress test that steps up the number of users over time
• events_without_dedup_callback - Stress test for events with a lot of users and deduplication DISABLED using callback mode
• demand-control-instrumented - A copy of the step test, but with demand control monitoring and metrics enabled
• events_without_dedup - Stress test for events with a lot of users and deduplication DISABLED
• events_big_cap_high_rate - Stress test for events with a lot of users, deduplication enabled and high rate event with a big queue capacity
• no-graphos - Basic stress test, no GraphOS.
• events_callback - Stress test for events with a lot of users and deduplication ENABLED in callback mode
• xlarge-request - Stress test with 10 MB request payload
• xxlarge-request - Stress test with 100 MB request payload
• step-jemalloc-tuning - Clone of the basic stress test for jemalloc tuning
• step-with-prometheus - A copy of the step test with the Prometheus metrics exporter enabled
• demand-control-uninstrumented - A copy of the step test, but with demand control monitoring enabled

[lrlna]

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.

I potentially don't mind this. What are the potential downsides to this, except for having to manually write out file names?

[SimonSapin]

Right, that might not be so bad. It’s just yet an extra step when porting or adding new tests.