From bf45023361819f593f7a9796d9df1affcd812d98 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 19 Mar 2025 17:51:42 -0400 Subject: [PATCH 1/6] Temporarily repeat the fs snapshot journey many times This is to smoke out nondeterministic failures on any platforms that have them, so as to gain greater confidence that the forthcoming fix is effective. The two major matrix job definitions in `ci.yml` are accordingly made so a failing job does not cancel the other jobs, so all jobs that would fail can get as far as to show their failures. This is likewise temporary. --- .github/workflows/ci.yml | 2 ++ gix-fs/tests/fs/snapshot.rs | 7 +++++++ 2 files changed, 9 insertions(+) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 852f361feb3..b704c97bd6f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -162,6 +162,7 @@ jobs: - macos-latest - ubuntu-latest - ubuntu-24.04-arm + fail-fast: false runs-on: ${{ matrix.os }} @@ -265,6 +266,7 @@ jobs: runner-arch: arm64 runner-os: ubuntu-24.04-arm host-triple: armv7-unknown-linux-gnueabihf + fail-fast: false runs-on: ${{ matrix.runner-os }} diff --git a/gix-fs/tests/fs/snapshot.rs b/gix-fs/tests/fs/snapshot.rs index d7f4ae91c58..a5480aa1e85 100644 --- a/gix-fs/tests/fs/snapshot.rs +++ b/gix-fs/tests/fs/snapshot.rs @@ -3,6 +3,13 @@ use std::path::Path; #[test] fn journey() -> Result<(), Box> { + for _ in 0..250 { + do_journey()?; + } + Ok(()) +} + +fn do_journey() -> Result<(), Box> { let tmp = tempfile::tempdir().unwrap(); if !has_nanosecond_times(tmp.path())? { return Ok(()); From d4a8d796222316708fc26421b47bbf66946d46be Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 19 Mar 2025 18:50:23 -0400 Subject: [PATCH 2/6] Improve filesystem timestamp granularity check This avoids false positives for the check of whether we can proceed to run the filesystem snapshot `journey` test, by making a hundred files about as fast as we can using ordinary non-parallelized techniques, then checking if all their timestamps are distinct. This carries a small risk of false negatives, but experiments on macOS, where the `journey` test is typically able to run, suggest that the rate of false negatives will be low. (False positive are also, in theory, possible, but should be extremely rare or never happen.) --- gix-fs/tests/fs/snapshot.rs | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/gix-fs/tests/fs/snapshot.rs b/gix-fs/tests/fs/snapshot.rs index a5480aa1e85..5dd5a322a3b 100644 --- a/gix-fs/tests/fs/snapshot.rs +++ b/gix-fs/tests/fs/snapshot.rs @@ -11,7 +11,7 @@ fn journey() -> Result<(), Box> { fn do_journey() -> Result<(), Box> { let tmp = tempfile::tempdir().unwrap(); - if !has_nanosecond_times(tmp.path())? { + if !has_granular_times(tmp.path())? { return Ok(()); } @@ -48,17 +48,21 @@ fn do_journey() -> Result<(), Box> { Ok(()) } -fn has_nanosecond_times(root: &Path) -> std::io::Result { - let test_file = root.join("nanosecond-test"); +fn has_granular_times(root: &Path) -> std::io::Result { + let n = 100; - std::fs::write(&test_file, "a")?; - let first_time = test_file.metadata()?.modified()?; - - std::fs::write(&test_file, "b")?; - let second_time = test_file.metadata()?.modified()?; + let names: Vec<_> = (0..n).map(|i| format!("{i:03}")).collect(); + for name in &names { + std::fs::write(root.join(name), name)?; + } + let mut times = Vec::new(); + for name in names { + times.push(root.join(name).symlink_metadata()?.modified()?); + } + times.sort(); + times.dedup(); - Ok(second_time.duration_since(first_time).is_ok_and(|d| - // This can be falsely false if a filesystem would be ridiculously fast, - // which means a test won't run even though it could. But that's OK, and unlikely. - d.subsec_nanos() != 0)) + // This could be wrongly false if a filesystem has very precise timings yet is ridiculously + // fast. Then the `journey` test wouldn't run, though it could. But that's OK, and unlikely. + Ok(times.len() == n) } From d1b0d1c231eccc8862dba925f4e5eb3f5f0bf8d3 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 19 Mar 2025 20:11:49 -0400 Subject: [PATCH 3/6] Assert that macOS on CI is high granularity This raises the risk of a failure when there is no bug, but hopefully only slightly, and it's a simple way to verify that the snapshot `journey` test really is running fully in at least one regularlly tested platform. --- Cargo.lock | 1 + gix-fs/Cargo.toml | 3 ++- gix-fs/tests/fs/snapshot.rs | 4 ++++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index a0f54606ae0..6fdc84aeb21 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1903,6 +1903,7 @@ dependencies = [ "fastrand", "gix-features 0.40.0", "gix-utils 0.1.14", + "is_ci", "serde", "tempfile", ] diff --git a/gix-fs/Cargo.toml b/gix-fs/Cargo.toml index ba396c5f25a..a11252b2c52 100644 --- a/gix-fs/Cargo.toml +++ b/gix-fs/Cargo.toml @@ -27,5 +27,6 @@ serde = { version = "1.0.114", optional = true, default-features = false, featur fastrand = { version = "2.1.0", default-features = false, features = ["std"] } [dev-dependencies] -tempfile = "3.5.0" crossbeam-channel = "0.5.0" +is_ci = "1.1.1" +tempfile = "3.5.0" diff --git a/gix-fs/tests/fs/snapshot.rs b/gix-fs/tests/fs/snapshot.rs index 5dd5a322a3b..435b77ac45c 100644 --- a/gix-fs/tests/fs/snapshot.rs +++ b/gix-fs/tests/fs/snapshot.rs @@ -64,5 +64,9 @@ fn has_granular_times(root: &Path) -> std::io::Result { // This could be wrongly false if a filesystem has very precise timings yet is ridiculously // fast. Then the `journey` test wouldn't run, though it could. But that's OK, and unlikely. + // However, for now, on CI, on macOS only, we assert the expectation of high granularity. + if cfg!(target_os = "macos") && is_ci::cached() { + assert_eq!(times.len(), n); + } Ok(times.len() == n) } From b68ba95d598416b8881bf19942378cea227d9f92 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 19 Mar 2025 21:43:33 -0400 Subject: [PATCH 4/6] Improve `has_granular_times()` as suggested in review - Decrease n=100 to n=50, which remains decisively enough on all platforms being tested (including Windows). - Slightly streamline iteration by cloning the first iterator so its values need not be collected into a `Vec`. - Make it so that if the CI+macOS high-granularity assertion ever fails, the message explains the significance of the failing `assert_eq!` comparison. (This condenses an added comment and makes it an assertion message instead.) Suggested-by: Sebastian Thiel --- gix-fs/tests/fs/snapshot.rs | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/gix-fs/tests/fs/snapshot.rs b/gix-fs/tests/fs/snapshot.rs index 435b77ac45c..6ed55f9965a 100644 --- a/gix-fs/tests/fs/snapshot.rs +++ b/gix-fs/tests/fs/snapshot.rs @@ -49,11 +49,11 @@ fn do_journey() -> Result<(), Box> { } fn has_granular_times(root: &Path) -> std::io::Result { - let n = 100; + let n = 50; - let names: Vec<_> = (0..n).map(|i| format!("{i:03}")).collect(); - for name in &names { - std::fs::write(root.join(name), name)?; + let names = (0..n).map(|i| format!("{i:03}")); + for name in names.clone() { + std::fs::write(root.join(&name), name)?; } let mut times = Vec::new(); for name in names { @@ -64,9 +64,12 @@ fn has_granular_times(root: &Path) -> std::io::Result { // This could be wrongly false if a filesystem has very precise timings yet is ridiculously // fast. Then the `journey` test wouldn't run, though it could. But that's OK, and unlikely. - // However, for now, on CI, on macOS only, we assert the expectation of high granularity. if cfg!(target_os = "macos") && is_ci::cached() { - assert_eq!(times.len(), n); + assert_eq!( + times.len(), + n, + "should have very granular timestamps at least on macOS on CI" + ); } Ok(times.len() == n) } From 37aeb2f769773417ad9049f39b5de8a58d3c1b53 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 19 Mar 2025 22:02:31 -0400 Subject: [PATCH 5/6] Apply further review suggestion Co-authored-by: Sebastian Thiel --- gix-fs/tests/fs/snapshot.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/gix-fs/tests/fs/snapshot.rs b/gix-fs/tests/fs/snapshot.rs index 6ed55f9965a..b85e046d8eb 100644 --- a/gix-fs/tests/fs/snapshot.rs +++ b/gix-fs/tests/fs/snapshot.rs @@ -51,13 +51,13 @@ fn do_journey() -> Result<(), Box> { fn has_granular_times(root: &Path) -> std::io::Result { let n = 50; - let names = (0..n).map(|i| format!("{i:03}")); - for name in names.clone() { - std::fs::write(root.join(&name), name)?; + let paths = (0..n).map(|i| root.join(format!("{i:03}"))); + for (index, path) in paths.clone().enumerate() { + std::fs::write(&path, index.to_string().as_bytes())?; } let mut times = Vec::new(); - for name in names { - times.push(root.join(name).symlink_metadata()?.modified()?); + for path in paths { + times.push(path.symlink_metadata()?.modified()?); } times.sort(); times.dedup(); From a5c58dd18ac291f3821426fc0f8d1274f8cc6b31 Mon Sep 17 00:00:00 2001 From: Eliah Kagan Date: Wed, 19 Mar 2025 19:22:49 -0400 Subject: [PATCH 6/6] Revert "Temporarily repeat the fs snapshot journey many times" This reverts commit bf45023361819f593f7a9796d9df1affcd812d98. --- .github/workflows/ci.yml | 2 -- gix-fs/tests/fs/snapshot.rs | 7 ------- 2 files changed, 9 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index b704c97bd6f..852f361feb3 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -162,7 +162,6 @@ jobs: - macos-latest - ubuntu-latest - ubuntu-24.04-arm - fail-fast: false runs-on: ${{ matrix.os }} @@ -266,7 +265,6 @@ jobs: runner-arch: arm64 runner-os: ubuntu-24.04-arm host-triple: armv7-unknown-linux-gnueabihf - fail-fast: false runs-on: ${{ matrix.runner-os }} diff --git a/gix-fs/tests/fs/snapshot.rs b/gix-fs/tests/fs/snapshot.rs index b85e046d8eb..00e0ad2d76e 100644 --- a/gix-fs/tests/fs/snapshot.rs +++ b/gix-fs/tests/fs/snapshot.rs @@ -3,13 +3,6 @@ use std::path::Path; #[test] fn journey() -> Result<(), Box> { - for _ in 0..250 { - do_journey()?; - } - Ok(()) -} - -fn do_journey() -> Result<(), Box> { let tmp = tempfile::tempdir().unwrap(); if !has_granular_times(tmp.path())? { return Ok(());