From 992d6c29cc73ec4e9fcd690bc1e36a84c82ea355 Mon Sep 17 00:00:00 2001 From: elementalvoid Date: Fri, 6 Nov 2026 07:05:48 +0000 Subject: [PATCH 1/6] perf(test): link one test binary per crate 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. --- crates/app/src/logging.rs | 4 +-- crates/app/src/shell.rs | 2 +- crates/app/tests/{ => it}/common/mod.rs | 0 crates/app/tests/{ => it}/dashboard.rs | 2 +- crates/app/tests/{ => it}/diagnostics.rs | 2 +- crates/app/tests/{ => it}/engine_model.rs | 2 +- crates/app/tests/{ => it}/failed_ui.rs | 0 crates/app/tests/{ => it}/folder_detail.rs | 2 +- crates/app/tests/{ => it}/keyring.rs | 0 crates/app/tests/{ => it}/log_redaction.rs | 2 +- crates/app/tests/{ => it}/logging.rs | 0 crates/app/tests/{ => it}/login.rs | 0 crates/app/tests/{ => it}/logs_ui.rs | 2 +- crates/app/tests/it/main.rs | 26 +++++++++++++++++++ crates/app/tests/{ => it}/notices.rs | 2 +- crates/app/tests/{ => it}/packaging.rs | 0 crates/app/tests/{ => it}/review_ui.rs | 0 crates/app/tests/{ => it}/screenshot_seed.rs | 0 crates/app/tests/{ => it}/settings.rs | 0 crates/app/tests/{ => it}/settings_ui.rs | 2 +- crates/app/tests/{ => it}/shell.rs | 2 +- crates/app/tests/{ => it}/upload_ui.rs | 2 +- crates/app/tests/{ => it}/window.rs | 0 crates/immich-client/tests/{ => it}/albums.rs | 0 crates/immich-client/tests/{ => it}/client.rs | 0 crates/immich-client/tests/{ => it}/limits.rs | 0 crates/immich-client/tests/it/main.rs | 12 +++++++++ crates/immich-client/tests/{ => it}/tags.rs | 0 crates/immich-client/tests/{ => it}/trash.rs | 0 crates/immich-client/tests/{ => it}/upload.rs | 0 crates/mock-immich/tests/{ => it}/about.rs | 0 crates/mock-immich/tests/{ => it}/albums.rs | 0 crates/mock-immich/tests/{ => it}/api_keys.rs | 0 .../mock-immich/tests/{ => it}/assets_ops.rs | 0 crates/mock-immich/tests/{ => it}/auth.rs | 0 crates/mock-immich/tests/{ => it}/faults.rs | 0 crates/mock-immich/tests/it/main.rs | 15 +++++++++++ crates/mock-immich/tests/{ => it}/sidecar.rs | 0 crates/mock-immich/tests/{ => it}/tags.rs | 0 crates/mock-immich/tests/{ => it}/upload.rs | 0 crates/sync-core/src/backfill.rs | 2 +- .../sync-core/tests/{ => it}/album_title.rs | 2 +- crates/sync-core/tests/{ => it}/common/mod.rs | 0 crates/sync-core/tests/{ => it}/connection.rs | 0 .../tests/{ => it}/dependency_rule.rs | 4 +-- crates/sync-core/tests/{ => it}/engine.rs | 2 +- crates/sync-core/tests/{ => it}/hash.rs | 0 crates/sync-core/tests/{ => it}/limits.rs | 2 +- .../sync-core/tests/{ => it}/log_redaction.rs | 0 crates/sync-core/tests/it/main.rs | 23 ++++++++++++++++ crates/sync-core/tests/{ => it}/meter.rs | 2 +- crates/sync-core/tests/{ => it}/missing.rs | 0 crates/sync-core/tests/{ => it}/pairing.rs | 2 +- crates/sync-core/tests/{ => it}/status.rs | 0 crates/sync-core/tests/{ => it}/store.rs | 0 .../sync-core/tests/{ => it}/tag_backfill.rs | 2 +- crates/sync-core/tests/{ => it}/tags.rs | 2 +- crates/sync-core/tests/{ => it}/tracking.rs | 0 crates/sync-core/tests/{ => it}/upload.rs | 0 docs/adr/0001-rust-gpui-kit-shell.md | 2 +- docs/theme-seam.md | 2 +- 61 files changed, 101 insertions(+), 25 deletions(-) rename crates/app/tests/{ => it}/common/mod.rs (100%) rename crates/app/tests/{ => it}/dashboard.rs (99%) rename crates/app/tests/{ => it}/diagnostics.rs (99%) rename crates/app/tests/{ => it}/engine_model.rs (99%) rename crates/app/tests/{ => it}/failed_ui.rs (100%) rename crates/app/tests/{ => it}/folder_detail.rs (99%) rename crates/app/tests/{ => it}/keyring.rs (100%) rename crates/app/tests/{ => it}/log_redaction.rs (99%) rename crates/app/tests/{ => it}/logging.rs (100%) rename crates/app/tests/{ => it}/login.rs (100%) rename crates/app/tests/{ => it}/logs_ui.rs (99%) create mode 100644 crates/app/tests/it/main.rs rename crates/app/tests/{ => it}/notices.rs (99%) rename crates/app/tests/{ => it}/packaging.rs (100%) rename crates/app/tests/{ => it}/review_ui.rs (100%) rename crates/app/tests/{ => it}/screenshot_seed.rs (100%) rename crates/app/tests/{ => it}/settings.rs (100%) rename crates/app/tests/{ => it}/settings_ui.rs (99%) rename crates/app/tests/{ => it}/shell.rs (99%) rename crates/app/tests/{ => it}/upload_ui.rs (99%) rename crates/app/tests/{ => it}/window.rs (100%) rename crates/immich-client/tests/{ => it}/albums.rs (100%) rename crates/immich-client/tests/{ => it}/client.rs (100%) rename crates/immich-client/tests/{ => it}/limits.rs (100%) create mode 100644 crates/immich-client/tests/it/main.rs rename crates/immich-client/tests/{ => it}/tags.rs (100%) rename crates/immich-client/tests/{ => it}/trash.rs (100%) rename crates/immich-client/tests/{ => it}/upload.rs (100%) rename crates/mock-immich/tests/{ => it}/about.rs (100%) rename crates/mock-immich/tests/{ => it}/albums.rs (100%) rename crates/mock-immich/tests/{ => it}/api_keys.rs (100%) rename crates/mock-immich/tests/{ => it}/assets_ops.rs (100%) rename crates/mock-immich/tests/{ => it}/auth.rs (100%) rename crates/mock-immich/tests/{ => it}/faults.rs (100%) create mode 100644 crates/mock-immich/tests/it/main.rs rename crates/mock-immich/tests/{ => it}/sidecar.rs (100%) rename crates/mock-immich/tests/{ => it}/tags.rs (100%) rename crates/mock-immich/tests/{ => it}/upload.rs (100%) rename crates/sync-core/tests/{ => it}/album_title.rs (99%) rename crates/sync-core/tests/{ => it}/common/mod.rs (100%) rename crates/sync-core/tests/{ => it}/connection.rs (100%) rename crates/sync-core/tests/{ => it}/dependency_rule.rs (95%) rename crates/sync-core/tests/{ => it}/engine.rs (99%) rename crates/sync-core/tests/{ => it}/hash.rs (100%) rename crates/sync-core/tests/{ => it}/limits.rs (99%) rename crates/sync-core/tests/{ => it}/log_redaction.rs (100%) create mode 100644 crates/sync-core/tests/it/main.rs rename crates/sync-core/tests/{ => it}/meter.rs (99%) rename crates/sync-core/tests/{ => it}/missing.rs (100%) rename crates/sync-core/tests/{ => it}/pairing.rs (99%) rename crates/sync-core/tests/{ => it}/status.rs (100%) rename crates/sync-core/tests/{ => it}/store.rs (100%) rename crates/sync-core/tests/{ => it}/tag_backfill.rs (99%) rename crates/sync-core/tests/{ => it}/tags.rs (99%) rename crates/sync-core/tests/{ => it}/tracking.rs (100%) rename crates/sync-core/tests/{ => it}/upload.rs (100%) 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..42d8495 100644 --- a/crates/app/tests/shell.rs +++ b/crates/app/tests/it/shell.rs @@ -4,7 +4,7 @@ //! cannot observe, so those are checked by the probe and the GUI smoke job. //! 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/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 100% rename from crates/immich-client/tests/limits.rs rename to crates/immich-client/tests/it/limits.rs 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/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/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 From 7b0bcb73a33d4bc64e6bf4a52b5b54dfdb8b0028 Mon Sep 17 00:00:00 2001 From: elementalvoid Date: Fri, 6 Nov 2026 07:06:01 +0000 Subject: [PATCH 2/6] fix(immich-client): measure the bandwidth bucket from one instant `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. --- crates/immich-client/src/bandwidth.rs | 60 ++++++++++++++++++++++----- 1 file changed, 49 insertions(+), 11 deletions(-) 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:?}" From 29a530e8a921ee0ebe08fc96c47b1f0e953c4881 Mon Sep 17 00:00:00 2001 From: elementalvoid Date: Fri, 6 Nov 2026 07:06:01 +0000 Subject: [PATCH 3/6] chore(deps): drop four dead dependencies from mock-immich `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.) --- Cargo.lock | 1 - crates/mock-immich/Cargo.toml | 8 ++++---- 2 files changed, 4 insertions(+), 5 deletions(-) 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/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"] } From f3704dd2e1e7354bda3228a55de5f142474238de Mon Sep 17 00:00:00 2001 From: elementalvoid Date: Fri, 6 Nov 2026 07:06:15 +0000 Subject: [PATCH 4/6] perf(ci): run tests under nextest, stop busting the cache on version 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. --- .github/workflows/ci.yml | 62 ++++++++++++++++++++++++++-------- CONTRIBUTING.md | 26 ++++++++++---- docs/research/macos-support.md | 2 +- 3 files changed, 68 insertions(+), 22 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 84c6bdf..2336a2b 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 @@ -233,7 +255,7 @@ jobs: # 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') }} + prefix-key: v1-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.toml') }} add-rust-environment-hash-key: "false" save-if: "false" cache-on-failure: "true" @@ -251,7 +273,10 @@ jobs: 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="" + ICD="" + for candidate in /usr/share/vulkan/icd.d/lvp_icd*.json; do + [ -e "$candidate" ] && { ICD=$candidate; break; } + done if [ -z "$ICD" ]; then echo "no lavapipe ICD file under /usr/share/vulkan/icd.d" >&2 exit 1 @@ -261,6 +286,9 @@ jobs: export VK_DRIVER_FILES="$ICD" export LIBGL_ALWAYS_SOFTWARE=1 unset WAYLAND_DISPLAY + # The single-quoted argument below is this script's inner script: its + # $PWD, $APP and $LOG_DIR expand inside the new bash, not here. + # shellcheck disable=SC2016 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 @@ -318,7 +346,11 @@ jobs: 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) + newest="" + for candidate in "$LOG_DIR"/immich-sync.log*; do + [ -e "$candidate" ] || continue + if [ -z "$newest" ] || [ "$candidate" -nt "$newest" ]; then newest=$candidate; fi + done if [ -n "$newest" ]; then echo "--- app log: $newest" cat "$newest" diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index cdfdc6a..11a43af 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -65,13 +65,21 @@ 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 ``` +`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, and +60s of it was one real-time bandwidth binary running alone. nextest takes it to +about 60s. 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`. + 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. +headless GPUI tests green; the `*_ui` modules under `crates/app/tests/it/` cover +the surfaces. `--all-features` is invalid here: the three `gpui-tray` backends are mutually exclusive. @@ -81,12 +89,18 @@ 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. + ## Where the design lives | What | Where | @@ -100,7 +114,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 +124,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/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. From 9acb55f176920696ccfb4664e28b76d0bd141ba0 Mon Sep 17 00:00:00 2001 From: elementalvoid Date: Fri, 6 Nov 2026 07:49:36 +0000 Subject: [PATCH 5/6] test(immich-client): run the stall guard on an injected window 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. --- CONTRIBUTING.md | 17 + crates/immich-client/src/client.rs | 399 +++++++++++++----------- crates/immich-client/tests/it/limits.rs | 56 ++-- 3 files changed, 269 insertions(+), 203 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 11a43af..f8cd53b 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -101,6 +101,23 @@ reads it once: `BandwidthLimit`'s unit tests pass one `Instant` to `set_idle_at` 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 | 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/it/limits.rs b/crates/immich-client/tests/it/limits.rs index a761381..f04c971 100644 --- a/crates/immich-client/tests/it/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:?}" ); } From 33631fee66da872f3be95586d83cb40a9d066ef5 Mon Sep 17 00:00:00 2001 From: elementalvoid Date: Sat, 7 Nov 2026 03:51:24 +0000 Subject: [PATCH 6/6] ci: drop the Linux GUI smoke job 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. --- .github/workflows/ci.yml | 134 ----------------------------------- CONTRIBUTING.md | 22 +++--- crates/app/tests/it/shell.rs | 3 +- 3 files changed, 14 insertions(+), 145 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 2336a2b..b3ce5cd 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -236,137 +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: v1-rust-${{ hashFiles('Cargo.lock', 'rust-toolchain.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="" - for candidate in /usr/share/vulkan/icd.d/lvp_icd*.json; do - [ -e "$candidate" ] && { ICD=$candidate; break; } - done - 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 - # The single-quoted argument below is this script's inner script: its - # $PWD, $APP and $LOG_DIR expand inside the new bash, not here. - # shellcheck disable=SC2016 - 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="" - for candidate in "$LOG_DIR"/immich-sync.log*; do - [ -e "$candidate" ] || continue - if [ -z "$newest" ] || [ "$candidate" -nt "$newest" ]; then newest=$candidate; fi - done - 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 f8cd53b..074a901 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -70,16 +70,18 @@ cargo nextest run --workspace `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, and -60s of it was one real-time bandwidth binary running alone. nextest takes it to -about 60s. 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`. - -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; the `*_ui` modules under `crates/app/tests/it/` cover -the surfaces. +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. diff --git a/crates/app/tests/it/shell.rs b/crates/app/tests/it/shell.rs index 42d8495..aca482b 100644 --- a/crates/app/tests/it/shell.rs +++ b/crates/app/tests/it/shell.rs @@ -1,7 +1,8 @@ //! 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. use super::common;