Repository navigation
perf: 51 test binaries → 4, nextest gate, and a cache key that survives a version bump - #92
Merged
Merged
Conversation
Every file under `tests/` was its own binary, so the workspace linked 51 of them and 19 of those re-linked the whole GPUI stack into a ~230 MB executable. `crates/*/tests/it/main.rs` now declares the files as modules: four binaries, and `tests/it/common/` compiles once instead of once per file. Measured on the maintainer's box, editing one line of `crates/app/src/lib.rs` and rebuilding every test target: 24 s before, 5.8 s after. The gate itself barely moves locally (linking was never the local floor), but this is what macOS and Windows pay for every CI run, and it drops ~4.5 GB of test binaries out of the caches those jobs save and restore. Two mechanical consequences: `dependency_rule.rs` reads its manifests with `include_str!`, which now needs one more `..`, and the shared helpers are imported with `use super::common;` instead of `mod common;` in each file.
`a_spent_bucket_reports_the_time_the_budget_needs` called `set_idle`, which read `Instant::now()`, and then `wait_for`, which read it again. A thread descheduled in between refilled the bucket by the gap and the absolute bounds failed: seen as "an empty bucket at 1 kB/s needs 100 ms: 20.026619ms" under nextest, whose higher parallelism widens that window. The clock is now read once and threaded through `set_idle_at`, `debit_at` and `wait_for_at`, so the assertion cannot race the refill. The public `set_idle`/`debit`/`wait_for` keep reading the wall clock themselves.
`serde` was listed but never used: the double builds every body with `serde_json::Value` and axum's `Json` and derives nothing. `hex`, `sha1` and `tokio` were also in `[dev-dependencies]` with the same or a subset of the features already required by the normal dependency, which a dev target sees anyway. `cargo machete` is now clean for the workspace. No build-time effect: every one of these crates is pulled in by another dependency regardless. It is one fewer edge in Cargo.lock and four fewer claims to keep true. (Also audited the 86 crates that resolve to more than one version; every one belongs to the pinned gpui stack, a platform binding or a build-time host, none to us.)
…bumps Three separate costs in the CI run: `cargo test` runs one test binary at a time, and the macOS job is the critical path: 8m03 of test step for 911 tests, 6m of it serial execution. nextest runs the binaries concurrently (`taiki-e/install-action` pinned by SHA, nextest 0.9.150). Locally the same suite goes from 2m51 to 60 s, floored by one real-time bandwidth test that can now overlap everything else. macOS's `--no-fail-fast` goes away with it: nextest runs everything regardless. The rust-cache prefix hashed `Cargo.toml` and `crates/*/Cargo.toml`, so a release-please version bump was a guaranteed miss on all three OSes -- the 0.3.0 release run compiled 850 crates cold on each (Windows clippy 4m24 against 57 s warm). Only `Cargo.lock` and `rust-toolchain.toml` now, which is what a dependency or compiler change moves; a cache missing an artifact cargo now wants just compiles that delta. The prefix is bumped to v1, so the first run after this is cold by design. `CARGO_PROFILE_TEST_DEBUG=0`: a test binary is the entire GPUI stack, and the panic location and `assert_eq!` output carry what a CI failure needs. The dev profile keeps line-tables-only for the workspace crates. Also fixes two shellcheck findings in the Linux smoke job's script (it used `ls` to pick a file) and records the nextest gate in CONTRIBUTING.
The four real-timing tests in `limits.rs` spent 60.2 s, 37.9 s, 30.1 s and 44.9 s of wall clock proving four properties about a 30 s window. None of those seconds bought knowledge: the window is a constant, and the tests were re-measuring it once per property on a real clock. `ImmichClient` now carries the window it enforces, `REQUEST_WINDOW` (30 s) by default, with `with_window` for a test to shrink it. The guard, the request deadline and the upload path read that field instead of the old `request_timeout()` function and its `TEST_TIMEOUT_MILLIS` static -- which was a process-global that two tests running at once could overwrite. The tests keep their real sockets, real partial-body byte counts and real double; only the window shrinks to 200 ms, and the assertions get *tighter* because the arithmetic is now visible: a body blocked mid-flight gives up between one and two windows, a server that accepted the body and went quiet gives up one window after the last byte, and a paced upload outlives many windows without being cut. The one thing a shrunk window must not lose is the value production gets, so `the_production_window_is_the_specified_thirty_seconds` asserts 30 s and that `new()` installs it. The whole crate goes 60.3 s -> 8.35 s, and the workspace gate 60.3 s -> 50.3 s. The remaining time is no longer a clock: 912 tests total ~700 s of test time on 12 cores, so the suite is throughput-bound, and the top of the list is real work (230 files hashed, uploaded and fsynced; four `folder_detail` UI tests driving `cx.wait_for`). `a_rate_too_low_to_pay_out_one_read` at 8.2 s is the R7.1 floor's own arithmetic (4 KiB at 1 kB/s) and is irreducible. Tried and rejected: `tokio::time::pause()`. The timing core is all tokio clock already, so it turned the 60 s test into 0.156 s with no production change -- but auto-advance cannot tell a runtime parked on a socket from one parked on a timer, so the next real round-trip (the login handshake) raced its own deadline and failed as `TimedOut`. A virtual clock and a real socket cannot share a test.
It built the binary, launched it under Xvfb with lavapipe, captured the root window and failed on a single-colour histogram. That was the only automated launch of the real app on any OS, and it was never on the critical path: macOS owns that, so removing it saves runner minutes, not wall clock. What it bought was narrower than it looked. Startup, window creation, Vulkan init and a non-blank frame are covered in pieces by the headless GPUI tests, which assert per-surface snapshots without a GPU or a display; the smoke's unique contributions were "the real binary accepts a software Vulkan device" and "a second launch hands off and exits 0", and both were verified by hand before this and will be again by the capture recipe when a ticket touches startup, rendering, the tray or the handoff. Against that, it kept a second distro package list (`xvfb`, `mesa-vulkan-drivers`, `imagemagick`, `dbus`, `xauth`), a 60-line inner shell script, and a cache key that had to stay restore-only so a bin-only save could not starve the Linux test job of dev-dependencies. CONTRIBUTING now says no CI job launches the binary and that the capture recipe is the manual check for anything touching those surfaces.
This file contains hidden or 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
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.
Summary
Five changes, each measured before it was made:
cargo test→cargo nextestimmich-client60.3 s → 8.4 sCARGO_PROFILE_TEST_DEBUG=0Evidence
CI, warm cache, run 41266533223: Linux 3 m 49 (test 2 m 52), Windows 7 m 37 (test 5 m 45), macOS 9 m 31 (test 8 m 03, the critical path).
CI, cold, run 41269865243: "No cache found" on Linux and Windows — a version bump in every manifest changed the cache prefix. Windows clippy 4 m 24 against 57 s warm.
CI on this branch: run 42179209035 (four jobs, cold by design, prefix
v0→v1) and run 42335270218 (five commits) both green on every job.Two bugs found on the way, each in its own commit:
bandwidth::tests::a_spent_bucket_reports_the_time_the_budget_needsreadInstant::now()twice, so a thread descheduled for 80 ms refilled the bucket:an empty bucket at 1 kB/s needs 100 ms: 20.026619ms. The clock is now read once.limits.rstests spent 60.2 + 37.9 + 30.1 + 44.9 s proving four properties about a 30 s window.ImmichClientnow carries its window (REQUEST_WINDOW, 30 s) withwith_windowfor tests, replacing a#[cfg(test)]static that two parallel tests could overwrite. Real sockets, real partial-body byte counts, a 200 ms window, and tighter assertions because the arithmetic is visible. The 30 s production value is asserted once, bythe_production_window_is_the_specified_thirty_seconds.Tried and rejected:
tokio::time::pause(). It turned the 60 s test into 0.156 s with no production change, but auto-advance cannot tell a runtime parked on a socket from one parked on a timer, so the next real round-trip raced its own deadline and failed asTimedOuton the login handshake.cargo macheteis clean; the deadserdeand three redundant dev-dependencies inmock-immichare gone. The 86 crates that resolve to two versions all belong to the pinned gpui stack, a platform binding, or a build-time host — the dependency audit is in that commit message.Merge Danger
Door: two-way — revert restores the old layout; the caches need one cold run either way.
Blast Radius: test layout, test timing and CI; one new public method (
ImmichClient::with_window) and no change to any production timing behaviour.--test <file>filters are nowcargo nextest run -p <crate> -E 'test(<module>)'.CONTRIBUTING.mdand the source doc comments are updated; any agent prompt or script still using the old form silently selects nothing.v0→v1).CARGO_PROFILE_TEST_DEBUG=0drops line numbers from test backtraces (panic locations survive). One line inci.ymlto revert if a CI failure needs them.with_windowis public API. The alternative is atest-supportfeature or moving the tests in-crate; both were considered and are worse than one documented knob.CONTRIBUTING.md, run by hand when a ticket touches startup, rendering, the tray or the handoff. If a real-GPU regression ever slips through, this is the job to bring back.engine_model::a_click_on_a_running_engine_starts_a_scanis a wall-clock budget test (64 MiB hashed to outlive a 250 ms status poll) with no deterministic barrier. nextest's parallelism widens the starvation window: it failed once at 25.4 s on a loaded 12-core box, passed alone in 1.5 s, and passed on the next full run and on CI.CONTRIBUTING.mdalready lists it. The fix is a barrier onEngineModel's surface, not a wider bound.implement-orchestrator-rules.mdis untracked in the operator's tree; the rule changes it needs (touchnarrowed to the uplift path,molddropped) are applied there but not part of this PR.