From 3c96fdc9de1eb345a5039c64d9d273915492851b Mon Sep 17 00:00:00 2001 From: Chad Elliott Date: Mon, 26 Oct 2026 12:53:11 -0500 Subject: [PATCH] test(ui-egui): #3065 put the memory budget back as it was found, one changing test at a time settings_dialog_by_keyboard_and_clicks changes the process-wide memory budget through Settings > Performance (app.memoryBudget) and then reset it to default_budget(), undoing a budget set from outside (LIGHTCRAFT_MEMORY_MB, a harness cap), and its assertion on the global could race with another test changing it. A test-only BudgetGuard (crates/ui-egui/src/test_budget.rs) now takes a shared lock, so tests that change the budget run one at a time, and puts the budget back to exactly the value it found when dropped, also on a failed assertion (drop runs on unwind; a poisoned lock is taken anyway). The settings test holds it only for the few frames of the change, and also checks the session's own cache share, which only the command can change. Unit tests cover restoring the outside value (not the default), restoring on panic, and a failed holder not blocking the next. Co-Authored-By: Claude Opus 5.5 --- crates/ui-egui/src/headless/tests.rs | 27 +++++--- crates/ui-egui/src/lib.rs | 2 + crates/ui-egui/src/test_budget.rs | 93 ++++++++++++++++++++++++++++ 3 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 crates/ui-egui/src/test_budget.rs diff --git a/crates/ui-egui/src/headless/tests.rs b/crates/ui-egui/src/headless/tests.rs index 80d559efc..52f1c917b 100644 --- a/crates/ui-egui/src/headless/tests.rs +++ b/crates/ui-egui/src/headless/tests.rs @@ -4268,13 +4268,26 @@ fn settings_dialog_by_keyboard_and_clicks() { assert_eq!(h.app.session.cache_mb, 512); h.request("ui.clickWidget", json!({"id": "button:settingsPreview-3"}), t); assert_eq!(h.app.ui.settings.preview_limit, 3840); - h.request("ui.clickWidget", json!({"id": "button:settingsMemory-2"}), t); - assert_eq!(h.app.ui.settings.memory_mb, 1024); - h.step(); - assert_eq!(lightcraft_engine::memory::budget(), 1024 << 20, "applied through app.memoryBudget"); - h.request("ui.clickWidget", json!({"id": "button:settingsMemory-0"}), t); - h.step(); - assert_eq!(lightcraft_engine::memory::budget(), lightcraft_engine::memory::default_budget(), "back to automatic"); + { + // the memory budget is process-wide: this test's turn to change it, and it goes back + // to what it was (not the default) when the guard drops, also on a failed assertion + // (issue #3065). The change lasts these few frames. + use lightcraft_engine::memory::{budget, cache_share, default_budget}; + let _budget = crate::test_budget::BudgetGuard::hold(); + // the session's own share, set apart from the 1024 MB choice's: only the command + // changes it, whatever another test does to the process-wide budget + h.app.session.media.set_budget(cache_share(1024 << 20) + (1 << 20)); + h.request("ui.clickWidget", json!({"id": "button:settingsMemory-2"}), t); + assert_eq!(h.app.ui.settings.memory_mb, 1024); + h.step(); + assert_eq!(h.app.session.media.budget(), cache_share(1024 << 20), "the session's caches follow"); + assert_eq!(budget(), 1024 << 20, "applied through app.memoryBudget"); + h.request("ui.clickWidget", json!({"id": "button:settingsMemory-0"}), t); + h.step(); + let automatic = (default_budget() >> 20) << 20; + assert_eq!(h.app.session.media.budget(), cache_share(automatic), "back to automatic"); + assert_eq!(budget(), automatic, "back to automatic"); + } // Import: per-camera defaults h.request("ui.clickWidget", json!({"id": "button:settingsTab-import"}), t); h.request("ui.clickWidget", json!({"id": "check:settings.perCamera"}), t); diff --git a/crates/ui-egui/src/lib.rs b/crates/ui-egui/src/lib.rs index 8da88f9ae..3ab3f6bcd 100644 --- a/crates/ui-egui/src/lib.rs +++ b/crates/ui-egui/src/lib.rs @@ -39,6 +39,8 @@ pub mod theme; pub mod widgets; pub mod window_place; +#[cfg(test)] +mod test_budget; #[cfg(test)] mod tests; #[cfg(test)] diff --git a/crates/ui-egui/src/test_budget.rs b/crates/ui-egui/src/test_budget.rs new file mode 100644 index 000000000..1442cd1a0 --- /dev/null +++ b/crates/ui-egui/src/test_budget.rs @@ -0,0 +1,93 @@ +//! For tests that change the process-wide memory budget ([`lightcraft_engine::memory::budget`]). +//! +//! The budget is one number for the whole test process: the decode, export and GPU gates, every +//! new session's caches and the demo scene cache size from it, and other tests run at the same +//! time. A test that changes it (through `app.memoryBudget` or Settings ▸ Performance) holds a +//! [`BudgetGuard`] for as long as the change lasts. Tests that change the budget then run one at +//! a time, so one can't change it under another's assertion. The budget goes back to exactly what +//! it was before, also when the test fails: putting back the value found, not `default_budget()`, +//! keeps a budget set from outside (`LIGHTCRAFT_MEMORY_MB`, a test harness) (issue #3065). +//! +//! Tests that only read the budget don't take the lock; keep the change short. +//! The lock is per process, so per test binary, which is all the budget is shared by. + +use std::sync::{Mutex, MutexGuard, PoisonError}; + +use lightcraft_engine::memory; + +/// Taken by every test that changes the process-wide budget. +static CHANGING_BUDGET: Mutex<()> = Mutex::new(()); + +/// Held while a test changes the process-wide memory budget: the other tests that change it wait, +/// and the budget is put back as it was found when the guard drops. +pub(crate) struct BudgetGuard { + // fields drop in order: the budget is put back before the next test may take its turn + _restore: Restore, + _turn: MutexGuard<'static, ()>, +} + +impl BudgetGuard { + /// Wait for the other tests that change the budget, then remember the budget as it is now. + pub(crate) fn hold() -> BudgetGuard { + // poisoned: a test failed holding it, and its guard put the budget back on unwind + let turn = CHANGING_BUDGET.lock().unwrap_or_else(PoisonError::into_inner); + BudgetGuard { _restore: Restore::new(), _turn: turn } + } +} + +/// Puts the process-wide budget back to what it was when made. +struct Restore(usize); + +impl Restore { + fn new() -> Restore { + Restore(memory::budget()) + } +} + +impl Drop for Restore { + fn drop(&mut self) { + // doesn't panic; the value came from `budget()`, so it's within what `set_budget` keeps + memory::set_budget(self.0); + } +} + +mod tests { + use super::*; + + // The budget is put back as it was found, not to the default, also when the test fails + // (issue #3065) + #[test] + fn the_budget_is_put_back_as_it_was_found() { + let _turn = BudgetGuard::hold(); + // a budget set from outside the test, unlike the default + let outside = if memory::budget() == 700 << 20 { 600 << 20 } else { 700 << 20 }; + memory::set_budget(outside); + + { + let _r = Restore::new(); + memory::set_budget(1024 << 20); + assert_eq!(memory::budget(), 1024 << 20); + } + assert_eq!(memory::budget(), outside, "the budget found, not the default"); + + let failed = std::panic::catch_unwind(|| { + let _r = Restore::new(); + memory::set_budget(96 << 20); + panic!("an assertion fails while the budget is changed"); + }); + assert!(failed.is_err()); + assert_eq!(memory::budget(), outside, "put back on unwind too"); + } + + // A test that fails holding the lock doesn't make the next one fail + #[test] + fn a_failed_test_does_not_block_the_next() { + let failed = std::thread::spawn(|| { + let _g = BudgetGuard::hold(); + panic!("an assertion fails while holding the turn"); + }) + .join(); + assert!(failed.is_err()); + let _g = BudgetGuard::hold(); + } +}