fix: run the suite on the systems that get a binary

.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:

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

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(())
 }
 

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]