From 9cabe478e4807546e213acc21506673c9506aa9c Mon Sep 17 00:00:00 2001
From: Yasunobu <42543015+P4suta@users.noreply.github.com>
Date: Sat, 19 Sep 2026 20:34:47 +0900
Subject: [PATCH] fix: run the suite on the systems that get a binary

`cargo test` ran on Linux alone, while `release.yml` ships
`x86_64-pc-windows-msvc`. What Windows CI measured was that the crate
builds and prints its version.

That is worse than measuring nothing. The job went green, so the run
went green, and a green run reads as *Windows passes* -- the ground for
it appears nowhere in the output. A claim taken from how the measuring
was done rather than from what was measured.

The first thing it finds was already known to one person who had run the
suite by hand. `a_first_segment_that_reads_as_a_drive_letter_is_
disambiguated` asked a question with two right answers: `c:/a.rs` names
a directory called `c:` in a POSIX checkout and the root of a drive on
Windows, `std::path` says so, and the SARIF location follows -- under
`%SRCROOT%` with a `./` on one system and under no base on the other.
The implementation was right on both. The test held one system's answer,
and nothing had ever asked the other.

It now asks each. A second case pins `under_source_root` itself, because
both halves of the first would pass if that function simply stopped
answering.

The step is skipped on Linux, where the `rust` job already runs it with
the two switches that turn a skip into a failure. Neither may be set
here: they are read with `is_some`, so `OCOMMENT_REQUIRE_FORMATTERS:
"0"` would demand the formatters rather than excuse them.

`sync_parent` is split by system rather than guarding its body, so the
Windows build stops warning about a parameter the arm that does nothing
cannot use. Taken from an abandoned branch.
---
 .github/workflows/ci.yml    | 11 ++++++++
 CHANGELOG.md                | 23 +++++++++++++++++
 rust/ocomment/src/atomic.rs | 20 +++++++++++----
 rust/ocomment/src/output.rs | 50 ++++++++++++++++++++++++++++++++++---
 4 files changed, 95 insertions(+), 9 deletions(-)

diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml
index 333e9f2..133e1bf 100644
--- a/.github/workflows/ci.yml
+++ b/.github/workflows/ci.yml
@@ -239,6 +239,17 @@ jobs:
         if: runner.os == 'Windows'
         shell: pwsh
         run: '& rust/target/release/ocomment.exe --version'
+      # NOTE: The suite, on the systems this repository ships a binary for.
+      # NOTE: Until now `cargo test` ran on Linux alone while `release.yml`
+      # NOTE: shipped x86_64-pc-windows-msvc: what Windows measured was that it
+      # NOTE: builds and prints its version, and because this job went green
+      # NOTE: the whole run did, reading as "Windows passes". Skipped on Linux,
+      # NOTE: where the `rust` job runs it with the switches that turn a skip
+      # NOTE: into a failure -- which must not be set here, because they are
+      # NOTE: read with `is_some` and a "0" would demand rather than excuse.
+      - name: The suite runs where the binary ships
+        if: runner.os != 'Linux'
+        run: cargo test --manifest-path rust/Cargo.toml --workspace --locked
 
   action-smoke:
     strategy:
diff --git a/CHANGELOG.md b/CHANGELOG.md
index cdce713..bf872d2 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -7,6 +7,29 @@ All notable changes to OComment will be documented here. The project follows
 
 ### Fixed
 
+- The test suite runs on the systems this repository publishes a binary for.
+  `cargo test` ran on Linux alone while `release.yml` shipped
+  `x86_64-pc-windows-msvc`; what Windows CI measured was that the crate builds
+  and prints its version. Because that job went green the whole run went green,
+  and a reader takes a green run for *Windows passes* — which is worse than
+  claiming nothing, because the ground for it is nowhere in the output.
+
+  The first thing it found was already known to one person who had run it by
+  hand: `a_first_segment_that_reads_as_a_drive_letter_is_disambiguated` asked a
+  question with two right answers. `c:/a.rs` names a directory called `c:` in a
+  POSIX checkout and the root of a drive on Windows, `std::path` says so, and
+  the SARIF location follows — under `%SRCROOT%` with a `./` on one system,
+  under no base on the other. The implementation was right on both; the test
+  held one system's answer and nothing had ever asked the other. It now asks
+  each, and a second case pins `under_source_root` itself, because both halves
+  would pass if that function simply stopped answering.
+
+- `sync_parent` is split by system instead of guarding its body, so the Windows
+  build no longer warns about a parameter the arm that does nothing cannot use.
+  Taken from an abandoned branch.
+
+### Fixed
+
 - A Go comment that opens with the word `go:` or `line ` after a space is
   prose, and both implementations were reading it as something the build
   requires. `// go:generate is what this line is about` was kept as
diff --git a/rust/ocomment/src/atomic.rs b/rust/ocomment/src/atomic.rs
index 446f09e..41b995a 100644
--- a/rust/ocomment/src/atomic.rs
+++ b/rust/ocomment/src/atomic.rs
@@ -161,12 +161,22 @@ fn reject_symlink(path: &Path, phase: &str) -> Result<()> {
     Ok(())
 }
 
+/// Flush the directory entry, so a rename survives a power cut.
+///
+/// Split by system rather than guarded inside one body: the parameter is unused
+/// on the arm that does nothing, and a warning a platform emits and nobody
+/// reads is one more line of noise between a reader and the warning that
+/// matters.
+#[cfg(unix)]
 fn sync_parent(path: &Path) -> Result<()> {
-    #[cfg(unix)]
-    {
-        let directory = fs::File::open(parent_directory(path))?;
-        directory.sync_all()?;
-    }
+    let directory = fs::File::open(parent_directory(path))?;
+    directory.sync_all()?;
+    Ok(())
+}
+
+/// Windows has no directory handle to flush; the rename is durable on its own.
+#[cfg(not(unix))]
+fn sync_parent(_: &Path) -> Result<()> {
     Ok(())
 }
 
diff --git a/rust/ocomment/src/output.rs b/rust/ocomment/src/output.rs
index 92226c5..296a60a 100644
--- a/rust/ocomment/src/output.rs
+++ b/rust/ocomment/src/output.rs
@@ -3756,12 +3756,31 @@ mod tests {
     /// scheme, so a checkout that really does hold a directory named `c:` says
     /// so with the one `.` segment a URI keeps for the purpose. Nothing else
     /// gains one, and a path that is under no base is left exactly as it was.
+    ///
+    /// The two spellings this is about are a different path on each system, so
+    /// the case is asked once per system rather than assumed. `c:/a.rs` names
+    /// a directory called `c:` in a POSIX checkout and the root of a drive on
+    /// Windows, and `std::path` says so: `components()` yields two `Normal`s
+    /// there and a `Prefix` here. Being under the source root and needing a
+    /// `./` follows from that, so the answer differs and both are right.
     #[test]
     fn a_first_segment_that_reads_as_a_drive_letter_is_disambiguated() {
-        let location = artifact_location(Path::new("c:/a.rs"));
-        assert_eq!(location["uri"], json!("./c:/a.rs"));
-        assert_eq!(location["uriBaseId"], json!(SRCROOT));
-        assert_eq!(artifact_location(Path::new("c:"))["uri"], json!("./c:"));
+        #[cfg(unix)]
+        {
+            let location = artifact_location(Path::new("c:/a.rs"));
+            assert_eq!(location["uri"], json!("./c:/a.rs"));
+            assert_eq!(location["uriBaseId"], json!(SRCROOT));
+            assert_eq!(artifact_location(Path::new("c:"))["uri"], json!("./c:"));
+        }
+        #[cfg(windows)]
+        {
+            /* NOTE: An absolute path, so it is under no base and claims none.
+             * The `./` exists to stop a reader taking a relative reference for
+             * a scheme, and there is no relative reference here to mistake. */
+            let location = artifact_location(Path::new("c:/a.rs"));
+            assert_eq!(location["uri"], json!(sarif_uri(Path::new("c:/a.rs"))));
+            assert!(location.get("uriBaseId").is_none());
+        }
         for plain in ["a.rs", "sub/doc.rs", "cc:/a.rs", "sub/c:/a.rs"] {
             assert_eq!(
                 artifact_location(Path::new(plain))["uri"],
@@ -3776,6 +3795,29 @@ mod tests {
         );
     }
 
+    /// The same question the case above asks, asked of the thing it turns on.
+    ///
+    /// Both halves of that test would pass if `under_source_root` simply
+    /// stopped answering, so this names what each system is expected to say
+    /// and why: a checkout holds `c:` as a directory only where `c:` can be a
+    /// directory name.
+    #[test]
+    fn a_drive_letter_is_a_directory_name_on_one_system_and_a_root_on_the_other() {
+        assert_eq!(under_source_root(Path::new("c:/a.rs")), cfg!(unix));
+        for both in ["a.rs", "sub/doc.rs", "cc:/a.rs"] {
+            assert!(
+                under_source_root(Path::new(both)),
+                "`{both}` is a relative path on every system"
+            );
+        }
+        for neither in ["/tmp/a.rs", "../a.rs"] {
+            assert!(
+                !under_source_root(Path::new(neither)),
+                "`{neither}` is not under the checkout on any system"
+            );
+        }
+    }
+
     /// Every result points into the rules by index, so the two orders have to
     /// be the same one.
     #[test]