Use scoped feature-flag overrides in view_tests instead of global set_enabled#14039
Use scoped feature-flag overrides in view_tests instead of global set_enabled#14039fbartho wants to merge 1 commit into
Conversation
view_tests.rs contained 22 call sites that flipped process-global
FeatureFlag state via FeatureFlag::{AgentView,CloudMode}.set_enabled(true)
without any teardown. Under plain `cargo test`, all tests in a target
share one process, so the leaked state bled into later tests that assume
these flags default to false, making them fail whenever they ran after
one of the 22 offenders.
Convert every raw set_enabled(true) call in view_tests.rs to the
thread-local override_enabled(true) guard the module already uses
elsewhere (e.g. updated_conversation_metadata_refreshes_selected_conversation_pane_title,
jump_to_latest_agent_message_no_ops_when_agent_view_disabled). The guard
reverts on drop, so state no longer survives past the end of each test.
Closes warpdotdev#13972
|
Every PR must be linked to a same-repo issue before Oz can review it. This PR is linked to #13972, but no linked issue is marked See the contribution guidelines for the full readiness model. Powered by Oz |
There was a problem hiding this comment.
Every PR must be linked to a same-repo issue before Oz can review it.
This PR is linked to #13972, but no linked issue is marked ready-to-implement yet. Only repository maintainers apply that label, so please wait for a maintainer to mark the issue. Once it is marked, push a new commit or comment /oz-review to re-trigger review.
See the contribution guidelines for the full readiness model.
Powered by Oz
Closes #13972
Description
view_tests.rshad 22 call sites flipping the process-globalFeatureFlag::AgentView/FeatureFlag::CloudModestate viaset_enabled(true)with no reset. Under plaincargo test, every test in thewarplib target shares one process, so the flag state leaked into whichever tests ran next — five tests that assumeAgentViewdefaults tofalsefailed whenever they ran after an offender.This converts all 22 sites to
override_enabled(true), the thread-local RAII guard the same module already uses elsewhere; the override reverts on drop at the end of each test. Thread-locality is sufficient:App::test's executor is a single-threadedLocalExecutor, and none of the converted tests spawn OS threads (so noget_overrides()/set_overrides()propagation is needed). Two tests set both flags and bind both guards for the full test body.override_enableditself is unchanged.Why CI didn't catch this
CI runs
cargo nextest, which isolates each test in its own process — the leak needs a shared-process runner to manifest.The
set_enabledpanic guard that should have flagged the misuse (warp_features/src/lib.rs:if cfg!(test) && cfg!(not(feature = "integration_tests")) { panic!(…) }) is dead code by construction:cfg!(test)resolves per-crate at the point it's written, andwarp_featuresis a normal[dependencies]library that's never compiled as a test binary — so the check is unconditionallyfalsefor every caller.Ruled out along the way: feature unification
The issue originally theorized that
crates/integrationenablingintegration_testsdefeats the guard via Cargo feature unification. Verified false:warp_core/Cargo.toml'sintegration_testsfeature is[]and never forwards towarp_features/integration_tests, andappdoesn't depend onwarp_featuresdirectly — so that feature can't be reached from a normalapp/warpbuild at all. The guard's deadness is independent of feature flags.Reproduction
Deterministic single-process repro — a flag-setting test followed by affected tests:
Before:
FAILED. 1 passed; 3 failed(assertions likeleft: "One" / right: "None", scroll-position and viewport-iter panics — matching the issue's reported failures). After: all originally-affected tests plus the flag-setter pass together,ok. 6 passed; 0 failed.cargo nextest run -p warp -E 'test(view_tests) or test(terminal::view)': 307 passed before and after (process isolation always masked this).Preventing recurrence
I looked at catching this class mechanically going forward. Verdicts:
disallowed-methodsonset_enabled(repo already has.clippy.toml+-D warningsCI)crates/integrationtests that want real global state) would each need#[allow(clippy::disallowed_methods)]— and every allow looks identical to a reintroduced leak, burying the signalcfg!(test)panic guardoverride_enabledfor the global case) — design work, and it inherits the same production/integration scoping problem./script/presubmitdisallowed-methods; only worth it if the lint is rejected specifically on annotation-noise groundsTesting
cargo test -p warp --libtargeted repro (before/after above)cargo nextest run -p warp -E 'test(view_tests) or test(terminal::view)'— 307 passed./script/format --check— cleancargo clippy -p warp --lib --tests— zero warnings./script/runAgent Mode