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]