From 5a0e6be85209c15ea5e8bfabab8152f182e81cfe Mon Sep 17 00:00:00 2001 From: bflatastic Date: Mon, 5 Oct 2026 05:36:57 +0700 Subject: [PATCH] M3.52: stop the eyedropper image-sampling test from flaking The engine unit tests share one process, and two colour-management tests switched the process-wide Color Settings (the working RGB space to the wide gamut profile, Black Point Compensation off) for a moment. Any test that showed or rendered a colour at that moment saw it through the wrong profile: clicking_an_image_samples_its_pixel read #008181 instead of #008080. - Move the two tests that change the Color Settings to their own test binary (crates/engine/tests/color_settings.rs), so no unit test can see non-default settings. - The image-sampling test compares the stored colour, not its display hex. - every_command_survives_empty_params toggles the process-wide proof view (Proof Colors, Overprint Preview, Separations), so it now takes the colour-management test lock like the tests that read that view. Co-Authored-By: Claude Opus 5.5 --- crates/engine/src/tests.rs | 2 + crates/engine/src/tests_colormgmt.rs | 40 ++------------------ crates/engine/src/tests_eyedropper.rs | 5 ++- crates/engine/tests/color_settings.rs | 54 +++++++++++++++++++++++++++ 4 files changed, 62 insertions(+), 39 deletions(-) create mode 100644 crates/engine/tests/color_settings.rs diff --git a/crates/engine/src/tests.rs b/crates/engine/src/tests.rs index 7425fdd7..554bca3f 100644 --- a/crates/engine/src/tests.rs +++ b/crates/engine/src/tests.rs @@ -295,6 +295,8 @@ fn every_command_has_unique_id_and_doc() { #[test] fn every_command_survives_empty_params() { // Robustness: no command may panic on {} (with and without a selection). + // The view toggles among them flip the process-wide proof view. + let _proof_view = crate::tests_colormgmt::GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); for sel in [false, true] { for c in command_specs() { let mut s = session(); diff --git a/crates/engine/src/tests_colormgmt.rs b/crates/engine/src/tests_colormgmt.rs index 076b4ca1..0bc4e2d2 100644 --- a/crates/engine/src/tests_colormgmt.rs +++ b/crates/engine/src/tests_colormgmt.rs @@ -14,6 +14,9 @@ use vectorcraft_render::{RenderOptions, Renderer}; use super::*; /// Every test here reads or changes the process-wide proof view or colour settings: one at a time. +/// Tests that switch the Color Settings away from the defaults live in `tests/color_settings.rs` +/// (their own process): every colour shown or rendered goes through them, so no lock could cover +/// all the tests that would see the change. pub(crate) static GLOBAL: Mutex<()> = Mutex::new(()); fn session() -> Session { @@ -158,22 +161,6 @@ fn assign_profile_is_stored_and_undoable() { assert_eq!(cmd::colormgmt::doc_profiles(&s.doc().unwrap().doc), (None, None)); } -#[test] -fn color_settings_lists_profiles_and_validates() { - let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); - let mut s = session(); - let r = s.execute("edit.colorSettings", &json!({})).unwrap(); - let names: Vec<&str> = r["profiles"].as_array().unwrap().iter().map(|p| p["name"].as_str().unwrap()).collect(); - assert!(names.contains(&cms::GENERIC_CMYK) && names.contains(&cms::SRGB) && names.contains(&cms::DEVICE_CMYK)); - assert!(s.execute("edit.colorSettings", &json!({"cmyk": "Missing.icc"})).is_err()); - assert_eq!(cms::active_settings().cmyk, r["cmyk"].as_str().unwrap(), "failed change leaves settings alone"); - let before = r["bpc"].as_bool().unwrap(); - let r2 = s.execute("edit.colorSettings", &json!({"bpc": !before})).unwrap(); - assert_eq!(r2["bpc"], !before); - s.execute("edit.colorSettings", &json!({"bpc": before})).unwrap(); - assert!(s.execute("color.loadProfile", &json!({"path": "/nonexistent/x.icc"})).is_err()); -} - #[test] fn proof_colors_simulate_press_on_screen() { let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); @@ -391,24 +378,3 @@ fn intents_reach_the_document_conversion() { assert_ne!(first(&a), first(&b)); let _ = Intent::Perceptual; } - -#[test] -fn legacy_profile_name_resolves_in_commands_and_files() { - let _g = GLOBAL.lock().unwrap_or_else(|e| e.into_inner()); - let (old, _) = cms::LEGACY_NAMES[0]; - let mut s = session(); - let before = cms::active_settings(); - let r = s.execute("edit.colorSettings", &json!({"rgb": old})).unwrap(); - assert_eq!(r["rgb"], cms::WIDE_GAMUT_RGB); - assert_eq!(cms::active_settings().rgb, cms::WIDE_GAMUT_RGB); - let names: Vec<&str> = r["profiles"].as_array().unwrap().iter().map(|p| p["name"].as_str().unwrap()).collect(); - assert!(names.contains(&cms::WIDE_GAMUT_RGB) && !names.contains(&old)); - let r = s.execute("edit.assignProfile", &json!({"rgb": old})).unwrap(); - assert_eq!(r["rgb"], cms::WIDE_GAMUT_RGB, "stored under the current name"); - // A document saved by an older version names the profile the old way. - let mut d = (*s.doc().unwrap().doc).clone(); - d.unknown.insert(cmd::colormgmt::PROFILES_KEY.into(), json!({"rgb": old, "cmyk": null})); - let back = vectorcraft_format::load(&vectorcraft_format::save(&d, false)).unwrap(); - assert_eq!(cmd::colormgmt::doc_profiles(&back), (Some(cms::WIDE_GAMUT_RGB.to_string()), None)); - cms::set_active(&before).unwrap(); -} diff --git a/crates/engine/src/tests_eyedropper.rs b/crates/engine/src/tests_eyedropper.rs index 2a4c9b42..fb5e51f8 100644 --- a/crates/engine/src/tests_eyedropper.rs +++ b/crates/engine/src/tests_eyedropper.rs @@ -163,10 +163,11 @@ fn clicking_an_image_samples_its_pixel() { .unwrap(); let dst = rect(&mut s, 10.0); click(&mut s, 450.0, 50.0, Mods::default()); - assert_eq!(node(&s, dst).appearance.fill_paint().color().unwrap().to_hex(), "#008080"); + // Compared as stored (not as displayed through the working RGB space). + assert_eq!(node(&s, dst).appearance.fill_paint().color(), Some(Color::rgb8(0, 128, 128))); s.execute("eyedropper.setOptions", &json!({"sampleSize": 5})).unwrap(); click(&mut s, 350.0, 50.0, Mods::default()); - assert_eq!(node(&s, dst).appearance.fill_paint().color().unwrap().to_hex(), "#808040", "5 x 5 average of both pixels"); + assert_eq!(node(&s, dst).appearance.fill_paint().color(), Some(Color::rgb8(128, 128, 64)), "5 x 5 average of both pixels"); } #[test] diff --git a/crates/engine/tests/color_settings.rs b/crates/engine/tests/color_settings.rs new file mode 100644 index 00000000..0db5347a --- /dev/null +++ b/crates/engine/tests/color_settings.rs @@ -0,0 +1,54 @@ +//! Edit → Color Settings and Assign Profile with non-default profiles. The Color Settings are +//! process-wide and every colour shown or rendered goes through them, so these tests run in their +//! own test binary (process) instead of next to the engine's unit tests, which would see the +//! switched working spaces mid-test; here they take turns. + +use std::sync::{Mutex, MutexGuard}; + +use serde_json::json; +use vectorcraft_color::cms; +use vectorcraft_engine::cmd::colormgmt::{PROFILES_KEY, doc_profiles}; +use vectorcraft_testkit::fixtures::session; + +/// The active Color Settings: one test at a time. +fn settings() -> MutexGuard<'static, ()> { + static SETTINGS: Mutex<()> = Mutex::new(()); + SETTINGS.lock().unwrap_or_else(|e| e.into_inner()) +} + +#[test] +fn color_settings_lists_profiles_and_validates() { + let _g = settings(); + let mut s = session(); + let r = s.execute("edit.colorSettings", &json!({})).unwrap(); + let names: Vec<&str> = r["profiles"].as_array().unwrap().iter().map(|p| p["name"].as_str().unwrap()).collect(); + assert!(names.contains(&cms::GENERIC_CMYK) && names.contains(&cms::SRGB) && names.contains(&cms::DEVICE_CMYK)); + assert!(s.execute("edit.colorSettings", &json!({"cmyk": "Missing.icc"})).is_err()); + assert_eq!(cms::active_settings().cmyk, r["cmyk"].as_str().unwrap(), "failed change leaves settings alone"); + let before = r["bpc"].as_bool().unwrap(); + let r2 = s.execute("edit.colorSettings", &json!({"bpc": !before})).unwrap(); + assert_eq!(r2["bpc"], !before); + s.execute("edit.colorSettings", &json!({"bpc": before})).unwrap(); + assert!(s.execute("color.loadProfile", &json!({"path": "/nonexistent/x.icc"})).is_err()); +} + +#[test] +fn legacy_profile_name_resolves_in_commands_and_files() { + let _g = settings(); + let (old, _) = cms::LEGACY_NAMES[0]; + let mut s = session(); + let before = cms::active_settings(); + let r = s.execute("edit.colorSettings", &json!({"rgb": old})).unwrap(); + assert_eq!(r["rgb"], cms::WIDE_GAMUT_RGB); + assert_eq!(cms::active_settings().rgb, cms::WIDE_GAMUT_RGB); + let names: Vec<&str> = r["profiles"].as_array().unwrap().iter().map(|p| p["name"].as_str().unwrap()).collect(); + assert!(names.contains(&cms::WIDE_GAMUT_RGB) && !names.contains(&old)); + let r = s.execute("edit.assignProfile", &json!({"rgb": old})).unwrap(); + assert_eq!(r["rgb"], cms::WIDE_GAMUT_RGB, "stored under the current name"); + // A document saved by an older version names the profile the old way. + let mut d = (*s.doc().unwrap().doc).clone(); + d.unknown.insert(PROFILES_KEY.into(), json!({"rgb": old, "cmyk": null})); + let back = vectorcraft_format::load(&vectorcraft_format::save(&d, false)).unwrap(); + assert_eq!(doc_profiles(&back), (Some(cms::WIDE_GAMUT_RGB.to_string()), None)); + cms::set_active(&before).unwrap(); +}