diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 84c6bdf..b3ce5cd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -23,9 +23,12 @@ env: CARGO_INCREMENTAL: 0 # Keep file/line panic locations while cutting codegen, link time (Windows # especially) and cache size. Dependency debug info is already off in - # Cargo.toml; this trims the workspace crates and test binaries too. + # Cargo.toml; this trims the workspace crates too. The test profile keeps no + # debug info at all: a test binary is the whole GPUI stack, and a panic + # message and its `assert_eq!` output carry the location a CI failure needs, + # while a backtrace's line numbers rarely decide the diagnosis. CARGO_PROFILE_DEV_DEBUG: line-tables-only - CARGO_PROFILE_TEST_DEBUG: line-tables-only + CARGO_PROFILE_TEST_DEBUG: "0" # A newer push to the same ref replaces an in-flight run: before this, # superseded merges kept burning 10-25 minutes each. @@ -53,7 +56,13 @@ jobs: # environment hash varies across runner images. Put the lockfile # hash in the prefix: it rotates when dependencies change and stays # stable across runners. - prefix-key: v0-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml', 'Cargo.toml', 'crates/*/Cargo.toml') }} + # + # Only Cargo.lock and the toolchain: hashing the manifests too made + # every release-please version bump a cache miss, and the release run + # compiled all 850 crates cold on every OS (observed: Windows clippy + # 4m24 against 57s warm). A cache missing a newly added artifact is + # not a correctness problem -- cargo compiles that delta. + prefix-key: v1-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml') }} add-rust-environment-hash-key: "false" cache-on-failure: "true" - name: Install system dependencies @@ -64,9 +73,16 @@ jobs: libfontconfig-dev libfreetype-dev libwayland-dev libxkbcommon-dev \ libxkbcommon-x11-dev libx11-xcb-dev libxcb1-dev libvulkan1 \ libssl-dev libzstd-dev libdbus-1-dev + # Pinned by commit SHA, version tag in the comment, like the actions + # above. nextest runs test binaries concurrently instead of one at a + # time, which is where this suite's wall clock was going: `cargo test` + # took 2m51 here, 60s of it inside one real-time bandwidth binary. + - uses: taiki-e/install-action@5488ac5a951645ce95062214902ac4b6bc1a1037 # v2.87.32 + with: + tool: cargo-nextest@0.9.150 - run: cargo fmt --all -- --check - run: cargo clippy --locked --workspace --all-targets -- -D warnings - - run: cargo test --locked --workspace + - run: cargo nextest run --locked --workspace windows: name: Windows @@ -84,13 +100,16 @@ jobs: # `key:` is ignored with `shared-key:`. lld-link was tried here and # reverted: the linker is a global fingerprint input, so switching it # rebuilt all 526 crates every run and never warmed the cache. - prefix-key: v0-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml', 'Cargo.toml', 'crates/*/Cargo.toml') }} + prefix-key: v1-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml') }} add-rust-environment-hash-key: "false" cache-on-failure: "true" # fmt is platform-independent and already checked on Linux; running it # here only paid for a toolchain component and a fresh checkout. + - uses: taiki-e/install-action@5488ac5a951645ce95062214902ac4b6bc1a1037 # v2.87.32 + with: + tool: cargo-nextest@0.9.150 - run: cargo clippy --locked --workspace --all-targets -- -D warnings - - run: cargo test --locked --workspace + - run: cargo nextest run --locked --workspace # The only place the macOS platform arms are compiled and exercised: the # maintainer's Linux host cannot build them (aws-lc-sys needs a macOS C @@ -118,14 +137,17 @@ jobs: - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 with: shared-key: macos - prefix-key: v0-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml', 'Cargo.toml', 'crates/*/Cargo.toml') }} + prefix-key: v1-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml') }} add-rust-environment-hash-key: "false" cache-on-failure: "true" + - uses: taiki-e/install-action@5488ac5a951645ce95062214902ac4b6bc1a1037 # v2.87.32 + with: + tool: cargo-nextest@0.9.150 - run: cargo clippy --locked --workspace --all-targets -- -D warnings - # --no-fail-fast: one macOS-only failure must not hide the next test - # binary's. This job is the only place those tests run and each cold run - # costs minutes. - - run: cargo test --locked --workspace --no-fail-fast + # nextest runs every test whatever an earlier one did, which replaces the + # --no-fail-fast this job needed: one macOS-only failure must not hide + # the next one, and this job is the only place those tests run. + - run: cargo nextest run --locked --workspace windows-gui-subsystem: name: Windows GUI subsystem (release PE) @@ -144,7 +166,7 @@ jobs: # the other. Separate keys also keep the release profile out of the # debug cache the critical-path job restores. shared-key: windows-release - prefix-key: v0-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml', 'Cargo.toml', 'crates/*/Cargo.toml') }} + prefix-key: v1-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml') }} add-rust-environment-hash-key: "false" cache-on-failure: "true" # #28: the app is a tray process, so the release binary must be a GUI @@ -214,127 +236,3 @@ jobs: throw "$exe has no icon resource group 1; GPUI's window class icon and the shell icon would be blank" } "icon resource group 1 present" - - linux-gui-smoke: - name: Linux GUI smoke (Xvfb + lavapipe) - runs-on: ubuntu-24.04 - steps: - - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7.0.1 - - name: Install Rust 1.99.0 - run: | - rustup toolchain install 1.99.0 --profile minimal --component rustfmt --component clippy - rustup default 1.99.0 - - uses: Swatinem/rust-cache@6323deb102c322ba6fcbdcafc7e3dddab59af2b6 # v2.9.2 - with: - # Shared with the Linux job: rust-cache persists dependencies only, so - # this job restores the same dependency artifacts instead of - # compiling them a third time. It never saves: the Linux job owns the - # key and its cache covers the test targets too, while a bin-only save - # here finished first and starved the Linux job of dev-dependencies. - # The prefix must match the Linux job's exactly to share the cache. - shared-key: linux - prefix-key: v0-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml', 'Cargo.toml', 'crates/*/Cargo.toml') }} - add-rust-environment-hash-key: "false" - save-if: "false" - cache-on-failure: "true" - - name: Install system dependencies - run: | - sudo apt-get update - sudo apt-get install -y \ - gcc g++ clang pkg-config \ - libfontconfig-dev libfreetype-dev libwayland-dev libxkbcommon-dev \ - libxkbcommon-x11-dev libx11-xcb-dev libxcb1-dev \ - libvulkan1 mesa-vulkan-drivers libdbus-1-dev \ - xvfb xauth dbus-daemon imagemagick x11-utils - - run: cargo build --locked --bin immich-sync - - name: Capture a real frame - run: | - set -euo pipefail - # The ICD file name depends on the mesa version (lvp_icd.json vs lvp_icd.x86_64.json). - ICD=$(ls /usr/share/vulkan/icd.d/lvp_icd*.json 2>/dev/null | head -1) || ICD="" - if [ -z "$ICD" ]; then - echo "no lavapipe ICD file under /usr/share/vulkan/icd.d" >&2 - exit 1 - fi - echo "lavapipe ICD=$ICD" - export VK_ICD_FILENAMES="$ICD" - export VK_DRIVER_FILES="$ICD" - export LIBGL_ALWAYS_SOFTWARE=1 - unset WAYLAND_DISPLAY - dbus-run-session -- xvfb-run -a -s "-screen 0 1280x800x24" bash -c ' - set -euo pipefail - # Give this run its own log and state directory: a log left by an - # earlier run would satisfy the started poll below before this app - # ever starts, and a stale state database would hand off instead. - export XDG_DATA_HOME="$PWD/xdg-data" - target/debug/immich-sync >shot.png.log 2>&1 & - APP=$! - # The app logs here, not to stdout. - LOG_DIR="${XDG_DATA_HOME:-$HOME/.local/share}/immich-sync" - started() { grep -qs "immich-sync started" "$LOG_DIR"/immich-sync.log* 2>/dev/null; } - alive() { kill -0 $APP 2>/dev/null; } - # Wait for the client instead of sleeping a fixed 10 s: opening the - # store made startup slower, and a fixed budget captured a black - # root while the app was still booting. - START_TIMEOUT=60 - waited=$SECONDS - until started; do - alive || { echo "the app exited during startup:" >&2; cat shot.png.log >&2; exit 1; } - if [ $((SECONDS - waited)) -ge $START_TIMEOUT ]; then - echo "timeout: no immich-sync started line in $LOG_DIR after ${START_TIMEOUT}s" >&2 - cat shot.png.log >&2 - kill $APP 2>/dev/null || true - exit 1 - fi - sleep 1 - done - echo "app started after $((SECONDS - waited))s" - kill -0 $APP # the app survived startup - # That line is logged before the window opens, so wait for a drawn - # frame too: a bare root captures as one flat colour. - PAINT_TIMEOUT=60 - waited=$SECONDS - while :; do - alive || { echo "the app exited before it painted:" >&2; cat shot.png.log >&2; exit 1; } - # Bound the capture: a wedged import must not block the loop past - # the deadline below. - timeout 30 import -window root shot.png - colors=$(identify -format %k shot.png) - if [ "$colors" -gt 1 ]; then break; fi - if [ $((SECONDS - waited)) -ge $PAINT_TIMEOUT ]; then - echo "timeout: the frame is still one flat colour after ${PAINT_TIMEOUT}s (colors=$colors)" >&2 - cat shot.png.log >&2 - kill $APP 2>/dev/null || true - exit 1 - fi - sleep 1 - done - echo "frame painted after $((SECONDS - waited))s" - target/debug/immich-sync # second launch: non-primary, exits 0 under set -e - sleep 2 - kill -0 $APP # the handoff did not stop the primary - kill $APP 2>/dev/null || true - wait $APP 2>/dev/null || true - cat shot.png.log - echo "--- app log directory: $LOG_DIR" - ls -la "$LOG_DIR" 2>/dev/null || true - newest=$(ls -t "$LOG_DIR"/immich-sync.log* 2>/dev/null | head -1 || true) - if [ -n "$newest" ]; then - echo "--- app log: $newest" - cat "$newest" - else - echo "--- no app log file found" - fi - ' - - name: Assert the frame is not blank - run: | - set -euo pipefail - identify -format "geom=%wx%h colors=%k\n" shot.png - colors=$(identify -format %k shot.png) - mean=$(convert shot.png -colorspace gray -format '%[fx:mean]' info:) - echo "screenshot gray mean=$mean" - # The colour test is what rejects a flat frame whatever its - # brightness: a uniform grey frame is colors=1 with mean~0.5, so the - # mean test alone would pass it. Together they reject a bare root. - test "$colors" -gt 1 - test "$mean" != "0" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cdfdc6a..074a901 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -65,13 +65,23 @@ all three; the Windows and macOS jobs run clippy and the tests: ```sh cargo fmt --all -- --check cargo clippy --workspace --all-targets -- -D warnings -cargo test --workspace +cargo nextest run --workspace ``` -Plus a headless Linux smoke that launches the real binary under Xvfb with -lavapipe and proves the captured frame is not blank. UI changes should keep the -headless GPUI tests green; `cargo test -p immich-sync --test dashboard`, -`--test logs_ui`, and the other `tests/*_ui.rs` files cover the surfaces. +`cargo nextest` (`cargo install --locked cargo-nextest`; CI pins 0.9.150) runs +test *binaries* concurrently instead of one at a time. That was where the gate's +wall clock went: `cargo test --workspace` took 2m51 on the maintainer's box. +nextest takes it to about 47s. Each crate still has one test binary: +`crates/*/tests/it/main.rs` holds the old per-file tests as modules, so a filter +is `cargo nextest run -p immich-sync -E 'test(ui)'` rather than `--test logs_ui`. + +No CI job launches the binary. The Linux GUI smoke that used to run it under +Xvfb with lavapipe and assert the captured frame was not blank is gone: it cost +a job and a distro-package list to prove a frame that the headless GPUI tests +already cover in pieces. So a change that touches rendering, startup, the tray, +or the single-instance handoff should keep the headless GPUI tests green — the +`*_ui` modules under `crates/app/tests/it/` cover the surfaces — and be checked +against the capture recipe below on a real machine. `--all-features` is invalid here: the three `gpui-tray` backends are mutually exclusive. @@ -81,12 +91,35 @@ Some tests are wall-clock budgets rather than progress-bounded waits: tests in `engine.rs`, `a_server_that_stops_answering_produces_backoff_not_a_hot_loop`, the pacing test in `limits.rs`, and the port race in `connection.rs`. In -`crates/app/tests/engine_model.rs`, +`crates/app/tests/it/engine_model.rs`, `a_click_on_a_running_engine_starts_a_scan` is one too: it hashes a 64 MiB fixture so the scan outlives the 250 ms status poll that carries the scan flag, and `EngineModel`'s surface has no deterministic barrier. They can starve under heavy parallel load; run one alone before treating a failure as a regression. +Concurrency makes the ones that read the clock twice flaky, so a timing test +reads it once: `BandwidthLimit`'s unit tests pass one `Instant` to `set_idle_at`, +`debit_at` and `wait_for_at` instead of letting a descheduled thread refill the +bucket between two `Instant::now()` calls (nextest's parallelism is what exposed +that). Do the same for a new timing assertion rather than widening its bounds. + +A test that proves a *window* fires must not pay that window in wall clock. +Shrink it on the client instead of on the clock: +`ImmichClient::with_window(Duration::from_millis(200))` runs the real guard over +a real socket, and `limits.rs` then asserts the arithmetic it actually wants +(one window after the last byte moved, at most two for a body blocked +mid-flight). The production value is asserted once, by +`the_production_window_is_the_specified_thirty_seconds`; a test that re-measures +30 s is measuring the constant, not the guard. Do not reach for +`tokio::time::pause()` to do this: auto-advance cannot tell a parked runtime +waiting on a socket from one waiting on a timer, so a real round-trip races the +next deadline and fails as `TimedOut` (tried; the login handshake failed). + +Some tests stay on the real clock because the wire is the subject: the pacing +tests in `limits.rs` cost `bytes / rate` and are irreducible, and the scan and +100-file tests in `sync-core` cost real hashing, uploads and fsyncs. Shrink +those with a smaller fixture, never with a fake clock. + ## Where the design lives | What | Where | @@ -100,7 +133,7 @@ heavy parallel load; run one alone before treating a failure as a regression. ## Screenshots The dashboard image in the README is a real frame from the app, seeded with -`crates/app/tests/screenshot_seed.rs`. Give the capture its own XDG directories +`crates/app/tests/it/screenshot_seed.rs`. Give the capture its own XDG directories so it cannot touch your real state: ```sh @@ -110,7 +143,7 @@ mkdir -p "$XDG_CONFIG_HOME" "$XDG_DATA_HOME/immich-sync" IMMICH_SYNC_CAPTURE_DB="$XDG_DATA_HOME/immich-sync/state.db" \ IMMICH_SYNC_CAPTURE_ROOT="$XDG_DATA_HOME/capture" \ -cargo test -p immich-sync --test screenshot_seed -- --ignored +cargo nextest run -p immich-sync -E 'test(screenshot_seed)' --run-ignored all ``` Then launch the binary with the same environment under Xvfb, with lavapipe for diff --git a/Cargo.lock b/Cargo.lock index 69d537a..8c04871 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -4463,7 +4463,6 @@ dependencies = [ "base64 0.22.1", "hex", "reqwest", - "serde", "serde_json", "sha1", "tokio", diff --git a/crates/app/src/logging.rs b/crates/app/src/logging.rs index cab325e..daedd17 100644 --- a/crates/app/src/logging.rs +++ b/crates/app/src/logging.rs @@ -7,7 +7,7 @@ //! //! Redaction is a property of the recorded types: `Secret` and `SessionToken` //! redact their `Debug` output, and no event formats `Secret::expose`. -//! `crates/app/tests/log_redaction.rs` greps the real file after real sign-ins +//! `crates/app/tests/it/log_redaction.rs` greps the real file after real sign-ins //! to prove it. use std::fs; @@ -120,7 +120,7 @@ impl LogLevel { /// exercise the pruning the appender runs when it opens the directory. /// `tracing-appender` reads its own clock, so the prune that runs at a timed /// rollover uses the same code path but cannot be triggered deterministically; -/// see `crates/app/tests/logging.rs`. +/// see `crates/app/tests/it/logging.rs`. #[derive(Clone, Debug)] pub struct LogPolicy { /// When a new log file starts. diff --git a/crates/app/src/shell.rs b/crates/app/src/shell.rs index 8bbeb65..b5fb044 100644 --- a/crates/app/src/shell.rs +++ b/crates/app/src/shell.rs @@ -307,7 +307,7 @@ pub fn main_window_options(cx: &App) -> WindowOptions { // width the widest row of any surface still fits in, the Dashboard's // header, whose title block beside the Sync now and Pause buttons needs // 509 px; 480 px is the height the Logs surface's cards keep their - // floors at in their narrowest wrap (#54). `tests/shell.rs` stands at + // floors at in their narrowest wrap (#54). `tests/it/shell.rs` stands at // exactly this size, so a layout that outgrows it fails there. window_min_size: Some(size(px(520.), px(480.))), ..Default::default() diff --git a/crates/app/tests/common/mod.rs b/crates/app/tests/it/common/mod.rs similarity index 100% rename from crates/app/tests/common/mod.rs rename to crates/app/tests/it/common/mod.rs diff --git a/crates/app/tests/dashboard.rs b/crates/app/tests/it/dashboard.rs similarity index 99% rename from crates/app/tests/dashboard.rs rename to crates/app/tests/it/dashboard.rs index 96ba90f..56c711d 100644 --- a/crates/app/tests/dashboard.rs +++ b/crates/app/tests/it/dashboard.rs @@ -5,7 +5,7 @@ //! the whole dashboard. The rendered-view test opens a real headless window and //! reads native accessibility properties and layout bounds. -mod common; +use super::common; use std::collections::BTreeSet; diff --git a/crates/app/tests/diagnostics.rs b/crates/app/tests/it/diagnostics.rs similarity index 99% rename from crates/app/tests/diagnostics.rs rename to crates/app/tests/it/diagnostics.rs index 81aab11..4e3ed09 100644 --- a/crates/app/tests/diagnostics.rs +++ b/crates/app/tests/it/diagnostics.rs @@ -6,7 +6,7 @@ //! cannot read pixels, so "readable in both themes" is checked by rendering //! under both and reading the same accessibility properties. -mod common; +use super::common; use common::{contrast, mock_server, press_reaches}; diff --git a/crates/app/tests/engine_model.rs b/crates/app/tests/it/engine_model.rs similarity index 99% rename from crates/app/tests/engine_model.rs rename to crates/app/tests/it/engine_model.rs index 9ba099e..2696791 100644 --- a/crates/app/tests/engine_model.rs +++ b/crates/app/tests/it/engine_model.rs @@ -1,6 +1,6 @@ //! The app-scoped engine model's start handoff (SPEC R8.7). -mod common; +use super::common; use common::mock_server; diff --git a/crates/app/tests/failed_ui.rs b/crates/app/tests/it/failed_ui.rs similarity index 100% rename from crates/app/tests/failed_ui.rs rename to crates/app/tests/it/failed_ui.rs diff --git a/crates/app/tests/folder_detail.rs b/crates/app/tests/it/folder_detail.rs similarity index 99% rename from crates/app/tests/folder_detail.rs rename to crates/app/tests/it/folder_detail.rs index 95671b8..61d392a 100644 --- a/crates/app/tests/folder_detail.rs +++ b/crates/app/tests/it/folder_detail.rs @@ -5,7 +5,7 @@ //! state database, the real engine model, and the view's own click and keyboard //! handling. -mod common; +use super::common; use std::path::Path; use std::sync::Arc; diff --git a/crates/app/tests/keyring.rs b/crates/app/tests/it/keyring.rs similarity index 100% rename from crates/app/tests/keyring.rs rename to crates/app/tests/it/keyring.rs diff --git a/crates/app/tests/log_redaction.rs b/crates/app/tests/it/log_redaction.rs similarity index 99% rename from crates/app/tests/log_redaction.rs rename to crates/app/tests/it/log_redaction.rs index 3fa4e17..ffc0b71 100644 --- a/crates/app/tests/log_redaction.rs +++ b/crates/app/tests/it/log_redaction.rs @@ -11,7 +11,7 @@ //! echoes the submitted password in its error body, so a server-supplied //! message cannot slip a secret into the log either. -mod common; +use super::common; use common::mock_server; diff --git a/crates/app/tests/logging.rs b/crates/app/tests/it/logging.rs similarity index 100% rename from crates/app/tests/logging.rs rename to crates/app/tests/it/logging.rs diff --git a/crates/app/tests/login.rs b/crates/app/tests/it/login.rs similarity index 100% rename from crates/app/tests/login.rs rename to crates/app/tests/it/login.rs diff --git a/crates/app/tests/logs_ui.rs b/crates/app/tests/it/logs_ui.rs similarity index 99% rename from crates/app/tests/logs_ui.rs rename to crates/app/tests/it/logs_ui.rs index 2cd40fb..90f162c 100644 --- a/crates/app/tests/logs_ui.rs +++ b/crates/app/tests/it/logs_ui.rs @@ -6,7 +6,7 @@ //! properties; the colour tokens themselves are covered by the WCAG test in //! `tests/dashboard.rs`. -mod common; +use super::common; use std::fs; use std::path::{Path, PathBuf}; diff --git a/crates/app/tests/it/main.rs b/crates/app/tests/it/main.rs new file mode 100644 index 0000000..028d841 --- /dev/null +++ b/crates/app/tests/it/main.rs @@ -0,0 +1,26 @@ +//! One test binary for the whole crate. +//! +//! Separate files under `tests/` each became a separately linked binary (~230 MB +//! for this crate: 51 links across the workspace, 19 of them re-linking the whole +//! GPUI stack. One `main.rs` with the files as modules links once. + +mod common; +mod dashboard; +mod diagnostics; +mod engine_model; +mod failed_ui; +mod folder_detail; +mod keyring; +mod log_redaction; +mod logging; +mod login; +mod logs_ui; +mod notices; +mod packaging; +mod review_ui; +mod screenshot_seed; +mod settings; +mod settings_ui; +mod shell; +mod upload_ui; +mod window; diff --git a/crates/app/tests/notices.rs b/crates/app/tests/it/notices.rs similarity index 99% rename from crates/app/tests/notices.rs rename to crates/app/tests/it/notices.rs index fa8acc7..96372dd 100644 --- a/crates/app/tests/notices.rs +++ b/crates/app/tests/it/notices.rs @@ -5,7 +5,7 @@ //! accent colour, and a long quoted path wraps instead of clipping at a //! window narrow enough to squeeze the card. -mod common; +use super::common; use common::scroll_dashboard_into_view; use gpui_kit::base::test_support::{ElementSnapshot, snapshots}; diff --git a/crates/app/tests/packaging.rs b/crates/app/tests/it/packaging.rs similarity index 100% rename from crates/app/tests/packaging.rs rename to crates/app/tests/it/packaging.rs diff --git a/crates/app/tests/review_ui.rs b/crates/app/tests/it/review_ui.rs similarity index 100% rename from crates/app/tests/review_ui.rs rename to crates/app/tests/it/review_ui.rs diff --git a/crates/app/tests/screenshot_seed.rs b/crates/app/tests/it/screenshot_seed.rs similarity index 100% rename from crates/app/tests/screenshot_seed.rs rename to crates/app/tests/it/screenshot_seed.rs diff --git a/crates/app/tests/settings.rs b/crates/app/tests/it/settings.rs similarity index 100% rename from crates/app/tests/settings.rs rename to crates/app/tests/it/settings.rs diff --git a/crates/app/tests/settings_ui.rs b/crates/app/tests/it/settings_ui.rs similarity index 99% rename from crates/app/tests/settings_ui.rs rename to crates/app/tests/it/settings_ui.rs index 18f3508..3a75347 100644 --- a/crates/app/tests/settings_ui.rs +++ b/crates/app/tests/it/settings_ui.rs @@ -3,7 +3,7 @@ //! The harness can drive keyboard focus and native accessibility properties; it //! cannot read pixels, so the focus *ring* itself is not asserted here. -mod common; +use super::common; use common::mock_server; diff --git a/crates/app/tests/shell.rs b/crates/app/tests/it/shell.rs similarity index 99% rename from crates/app/tests/shell.rs rename to crates/app/tests/it/shell.rs index 61f78b5..aca482b 100644 --- a/crates/app/tests/shell.rs +++ b/crates/app/tests/it/shell.rs @@ -1,10 +1,11 @@ //! Headless tests for the shell's window effects. //! //! `cx.quit()` and the tray icon are platform effects that a headless test -//! cannot observe, so those are checked by the probe and the GUI smoke job. +//! cannot observe, so those are checked by the probe and, on a real machine, +//! by the capture recipe in `CONTRIBUTING.md`. //! The state-to-tooltip mapping is pure, so it is tested here. -mod common; +use super::common; use std::path::Path; use std::sync::Arc; diff --git a/crates/app/tests/upload_ui.rs b/crates/app/tests/it/upload_ui.rs similarity index 99% rename from crates/app/tests/upload_ui.rs rename to crates/app/tests/it/upload_ui.rs index 5d495c7..3d41ba6 100644 --- a/crates/app/tests/upload_ui.rs +++ b/crates/app/tests/it/upload_ui.rs @@ -3,7 +3,7 @@ //! The view is driven through its real controls: the path field, the button, //! and the result notice the window renders. -mod common; +use super::common; use common::mock_server; diff --git a/crates/app/tests/window.rs b/crates/app/tests/it/window.rs similarity index 100% rename from crates/app/tests/window.rs rename to crates/app/tests/it/window.rs diff --git a/crates/immich-client/src/bandwidth.rs b/crates/immich-client/src/bandwidth.rs index 520b02c..134ea6d 100644 --- a/crates/immich-client/src/bandwidth.rs +++ b/crates/immich-client/src/bandwidth.rs @@ -175,12 +175,19 @@ impl BandwidthLimit { /// Charges `bytes` to the budget. The debt may go negative; the next /// refill repays it, which is what keeps the long-run rate honest. fn debit(&self, bytes: u64) { + self.debit_at(bytes, Instant::now()); + } + + /// [`Self::debit`] at `now`. The test seam: a thread descheduled between + /// two wall-clock reads would otherwise refill the bucket behind the + /// assertion's back. + fn debit_at(&self, bytes: u64, now: Instant) { let rate = self.bytes_per_second(); if rate == 0 || bytes == 0 { return; } let mut bucket = self.bucket.lock().unwrap_or_else(PoisonError::into_inner); - bucket.refill(rate); + bucket.refill_at(rate, now); bucket.tokens -= bytes as f64; } @@ -188,10 +195,33 @@ impl BandwidthLimit { /// a known instant instead of from the wall clock. #[cfg(test)] fn set_idle(&self, elapsed: Duration) { + self.set_idle_at(elapsed, Instant::now()); + } + + /// [`Self::set_idle`] at `now`; paired with [`Self::wait_for_at`] so a test + /// takes one reading of the clock and cannot be raced by the refill. + #[cfg(test)] + fn set_idle_at(&self, elapsed: Duration, now: Instant) { self.bucket .lock() .unwrap_or_else(PoisonError::into_inner) - .refilled = Instant::now() - elapsed; + .refilled = now - elapsed; + } + + /// [`Self::wait_for`] at `now`, for tests that pin the instant. + #[cfg(test)] + fn wait_for_at(&self, bytes: u64, now: Instant) -> Duration { + let rate = self.bytes_per_second(); + if rate == 0 { + return Duration::ZERO; + } + let mut bucket = self.bucket.lock().unwrap_or_else(PoisonError::into_inner); + let tokens = bucket.refill_at(rate, now); + if tokens >= bytes as f64 { + Duration::ZERO + } else { + Duration::from_secs_f64((bytes as f64 - tokens) / rate as f64) + } } } @@ -213,7 +243,12 @@ impl Bucket { /// Tops the bucket up for the time since the last refill and returns the /// tokens now available. fn refill(&mut self, rate: u64) -> f64 { - let now = Instant::now(); + self.refill_at(rate, Instant::now()) + } + + /// [`Self::refill`] as of `now`, so a test's reading and its refill share + /// one instant. + fn refill_at(&mut self, rate: u64, now: Instant) -> f64 { let elapsed = now.duration_since(self.refilled).as_secs_f64(); self.refilled = now; let capacity = (rate / BURST_DIVISOR).max(BUCKET_FLOOR) as f64; @@ -354,19 +389,22 @@ mod tests { #[test] fn a_spent_bucket_reports_the_time_the_budget_needs() { let limit = BandwidthLimit::new(1_000); - // The bucket is measured from a known instant. It used to be measured - // from `BandwidthLimit::new()`, so a test thread descheduled in - // between refilled the bucket and failed the absolute bounds below. - limit.set_idle(Duration::ZERO); - let first = limit.wait_for(100); + // The bucket and both readings are measured from one instant. It used + // to be measured from `BandwidthLimit::new()`, and then from a second + // `Instant::now()`, so a test thread descheduled in between refilled + // the bucket and failed the absolute bounds below -- which is what + // nextest's higher parallelism exposed. + let now = Instant::now(); + limit.set_idle_at(Duration::ZERO, now); + let first = limit.wait_for_at(100, now); assert!( first.abs_diff(Duration::from_millis(100)) <= Duration::from_millis(10), "an empty bucket at 1 kB/s needs 100 ms: {first:?}" ); // A hundred charged bytes add their own tenth of a second on top. - limit.debit(100); - limit.set_idle(Duration::ZERO); - let second = limit.wait_for(100); + limit.debit_at(100, now); + limit.set_idle_at(Duration::ZERO, now); + let second = limit.wait_for_at(100, now); assert!( second.abs_diff(Duration::from_millis(200)) <= Duration::from_millis(10), "a hundred charged bytes add their own hundred: {second:?}" diff --git a/crates/immich-client/src/client.rs b/crates/immich-client/src/client.rs index 9e32115..ac83866 100644 --- a/crates/immich-client/src/client.rs +++ b/crates/immich-client/src/client.rs @@ -24,27 +24,13 @@ use crate::model::{ /// answer with an HTML page; the client keeps a short, useful prefix. const MESSAGE_LIMIT: usize = 200; -/// A request that has not finished within this window is unreachable. -const REQUEST_TIMEOUT: Duration = Duration::from_secs(30); - -/// The window [`send`] enforces. Production always gets [`REQUEST_TIMEOUT`]; -/// only a unit test can shrink it, so the stalled-server test does not spend -/// half a minute on the wall clock. -fn request_timeout() -> Duration { - #[cfg(test)] - { - let millis = TEST_TIMEOUT_MILLIS.load(std::sync::atomic::Ordering::Relaxed); - if millis > 0 { - return Duration::from_millis(millis); - } - } - REQUEST_TIMEOUT -} - -/// The test seam [`request_timeout`] reads. It exists only in the unit-test -/// build and defaults to zero, which means [`REQUEST_TIMEOUT`]. -#[cfg(test)] -static TEST_TIMEOUT_MILLIS: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); +/// The window a request has to finish, and the budget an upload may hand the +/// server no byte before it is called stalled. +/// +/// A test shrinks it on the client it builds ([`ImmichClient::with_window`]), +/// so a stall is measured in milliseconds instead of half a minute of wall +/// clock, and the value a production client gets is asserted here. +const REQUEST_WINDOW: Duration = Duration::from_secs(30); /// How long an upload may hand the server no byte before it is called stalled. /// @@ -57,56 +43,6 @@ static TEST_TIMEOUT_MILLIS: std::sync::atomic::AtomicU64 = std::sync::atomic::At /// A window that sees bytes move is renewed whole, so the stall is called at /// the last byte the server took plus this budget: at most twice it, never at /// one window from the start of the gap. -const STALL_BUDGET: Duration = REQUEST_TIMEOUT; - -/// Sends a request under the ordinary window. -/// -/// An upload does not come through here: a total deadline cannot tell a slow -/// upload from a stalled server, and one derived from the limit in force when -/// the request was built aborts a healthy upload the moment the limit changes. -async fn send(request: RequestBuilder) -> Result { - request - .timeout(request_timeout()) - .send() - .await - .map_err(ClientError::Unreachable) -} - -/// Sends an upload, failing it when the body has made no progress for -/// [`STALL_BUDGET`]. -/// -/// The upload carries no total deadline: the client's 30 s would cut off any -/// throttled upload larger than `limit x 30 s`, and a deadline scaled by the -/// limit in force when the request was built grows without bound for a large -/// file at a slow one (R7.1). Progress is watched instead, so the bound holds -/// whatever the limit does while the request is on the wire. A window that saw -/// bytes is renewed in full, so the call comes at the last byte moved plus -/// [`STALL_BUDGET`] - at most twice that budget. -async fn send_upload( - request: RequestBuilder, - progress: &Progress, -) -> Result { - let mut send = Box::pin(request.send()); - // The last byte that had moved when this window opened. - let mut waited = progress.stamp(); - loop { - if let Ok(response) = - tokio::time::timeout(STALL_BUDGET.saturating_sub(progress.idle()), &mut send).await - { - return response.map_err(ClientError::Unreachable); - } - // The window closed with nothing moving, and a timer that fires a - // fraction late must not count as progress on its own: the server - // stopped reading. - let moved = progress.stamp(); - if moved == waited { - return Err(ClientError::Stalled); - } - // Bytes did move: the request is healthy and waits out a full window. - waited = moved; - } -} - /// A typed client for the Immich Stable operations the client uses. /// /// The client is cheap to clone and holds no credential. The session owns the @@ -115,6 +51,8 @@ async fn send_upload( pub struct ImmichClient { http: Client, base: Url, + /// [`REQUEST_WINDOW`], unless a test shrank it. + window: Duration, } /// The login request body. It does not derive `Debug`, so a password cannot @@ -189,11 +127,60 @@ fn quota_exceeded(message: &str) -> bool { } impl ImmichClient { + /// Sends a request under the ordinary window. + /// + /// An upload does not come through here: a total deadline cannot tell a slow + /// upload from a stalled server, and one derived from the limit in force when + /// the request was built aborts a healthy upload the moment the limit changes. + async fn send(&self, request: RequestBuilder) -> Result { + request + .timeout(self.window) + .send() + .await + .map_err(ClientError::Unreachable) + } + + /// Sends an upload, failing it when the body has made no progress for + /// [`REQUEST_WINDOW`]. + /// + /// The upload carries no total deadline: the client's 30 s would cut off any + /// throttled upload larger than `limit x 30 s`, and a deadline scaled by the + /// limit in force when the request was built grows without bound for a large + /// file at a slow one (R7.1). Progress is watched instead, so the bound holds + /// whatever the limit does while the request is on the wire. A window that saw + /// bytes is renewed in full, so the call comes at the last byte moved plus + /// [`REQUEST_WINDOW`] - at most twice that budget. + async fn send_upload( + &self, + request: RequestBuilder, + progress: &Progress, + ) -> Result { + let mut send = Box::pin(request.send()); + // The last byte that had moved when this window opened. + let mut waited = progress.stamp(); + loop { + if let Ok(response) = + tokio::time::timeout(self.window.saturating_sub(progress.idle()), &mut send).await + { + return response.map_err(ClientError::Unreachable); + } + // The window closed with nothing moving, and a timer that fires a + // fraction late must not count as progress on its own: the server + // stopped reading. + let moved = progress.stamp(); + if moved == waited { + return Err(ClientError::Stalled); + } + // Bytes did move: the request is healthy and waits out a full window. + waited = moved; + } + } + /// Builds a client for one server. TLS verification stays on (N3). /// - /// The client carries no deadline of its own: [`send`] puts the ordinary - /// window on every request but an upload, and an upload is bounded by the - /// stall guard instead. + /// The client carries no deadline of its own: [`Self::send`] puts the + /// ordinary window on every request but an upload, and an upload is + /// bounded by the stall guard instead. /// /// # Errors /// @@ -237,7 +224,24 @@ impl ImmichClient { base.set_path(&path); } let http = Client::builder().build().map_err(ClientError::HttpClient)?; - Ok(Self { http, base }) + Ok(Self { + http, + base, + window: REQUEST_WINDOW, + }) + } + + /// The same client with a different [`REQUEST_WINDOW`]. + /// + /// Every real-timing test used to pay the production window on the wall + /// clock: half a minute for a stall, twice that for a body blocked + /// mid-flight. Shrinking the window at the client keeps the test on a real + /// socket with the real guard, and the value a production client gets is + /// asserted in [`tests::the_production_window_is_the_specified_thirty_seconds`]. + #[must_use] + pub fn with_window(mut self, window: Duration) -> Self { + self.window = window; + self } /// Logs in with an email address and password (R1.3). @@ -248,12 +252,13 @@ impl ImmichClient { /// [`ClientError::PasswordLoginDisabled`] when the server has password /// login turned off (R1.6), and the transport and protocol variants. pub async fn login(&self, email: &str, password: &str) -> Result { - let response = send( - self.http - .post(self.endpoint("auth/login")) - .json(&LoginRequest { email, password }), - ) - .await?; + let response = self + .send( + self.http + .post(self.endpoint("auth/login")) + .json(&LoginRequest { email, password }), + ) + .await?; let result = self.read_login(response).await; log_outcome("login", &result); result @@ -266,12 +271,13 @@ impl ImmichClient { /// [`ClientError::InvalidToken`] when the server refuses the token, plus the /// transport and protocol variants. pub async fn validate_token(&self, token: &SessionToken) -> Result<(), ClientError> { - let response = send( - self.http - .post(self.endpoint("auth/validateToken")) - .bearer_auth(token.expose()), - ) - .await?; + let response = self + .send( + self.http + .post(self.endpoint("auth/validateToken")) + .bearer_auth(token.expose()), + ) + .await?; let result = self .read_auth_status(response, ClientError::InvalidToken) .await; @@ -286,12 +292,13 @@ impl ImmichClient { /// [`ClientError::InvalidApiKey`] when the server refuses the key, plus the /// transport and protocol variants. pub async fn validate_api_key(&self, key: &str) -> Result<(), ClientError> { - let response = send( - self.http - .post(self.endpoint("auth/validateToken")) - .header("x-api-key", key), - ) - .await?; + let response = self + .send( + self.http + .post(self.endpoint("auth/validateToken")) + .header("x-api-key", key), + ) + .await?; let result = self .read_auth_status(response, ClientError::InvalidApiKey) .await; @@ -311,8 +318,9 @@ impl ImmichClient { /// Returns the typed refusal classes (R6.3) and the transport and protocol /// variants. pub async fn about(&self, auth: Auth<'_>) -> Result { - let response = - send(self.authorize(self.http.get(self.endpoint("server/about")), auth)).await?; + let response = self + .send(self.authorize(self.http.get(self.endpoint("server/about")), auth)) + .await?; let status = response.status(); let result = if status.is_success() { response @@ -340,8 +348,9 @@ impl ImmichClient { /// other typed refusal classes (R6.3), and the transport and protocol /// variants. pub async fn api_key_permissions(&self, auth: Auth<'_>) -> Result { - let response = - send(self.authorize(self.http.get(self.endpoint("api-keys/me")), auth)).await?; + let response = self + .send(self.authorize(self.http.get(self.endpoint("api-keys/me")), auth)) + .await?; let status = response.status(); let result = if status.is_success() { response @@ -369,14 +378,15 @@ impl ImmichClient { auth: Auth<'_>, items: &[BulkCheckItem], ) -> Result, ClientError> { - let response = send( - self.authorize( - self.http.post(self.endpoint("assets/bulk-upload-check")), - auth, + let response = self + .send( + self.authorize( + self.http.post(self.endpoint("assets/bulk-upload-check")), + auth, + ) + .json(&BulkCheckRequest { assets: items }), ) - .json(&BulkCheckRequest { assets: items }), - ) - .await?; + .await?; let result = self.read_bulk_check(response).await; log_outcome("bulk-upload-check", &result); result @@ -456,13 +466,14 @@ impl ImmichClient { if let Some(video_id) = upload.live_photo_video_id { form = form.text("livePhotoVideoId", video_id.to_string()); } - let sent = send_upload( - self.authorize(self.http.post(self.endpoint("assets")), auth) - .header("x-immich-checksum", upload.checksum) - .multipart(form), - &progress, - ) - .await; + let sent = self + .send_upload( + self.authorize(self.http.post(self.endpoint("assets")), auth) + .header("x-immich-checksum", upload.checksum) + .multipart(form), + &progress, + ) + .await; let result = match sent { Ok(response) => self.read_upload(response).await, Err(lost) => { @@ -527,14 +538,15 @@ impl ImmichClient { name: &str, asset_ids: &[String], ) -> Result { - let response = send( - self.authorize(self.http.post(self.endpoint("albums")), auth) - .json(&CreateAlbumRequest { - album_name: name, - asset_ids, - }), - ) - .await?; + let response = self + .send( + self.authorize(self.http.post(self.endpoint("albums")), auth) + .json(&CreateAlbumRequest { + album_name: name, + asset_ids, + }), + ) + .await?; let result = self.read_album(response).await; log_outcome("create-album", &result); result @@ -558,15 +570,16 @@ impl ImmichClient { album_id: &str, name: &str, ) -> Result { - let response = send( - self.authorize( - self.http - .patch(self.endpoint(&format!("albums/{album_id}"))), - auth, + let response = self + .send( + self.authorize( + self.http + .patch(self.endpoint(&format!("albums/{album_id}"))), + auth, + ) + .json(&UpdateAlbumRequest { album_name: name }), ) - .json(&UpdateAlbumRequest { album_name: name }), - ) - .await?; + .await?; let result = self.read_album(response).await; log_outcome("rename-album", &result); result @@ -591,15 +604,16 @@ impl ImmichClient { album_id: &str, asset_ids: &[String], ) -> Result<(), ClientError> { - let response = send( - self.authorize( - self.http - .put(self.endpoint(&format!("albums/{album_id}/assets"))), - auth, + let response = self + .send( + self.authorize( + self.http + .put(self.endpoint(&format!("albums/{album_id}/assets"))), + auth, + ) + .json(&AlbumAssetsRequest { ids: asset_ids }), ) - .json(&AlbumAssetsRequest { ids: asset_ids }), - ) - .await?; + .await?; let result = self.read_accepted(response).await; log_outcome("add-album-assets", &result); result @@ -620,11 +634,12 @@ impl ImmichClient { auth: Auth<'_>, asset_id: &str, ) -> Result { - let response = send(self.authorize( - self.http.get(self.endpoint(&format!("assets/{asset_id}"))), - auth, - )) - .await?; + let response = self + .send(self.authorize( + self.http.get(self.endpoint(&format!("assets/{asset_id}"))), + auth, + )) + .await?; let result = self.read_asset_info(response).await; log_outcome("asset-info", &result); result @@ -651,19 +666,20 @@ impl ImmichClient { target_id: &str, stack: bool, ) -> Result<(), ClientError> { - let response = send( - self.authorize(self.http.put(self.endpoint("assets/copy")), auth) - .json(&AssetCopyRequest { - source_id, - target_id, - albums: true, - shared_links: true, - favorite: true, - sidecar: true, - stack, - }), - ) - .await?; + let response = self + .send( + self.authorize(self.http.put(self.endpoint("assets/copy")), auth) + .json(&AssetCopyRequest { + source_id, + target_id, + albums: true, + shared_links: true, + favorite: true, + sidecar: true, + stack, + }), + ) + .await?; let result = self.read_accepted(response).await; log_outcome("copy-asset", &result); result @@ -685,11 +701,12 @@ impl ImmichClient { auth: Auth<'_>, asset_ids: &[String], ) -> Result { - let response = send( - self.authorize(self.http.post(self.endpoint("stacks")), auth) - .json(&CreateStackRequest { asset_ids }), - ) - .await?; + let response = self + .send( + self.authorize(self.http.post(self.endpoint("stacks")), auth) + .json(&CreateStackRequest { asset_ids }), + ) + .await?; let result = self.read_stack(response).await; log_outcome("create-stack", &result); result @@ -709,11 +726,12 @@ impl ImmichClient { /// not carry the tag that was asked for, and the transport variants. pub async fn upsert_tag(&self, auth: Auth<'_>, name: &str) -> Result { let names = [name.to_string()]; - let response = send( - self.authorize(self.http.put(self.endpoint("tags")), auth) - .json(&TagUpsertRequest { tags: &names }), - ) - .await?; + let response = self + .send( + self.authorize(self.http.put(self.endpoint("tags")), auth) + .json(&TagUpsertRequest { tags: &names }), + ) + .await?; let result = self.read_tag(response, name).await; log_outcome("upsert-tag", &result); result @@ -733,11 +751,12 @@ impl ImmichClient { tag_ids: &[String], asset_ids: &[String], ) -> Result<(), ClientError> { - let response = send( - self.authorize(self.http.put(self.endpoint("tags/assets")), auth) - .json(&BulkTagRequest { tag_ids, asset_ids }), - ) - .await?; + let response = self + .send( + self.authorize(self.http.put(self.endpoint("tags/assets")), auth) + .json(&BulkTagRequest { tag_ids, asset_ids }), + ) + .await?; let result = self.read_accepted(response).await; log_outcome("bulk-tag-assets", &result); result @@ -760,14 +779,15 @@ impl ImmichClient { auth: Auth<'_>, asset_ids: &[String], ) -> Result<(), ClientError> { - let response = send( - self.authorize(self.http.delete(self.endpoint("assets")), auth) - .json(&AssetBulkDeleteRequest { - ids: asset_ids, - force: false, - }), - ) - .await?; + let response = self + .send( + self.authorize(self.http.delete(self.endpoint("assets")), auth) + .json(&AssetBulkDeleteRequest { + ids: asset_ids, + force: false, + }), + ) + .await?; let result = self.read_accepted(response).await; log_outcome("trash-assets", &result); result @@ -792,11 +812,12 @@ impl ImmichClient { auth: Auth<'_>, asset_ids: &[String], ) -> Result<(), ClientError> { - let response = send( - self.authorize(self.http.post(self.endpoint("trash/restore/assets")), auth) - .json(&BulkIdsRequest { ids: asset_ids }), - ) - .await?; + let response = self + .send( + self.authorize(self.http.post(self.endpoint("trash/restore/assets")), auth) + .json(&BulkIdsRequest { ids: asset_ids }), + ) + .await?; let result = self.read_accepted(response).await; log_outcome("restore-assets", &result); result @@ -1099,6 +1120,16 @@ mod tests { assert_eq!(iso8601(SystemTime::UNIX_EPOCH), "1970-01-01T00:00:00.000Z"); } + /// The window the stall guard and the request deadline use, and the value + /// the real-time tests shrink. It is the SPEC's half-minute; a test that + /// shrinks it measures the guard, not this number. + #[test] + fn the_production_window_is_the_specified_thirty_seconds() { + assert_eq!(REQUEST_WINDOW, Duration::from_secs(30)); + let client = ImmichClient::new("http://127.0.0.1:2283").expect("a valid URL"); + assert_eq!(client.window, REQUEST_WINDOW); + } + #[test] fn only_immichs_quota_message_is_a_quota_refusal() { assert!(quota_exceeded("Quota has been exceeded!")); @@ -1112,15 +1143,15 @@ mod tests { mock_immich::Operation::CreateAlbum, mock_immich::Fault::Slow(Duration::from_secs(30)), ); - TEST_TIMEOUT_MILLIS.store(200, std::sync::atomic::Ordering::Relaxed); - let client = ImmichClient::new(server.base_url()).expect("a valid URL"); + let client = ImmichClient::new(server.base_url()) + .expect("a valid URL") + .with_window(Duration::from_millis(200)); let error = client .create_album(Auth::ApiKey("test-key"), "album", &[]) .await .expect_err("a server that accepts and never answers must fail the call"); - TEST_TIMEOUT_MILLIS.store(0, std::sync::atomic::Ordering::Relaxed); assert!( matches!(&error, ClientError::Unreachable(inner) if inner.is_timeout()), "the ordinary request window must end the call, got {error:?}" diff --git a/crates/immich-client/tests/albums.rs b/crates/immich-client/tests/it/albums.rs similarity index 100% rename from crates/immich-client/tests/albums.rs rename to crates/immich-client/tests/it/albums.rs diff --git a/crates/immich-client/tests/client.rs b/crates/immich-client/tests/it/client.rs similarity index 100% rename from crates/immich-client/tests/client.rs rename to crates/immich-client/tests/it/client.rs diff --git a/crates/immich-client/tests/limits.rs b/crates/immich-client/tests/it/limits.rs similarity index 86% rename from crates/immich-client/tests/limits.rs rename to crates/immich-client/tests/it/limits.rs index a761381..f04c971 100644 --- a/crates/immich-client/tests/limits.rs +++ b/crates/immich-client/tests/it/limits.rs @@ -16,6 +16,12 @@ const ONE_MEGABYTE_PER_SECOND: u64 = 1_000_000; /// the pace is what the clock measures, and under axum's 2 MB body limit in /// the double. const FILE_BYTES: usize = 1_572_864; +/// The window these tests run the guard on. Production gets the 30 s window +/// (`client.rs` asserts that); the knowledge under test is *that* the stall +/// guard fires a window after the last byte moved, and half a minute of wall +/// clock per test is not what measures it. +const WINDOW: Duration = Duration::from_millis(200); + /// A checksum for the request header. The double stores the checksum it /// computes from the body, so this one only has to be well-formed. const CHECKSUM: &str = "0123456789abcdef0123456789abcdef01234567"; @@ -47,6 +53,12 @@ async fn client(server: &MockImmich) -> (ImmichClient, SessionToken) { (client, token) } +/// The same logged-in client, on the short window. +async fn short_window_client(server: &MockImmich) -> (ImmichClient, SessionToken) { + let (client, token) = client(server).await; + (client.with_window(WINDOW), token) +} + fn upload_request<'a>(path: &'a std::path::Path, limit: &BandwidthLimit) -> AssetUpload<'a> { AssetUpload { path, @@ -144,12 +156,12 @@ async fn a_server_that_stops_answering_fails_the_upload_within_the_stall_budget( // accepted the upload and stopped talking is exactly this. Nothing is in // flight on the socket by then, so the only bound left is the client's. server.fail(Operation::Upload, Fault::Slow(Duration::from_secs(300))); - let (client, token) = client(&server).await; + let (client, token) = short_window_client(&server).await; let (_directory, path) = fixture(FILE_BYTES); - // 1.5 MiB at 200 kB/s is 7.9 s of streaming, so the deadline this replaces - // - three times the bytes' own time plus the client window - was 53 s, and - // nothing on the wire would have ended the upload inside 30 s. - let limit = BandwidthLimit::new(200_000); + // A limit is in force, so the body streams rather than leaving in one + // burst, and it costs several windows to do it: the guard must not fire + // while the bytes keep moving, only once the server stops talking. + let limit = BandwidthLimit::new(4_000_000); let started = Instant::now(); let error = client @@ -159,9 +171,13 @@ async fn a_server_that_stops_answering_fails_the_upload_within_the_stall_budget( let elapsed = started.elapsed(); assert!(matches!(error, ClientError::Stalled), "{error:?}"); + // The body's own paced time, then one window with the server silent. A + // deadline derived from the limit would have ended it before the body was + // out; the guard must let the whole body through and then call the stall. + let streaming = Duration::from_secs_f64(FILE_BYTES as f64 / 4_000_000.0); assert!( - elapsed <= Duration::from_secs(60), - "a stalled upload took {elapsed:?}; the bound has to be the stall budget" + elapsed >= streaming && elapsed <= streaming + WINDOW + Duration::from_millis(150), + "a stalled upload took {elapsed:?}: {streaming:?} of streaming plus a {WINDOW:?} window" ); } @@ -169,7 +185,7 @@ async fn a_server_that_stops_answering_fails_the_upload_within_the_stall_budget( async fn an_unlimited_upload_still_gives_up_at_the_client_wide_window() { let server = MockImmich::start().await; server.fail(Operation::Upload, Fault::Slow(Duration::from_secs(300))); - let (client, token) = client(&server).await; + let (client, token) = short_window_client(&server).await; let (_directory, path) = fixture(FILE_BYTES); let limit = BandwidthLimit::new(0); @@ -182,9 +198,9 @@ async fn an_unlimited_upload_still_gives_up_at_the_client_wide_window() { assert!(matches!(error, ClientError::Stalled), "{error:?}"); // An unlimited upload carries no total deadline any more, but it must not - // wait any longer than the 30 s window every request has always had. + // wait any longer than the window every request has always had. assert!( - elapsed >= Duration::from_secs(29) && elapsed <= Duration::from_secs(45), + elapsed >= WINDOW && elapsed <= WINDOW + Duration::from_millis(100), "an unlimited upload to a stalled server failed after {elapsed:?}" ); } @@ -247,7 +263,9 @@ async fn server_that_stops_reading() -> StoppedReading { #[tokio::test] async fn a_server_that_stops_reading_mid_body_fails_the_upload_within_the_stall_budget() { let server = server_that_stops_reading().await; - let client = ImmichClient::new(&server.url).expect("a loopback URL is valid"); + let client = ImmichClient::new(&server.url) + .expect("a loopback URL is valid") + .with_window(WINDOW); let (_directory, path) = fixture(BACKPRESSURE_BYTES); // Unlimited, so the only thing that can stop the body is the socket. let limit = BandwidthLimit::new(0); @@ -263,8 +281,8 @@ async fn a_server_that_stops_reading_mid_body_fails_the_upload_within_the_stall_ // The guard can only notice at the end of a window, so it needs at least // one and at most two of them: it saw bytes still moving in the first. assert!( - elapsed >= Duration::from_secs(29) && elapsed <= Duration::from_secs(65), - "an upload blocked mid-body gave up after {elapsed:?}" + elapsed >= WINDOW && elapsed <= WINDOW * 2 + Duration::from_millis(100), + "an upload blocked mid-body gave up after {elapsed:?}, on a {WINDOW:?} window" ); // The client could not get the whole body out, so it was blocked with // bytes still to send when the guard gave up - a server that stopped @@ -287,12 +305,12 @@ async fn a_server_that_stops_reading_mid_body_fails_the_upload_within_the_stall_ #[tokio::test] async fn lowering_the_limit_mid_upload_does_not_abort_a_healthy_upload() { let server = MockImmich::start().await; - let (client, token) = client(&server).await; + let (client, token) = short_window_client(&server).await; let (_directory, path) = fixture(FILE_BYTES); // The engine starts unlimited, so the request is built with no limit in - // force and used to carry the client's 30 s total deadline. At 20 kB/s - // the rest of the file needs well over a minute of streaming, which that - // deadline cut short however well the server was behaving. + // force and used to carry the client's total deadline. At 200 kB/s the + // rest of the file streams for many times the window, which a total + // deadline would cut short however well the server was behaving. let limit = BandwidthLimit::new(0); let started = Instant::now(); @@ -308,7 +326,7 @@ async fn lowering_the_limit_mid_upload_does_not_abort_a_healthy_upload() { while server.upload_requests() == 0 { tokio::time::sleep(Duration::from_millis(5)).await; } - limit.set(20_000); + limit.set(200_000); let response = upload .await @@ -326,7 +344,7 @@ async fn lowering_the_limit_mid_upload_does_not_abort_a_healthy_upload() { // the limit landed is the machine's business, and the proof is that the // upload finishes at all rather than dying on a deadline nobody re-read. assert!( - elapsed <= Duration::from_secs(150), + elapsed <= Duration::from_secs(30), "a slow but progressing upload took {elapsed:?}" ); } diff --git a/crates/immich-client/tests/it/main.rs b/crates/immich-client/tests/it/main.rs new file mode 100644 index 0000000..d0f30a6 --- /dev/null +++ b/crates/immich-client/tests/it/main.rs @@ -0,0 +1,12 @@ +//! One test binary for the whole crate. +//! +//! Separate files under `tests/` each became a separately linked binary (~230 MB +//! for this crate: 51 links across the workspace, 19 of them re-linking the whole +//! GPUI stack. One `main.rs` with the files as modules links once. + +mod albums; +mod client; +mod limits; +mod tags; +mod trash; +mod upload; diff --git a/crates/immich-client/tests/tags.rs b/crates/immich-client/tests/it/tags.rs similarity index 100% rename from crates/immich-client/tests/tags.rs rename to crates/immich-client/tests/it/tags.rs diff --git a/crates/immich-client/tests/trash.rs b/crates/immich-client/tests/it/trash.rs similarity index 100% rename from crates/immich-client/tests/trash.rs rename to crates/immich-client/tests/it/trash.rs diff --git a/crates/immich-client/tests/upload.rs b/crates/immich-client/tests/it/upload.rs similarity index 100% rename from crates/immich-client/tests/upload.rs rename to crates/immich-client/tests/it/upload.rs diff --git a/crates/mock-immich/Cargo.toml b/crates/mock-immich/Cargo.toml index 7a4fb2f..40f728f 100644 --- a/crates/mock-immich/Cargo.toml +++ b/crates/mock-immich/Cargo.toml @@ -10,15 +10,15 @@ publish = false axum = { version = "0.8", features = ["multipart"] } base64 = "0.22" hex = "0.4" -serde = { version = "1", features = ["derive"] } serde_json = "1" sha1 = "0.10" tokio = { version = "1", features = ["macros", "net", "rt-multi-thread", "sync", "time"] } tokio-stream = "0.1" uuid = { version = "1", features = ["v4"] } +# No `serde`: the double builds every body with `serde_json::Value` and axum's +# `Json`, and derives nothing. `hex`, `sha1` and `tokio` were also listed here +# with the same or a subset of the features above, which a dev target already +# sees through the normal dependency; only `reqwest` needs its own entry. [dev-dependencies] -hex = "0.4" reqwest = { version = "0.13", default-features = false, features = ["json", "multipart", "rustls"] } -sha1 = "0.10" -tokio = { version = "1", features = ["macros", "rt-multi-thread"] } diff --git a/crates/mock-immich/tests/about.rs b/crates/mock-immich/tests/it/about.rs similarity index 100% rename from crates/mock-immich/tests/about.rs rename to crates/mock-immich/tests/it/about.rs diff --git a/crates/mock-immich/tests/albums.rs b/crates/mock-immich/tests/it/albums.rs similarity index 100% rename from crates/mock-immich/tests/albums.rs rename to crates/mock-immich/tests/it/albums.rs diff --git a/crates/mock-immich/tests/api_keys.rs b/crates/mock-immich/tests/it/api_keys.rs similarity index 100% rename from crates/mock-immich/tests/api_keys.rs rename to crates/mock-immich/tests/it/api_keys.rs diff --git a/crates/mock-immich/tests/assets_ops.rs b/crates/mock-immich/tests/it/assets_ops.rs similarity index 100% rename from crates/mock-immich/tests/assets_ops.rs rename to crates/mock-immich/tests/it/assets_ops.rs diff --git a/crates/mock-immich/tests/auth.rs b/crates/mock-immich/tests/it/auth.rs similarity index 100% rename from crates/mock-immich/tests/auth.rs rename to crates/mock-immich/tests/it/auth.rs diff --git a/crates/mock-immich/tests/faults.rs b/crates/mock-immich/tests/it/faults.rs similarity index 100% rename from crates/mock-immich/tests/faults.rs rename to crates/mock-immich/tests/it/faults.rs diff --git a/crates/mock-immich/tests/it/main.rs b/crates/mock-immich/tests/it/main.rs new file mode 100644 index 0000000..f910ebc --- /dev/null +++ b/crates/mock-immich/tests/it/main.rs @@ -0,0 +1,15 @@ +//! One test binary for the whole crate. +//! +//! Separate files under `tests/` each became a separately linked binary (~230 MB +//! for this crate: 51 links across the workspace, 19 of them re-linking the whole +//! GPUI stack. One `main.rs` with the files as modules links once. + +mod about; +mod albums; +mod api_keys; +mod assets_ops; +mod auth; +mod faults; +mod sidecar; +mod tags; +mod upload; diff --git a/crates/mock-immich/tests/sidecar.rs b/crates/mock-immich/tests/it/sidecar.rs similarity index 100% rename from crates/mock-immich/tests/sidecar.rs rename to crates/mock-immich/tests/it/sidecar.rs diff --git a/crates/mock-immich/tests/tags.rs b/crates/mock-immich/tests/it/tags.rs similarity index 100% rename from crates/mock-immich/tests/tags.rs rename to crates/mock-immich/tests/it/tags.rs diff --git a/crates/mock-immich/tests/upload.rs b/crates/mock-immich/tests/it/upload.rs similarity index 100% rename from crates/mock-immich/tests/upload.rs rename to crates/mock-immich/tests/it/upload.rs diff --git a/crates/sync-core/src/backfill.rs b/crates/sync-core/src/backfill.rs index 14488d5..2db6d0e 100644 --- a/crates/sync-core/src/backfill.rs +++ b/crates/sync-core/src/backfill.rs @@ -145,7 +145,7 @@ impl Backfill { // completion would spend the one automatic pass before the first upload // exists, so a fresh install that starts with the tag on would never // tag the files it uploads next. - // `tests/tag_backfill.rs::an_empty_library_does_not_finish_the_backfill` pins this return. + // `tests/it/tag_backfill.rs::an_empty_library_does_not_finish_the_backfill` pins this return. if total == 0 { return; } diff --git a/crates/sync-core/tests/album_title.rs b/crates/sync-core/tests/it/album_title.rs similarity index 99% rename from crates/sync-core/tests/album_title.rs rename to crates/sync-core/tests/it/album_title.rs index 5d3644b..1dd95f1 100644 --- a/crates/sync-core/tests/album_title.rs +++ b/crates/sync-core/tests/it/album_title.rs @@ -5,7 +5,7 @@ //! title is one stored value; the tests assert where it lands, not how the //! mapper holds it on the way. -mod common; +use super::common; use std::path::{Path, PathBuf}; use std::sync::Arc; diff --git a/crates/sync-core/tests/common/mod.rs b/crates/sync-core/tests/it/common/mod.rs similarity index 100% rename from crates/sync-core/tests/common/mod.rs rename to crates/sync-core/tests/it/common/mod.rs diff --git a/crates/sync-core/tests/connection.rs b/crates/sync-core/tests/it/connection.rs similarity index 100% rename from crates/sync-core/tests/connection.rs rename to crates/sync-core/tests/it/connection.rs diff --git a/crates/sync-core/tests/dependency_rule.rs b/crates/sync-core/tests/it/dependency_rule.rs similarity index 95% rename from crates/sync-core/tests/dependency_rule.rs rename to crates/sync-core/tests/it/dependency_rule.rs index 77087b6..f156d21 100644 --- a/crates/sync-core/tests/dependency_rule.rs +++ b/crates/sync-core/tests/it/dependency_rule.rs @@ -7,7 +7,7 @@ const SHELL_NAMES: [&str; 2] = ["gpui", "immich_sync"]; #[test] fn sync_core_does_not_depend_on_the_shell() { - let manifest = include_str!("../Cargo.toml"); + let manifest = include_str!("../../Cargo.toml"); let dependencies = manifest .lines() .filter(|line| !line.trim_start().starts_with('#')); @@ -25,7 +25,7 @@ fn immich_client_does_not_depend_on_the_shell() { // The engine reaches the shell through sync-core's dependencies. The HTTP // client is on that path, so a GPUI dependency here would leak the shell // into the engine just the same. - let manifest = include_str!("../../immich-client/Cargo.toml"); + let manifest = include_str!("../../../immich-client/Cargo.toml"); let dependencies = manifest .lines() .filter(|line| !line.trim_start().starts_with('#')); diff --git a/crates/sync-core/tests/engine.rs b/crates/sync-core/tests/it/engine.rs similarity index 99% rename from crates/sync-core/tests/engine.rs rename to crates/sync-core/tests/it/engine.rs index 6e06833..fc2b175 100644 --- a/crates/sync-core/tests/engine.rs +++ b/crates/sync-core/tests/it/engine.rs @@ -4,7 +4,7 @@ //! Every test drives the real pipeline: a real directory, the real watcher and //! scan, the real state store, and the mock server's request counters. -mod common; +use super::common; use std::path::{Path, PathBuf}; use std::sync::Arc; diff --git a/crates/sync-core/tests/hash.rs b/crates/sync-core/tests/it/hash.rs similarity index 100% rename from crates/sync-core/tests/hash.rs rename to crates/sync-core/tests/it/hash.rs diff --git a/crates/sync-core/tests/limits.rs b/crates/sync-core/tests/it/limits.rs similarity index 99% rename from crates/sync-core/tests/limits.rs rename to crates/sync-core/tests/it/limits.rs index 8c5e4d6..7fd0185 100644 --- a/crates/sync-core/tests/limits.rs +++ b/crates/sync-core/tests/it/limits.rs @@ -3,7 +3,7 @@ //! Real timing against the offline double, because the criteria are a rate and //! a number of simultaneous uploads. -mod common; +use super::common; use std::path::{Path, PathBuf}; use std::sync::Arc; diff --git a/crates/sync-core/tests/log_redaction.rs b/crates/sync-core/tests/it/log_redaction.rs similarity index 100% rename from crates/sync-core/tests/log_redaction.rs rename to crates/sync-core/tests/it/log_redaction.rs diff --git a/crates/sync-core/tests/it/main.rs b/crates/sync-core/tests/it/main.rs new file mode 100644 index 0000000..2b18784 --- /dev/null +++ b/crates/sync-core/tests/it/main.rs @@ -0,0 +1,23 @@ +//! One test binary for the whole crate. +//! +//! Separate files under `tests/` each became a separately linked binary (~230 MB +//! for this crate: 51 links across the workspace, 19 of them re-linking the whole +//! GPUI stack. One `main.rs` with the files as modules links once. + +mod album_title; +mod common; +mod connection; +mod dependency_rule; +mod engine; +mod hash; +mod limits; +mod log_redaction; +mod meter; +mod missing; +mod pairing; +mod status; +mod store; +mod tag_backfill; +mod tags; +mod tracking; +mod upload; diff --git a/crates/sync-core/tests/meter.rs b/crates/sync-core/tests/it/meter.rs similarity index 99% rename from crates/sync-core/tests/meter.rs rename to crates/sync-core/tests/it/meter.rs index 3c04eb4..f2c3f32 100644 --- a/crates/sync-core/tests/meter.rs +++ b/crates/sync-core/tests/it/meter.rs @@ -5,7 +5,7 @@ //! a number of requests on the wire. The double's upload delay is what keeps a //! request in flight long enough to be sampled. -mod common; +use super::common; use std::path::{Path, PathBuf}; use std::sync::Arc; diff --git a/crates/sync-core/tests/missing.rs b/crates/sync-core/tests/it/missing.rs similarity index 100% rename from crates/sync-core/tests/missing.rs rename to crates/sync-core/tests/it/missing.rs diff --git a/crates/sync-core/tests/pairing.rs b/crates/sync-core/tests/it/pairing.rs similarity index 99% rename from crates/sync-core/tests/pairing.rs rename to crates/sync-core/tests/it/pairing.rs index c080c81..a4d2063 100644 --- a/crates/sync-core/tests/pairing.rs +++ b/crates/sync-core/tests/it/pairing.rs @@ -4,7 +4,7 @@ //! reconciliation scan, the real state store, and the mock server's record of //! what arrived and in which order. -mod common; +use super::common; use std::path::{Path, PathBuf}; use std::sync::Arc; diff --git a/crates/sync-core/tests/status.rs b/crates/sync-core/tests/it/status.rs similarity index 100% rename from crates/sync-core/tests/status.rs rename to crates/sync-core/tests/it/status.rs diff --git a/crates/sync-core/tests/store.rs b/crates/sync-core/tests/it/store.rs similarity index 100% rename from crates/sync-core/tests/store.rs rename to crates/sync-core/tests/it/store.rs diff --git a/crates/sync-core/tests/tag_backfill.rs b/crates/sync-core/tests/it/tag_backfill.rs similarity index 99% rename from crates/sync-core/tests/tag_backfill.rs rename to crates/sync-core/tests/it/tag_backfill.rs index 231ba83..36b2bbd 100644 --- a/crates/sync-core/tests/tag_backfill.rs +++ b/crates/sync-core/tests/it/tag_backfill.rs @@ -7,7 +7,7 @@ //! assert the requests on the wire as well as the progress the engine //! publishes, because the double is more forgiving than the real server. -mod common; +use super::common; use std::path::{Path, PathBuf}; use std::sync::Arc; diff --git a/crates/sync-core/tests/tags.rs b/crates/sync-core/tests/it/tags.rs similarity index 99% rename from crates/sync-core/tests/tags.rs rename to crates/sync-core/tests/it/tags.rs index 631da1d..7f14bfd 100644 --- a/crates/sync-core/tests/tags.rs +++ b/crates/sync-core/tests/it/tags.rs @@ -5,7 +5,7 @@ //! the tag the double holds on an asset, and the requests the client put on the //! wire, because the double is more forgiving than the real server. -mod common; +use super::common; use std::path::{Path, PathBuf}; use std::sync::Arc; diff --git a/crates/sync-core/tests/tracking.rs b/crates/sync-core/tests/it/tracking.rs similarity index 100% rename from crates/sync-core/tests/tracking.rs rename to crates/sync-core/tests/it/tracking.rs diff --git a/crates/sync-core/tests/upload.rs b/crates/sync-core/tests/it/upload.rs similarity index 100% rename from crates/sync-core/tests/upload.rs rename to crates/sync-core/tests/it/upload.rs diff --git a/docs/adr/0001-rust-gpui-kit-shell.md b/docs/adr/0001-rust-gpui-kit-shell.md index 4eda185..2c7ccbf 100644 --- a/docs/adr/0001-rust-gpui-kit-shell.md +++ b/docs/adr/0001-rust-gpui-kit-shell.md @@ -46,4 +46,4 @@ A card cannot host a real `SettingItem` on 0.8.1, so `crates/app/src/rows.rs::Ro One deliberate deviation, at `rows.rs` `Row::render`: the label column is `flex_1().min_w_0()` instead of the kit's `flex_1().max_w_3_5()` cap, which is `max_w(relative(0.6))` — 60%, not 3.5 rem (`gpui-pre-macros-0.3.10/src/styles.rs:1133`). A settings page caps a fixed-width field column; a card's field is a content-sized control, and the cap only shortens the help sentence beside it. The mirror also skips the help's `size_full` and the kit's 480 px stacking breakpoint (`settings.rs` passes it to every item), so a card row stays label-left at every width. -What a gpui-kit bump must re-check, because no test can see it: diff `gpui-component-/src/setting/item.rs:252-325` against `crates/app/src/rows.rs::Row::render` and re-run the row table in `crates/app/tests/logs_ui.rs` and `crates/app/tests/diagnostics.rs`. The suite asserts the rows' words and names, not their geometry. If a release makes `render_item` public, gives `SettingItem` a `Render` impl, or makes the group or page renderers public, delete the mirror and hand the item to the card; until then a bump that restyles the settings surface leaves these ten rows on 0.8.1's layout and nothing in `cargo test` will say so. +What a gpui-kit bump must re-check, because no test can see it: diff `gpui-component-/src/setting/item.rs:252-325` against `crates/app/src/rows.rs::Row::render` and re-run the row table in `crates/app/tests/it/logs_ui.rs` and `crates/app/tests/it/diagnostics.rs`. The suite asserts the rows' words and names, not their geometry. If a release makes `render_item` public, gives `SettingItem` a `Render` impl, or makes the group or page renderers public, delete the mirror and hand the item to the card; until then a bump that restyles the settings surface leaves these ten rows on 0.8.1's layout and nothing in `cargo test` will say so. diff --git a/docs/research/macos-support.md b/docs/research/macos-support.md index 50dd0ca..359209f 100644 --- a/docs/research/macos-support.md +++ b/docs/research/macos-support.md @@ -405,7 +405,7 @@ Current runners (2026-10-26): **`macos-26`** is arm64, **`macos-26-intel`** is I Recommended shape: - **Per-push CI:** one `macos-26` (arm64) job: `cargo clippy --workspace --all-targets - -- -D warnings` and `cargo test --workspace`. Add the Metal toolchain step first: + -- -D warnings` and `cargo nextest run --workspace`. Add the Metal toolchain step first: `xcrun metal --version || (xcodebuild -runFirstLaunch && xcodebuild -downloadComponent MetalToolchain)`. Do not add an Intel test job for v1; the code is platform-neutral and the Intel runner is on a clock. diff --git a/docs/theme-seam.md b/docs/theme-seam.md index 5b9019a..853cd91 100644 --- a/docs/theme-seam.md +++ b/docs/theme-seam.md @@ -6,7 +6,7 @@ keeps following the window's system appearance (`follow_system_theme`), which the shell calls when it opens the main window. The mapping and the apply step are covered by headless tests in -`crates/app/tests/dashboard.rs`; the appearance observer itself is shell +`crates/app/tests/it/dashboard.rs`; the appearance observer itself is shell wiring. ## Screenshots