ci: run the engine tests on Windows and macOS, not only Linux - #64
Merged
Conversation
Every job in this workflow ran on ubuntu, and the release workflow ships a Windows installer. So the platform-specific half of the engine — `default_excludes`, most of the rules database, quarantine's flattening of drive letters and backslashes — was only ever checked against string literals on Linux, and executed on Windows for the first time during the release build, after the tag was pushed. The test job is now a matrix over ubuntu, windows and macos, with `fail-fast` off so a break on two platforms reports as two. macOS is in it even though the release entry for macOS is still commented out: the rules describe `Library/Caches` and `Library/Logs` and the code has a `target_os = "macos"` arm, and none of that has ever run. `app-tauri` gains Windows for the same reason at a different layer: the Linux shell links webkitgtk and the GTK3 stack, the Windows one links WebView2, and a cfg-shaped mistake compiles on one and not the other. fmt and clippy move to their own `lint` job — they answer identically everywhere, and running them three times only slows the feedback. Note for whoever manages branch protection: the required check `test` becomes `test (ubuntu-22.04)`, `test (windows-latest)` and `test (macos-latest)`, and lint is now its own check.
Three gaps the new matrix can now actually exercise. `an_exclude_matches_a_real_directory_whatever_its_case` walks a real tree with a `Cache` directory excluded as `cache`. Windows and macOS fold case in the filesystem, Linux does not, so on Linux it passes only because `is_within` folds — which is the half that has to be right everywhere. `a_quarantine_name_carries_no_separator_from_the_original` pins the flattening. On Windows the original path opens `C:\`, and a colon or a backslash surviving into a filename is not a wrong name, it is an impossible one. `a_windows_path_that_cannot_be_recorded_is_refused` is the Windows half of the non-UTF-8 data-loss guard. Windows filenames are UTF-16 and may hold an unpaired surrogate, which serde rejects the same way it rejects invalid UTF-8 on Unix, so the same loss was reachable there. Creating such a file through the Win32 API isn't dependable, so it pins the guard that refuses the record rather than the whole flow — narrower than the Unix test, and deliberately so.
The matrix found both on its first run, and both were in the tests rather than the engine. Windows: `a_referenced_store_is_more_cautious_than_an_abandoned_one` looked for a finding whose path contained `live/node_modules`. Windows separates with `\`, so it matched nothing and the `unwrap` panicked. It compares whole paths now, built with `join`, which is separator-correct everywhere. macOS: `a_path_that_cannot_be_recorded_is_not_moved` could not create its fixture — APFS validates filenames as UTF-8 and answers EILSEQ, so a non-UTF-8 name cannot exist there and the data loss it caused is not reachable on macOS at all. Linux imposes no such rule, so the test returns early only there, and a creation failure anywhere else is still a hard failure rather than a quiet pass. Worth recording, since it narrows the bug fixed in #63: that loss was Linux-only in practice on Unix, and reachable on Windows through unpaired surrogates instead.
The comment claimed Windows and macOS fold case in the filesystem, so that only on Linux did the test depend on `is_within` folding. That is wrong in a way that invites damage: `is_excluded` compares two strings and never opens a file, so the filesystem's case rules cannot affect the outcome anywhere. Remove the folding and the test fails on all three. As written it read as licence either to skip the test off Linux or to treat the folding as redundant on a case-insensitive platform, both of which would quietly remove the coverage the test exists to give.
`Err(_) if cfg!(target_os = "macos")` treated any failure to create the fixture as the expected filesystem rejection, so a missing temp directory, a full disk or a permissions problem reported the test as passing having exercised nothing. The comment beside it already claimed a broken fixture would fail rather than pass quietly. Matching the errno APFS actually answers — EILSEQ, raw OS error 92, which is what the macOS run reported — makes that true instead of aspirational.
`cargo clippy` ran without `--all-targets`, so `#[cfg(test)]` code in diskern-core and diskern-cli was never linted — while `app-tauri` has always passed the flag. `cargo test` compiles test code but applies no lints to it, so a warning there was invisible on every platform. That matters more than it used to: the suite is now a good half of the crate by line count, and this workflow is the only thing looking at it. Clean as of this commit, so nothing to fix alongside — this is about the next one.
`app-tauri` covered Linux and Windows while `test` covered macOS too, so the one crate whose whole job is platform-specific was the one platform short. The reason Windows is in this job applies to macOS unchanged: the shells differ — webkitgtk on Linux, WebView2 on Windows, WKWebView on macOS — and a cfg-shaped mistake compiles on one and not another. The macOS release entry is commented out today, so leaving it off would have handed whoever re-enables it exactly the failure this job exists to prevent: a first compile after the tag is pushed. It costs a few minutes on pull requests that touch `crates/` or `app/` and nothing at all on any other, since `changes` gates the whole job.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
The engine ships on Linux and Windows, and every CI job ran on ubuntu.
So the platform-specific half of it —
default_excludes, most of therules database, quarantine's flattening of drive letters and separators —
was checked against string literals on Linux and executed on Windows for
the first time during the release build, after the tag was pushed.
This makes the platforms the code claims to support actually run it.
CI
testbecomes a matrix over ubuntu-22.04, windows-latest,macos-latest, with
fail-fast: falseso a break on two platformsreports as two rather than one.
app-taurigains windows-latest and macos-latest. The shellsdiffer — webkitgtk on Linux, WebView2 on Windows, WKWebView on macOS —
and a cfg-shaped mistake compiles on one and not another. Windows was
previously compiled for the first time at release; leaving macOS off
would hand the same failure to whoever re-enables its release entry.
fmtandclippymove to their ownlintjob. They answeridentically on every platform; running them three times only delays
the feedback. Clippy also gains
--all-targets, so the test code —by now a good half of the crate — is linted rather than merely
compiled.
macOS is in the matrix even though its release entry is still commented
out pending an Apple Developer account: the rules describe
Library/CachesandLibrary/Logs,default_excludeshas atarget_os = "macos"arm, and none of it has ever run.All runners are free on a public repository, so this costs wall-clock,
not money.
Tests
Three gaps the matrix can now exercise:
an_exclude_matches_a_real_directory_whatever_its_case— walks areal tree with a
Cachedirectory excluded ascache. Windows andmacOS fold case in the filesystem; Linux does not, so on Linux it
passes only because
is_withinfolds, which is the half that has tobe right on all three.
a_quarantine_name_carries_no_separator_from_the_original— pinsthe flattening. On Windows the original path opens
C:\, and a colonor backslash surviving into a filename isn't a wrong name, it's an
impossible one.
a_windows_path_that_cannot_be_recorded_is_refused— the Windowshalf of the non-UTF-8 data-loss guard from fix: close every open issue — glob rules, quarantine manifest, honest totals, the graph stage, and the advisory backlog #63. Windows filenames are
UTF-16 and may hold an unpaired surrogate, which serde rejects exactly
as it rejects invalid UTF-8 on Unix, so the same loss was reachable
there. Creating such a file through the Win32 API isn't dependable, so
this pins the guard that refuses the record rather than the whole
flow — narrower than its Unix counterpart, deliberately.
Two things to know
Check names change, but nothing is gated on them.
testbecomestest (ubuntu-22.04)/(windows-latest)/(macos-latest),app-taurilikewise, andlintis new. I originally flagged this asneeding a branch-protection update; I then checked, and
mainhas noprotection (
404 Branch not protected) and the repository's onlyruleset is
enforcement: disabled. So no required check breaks. Worthknowing if protection is ever turned on.
The first run failed on both new platforms, which is the whole
argument for the change. Both faults were in the tests, not the engine:
a_referenced_store_is_more_cautious_than_an_abandoned_onesearched for a path containing
live/node_modules. Windows separateswith
\, so it matched nothing and theunwrappanicked. It compareswhole paths built with
joinnow.a_path_that_cannot_be_recorded_is_not_movedcould notcreate its fixture: APFS validates filenames as UTF-8 and answers
EILSEQ. A non-UTF-8 filename cannot exist on macOS, so the dataloss fixed in fix: close every open issue — glob rules, quarantine manifest, honest totals, the graph stage, and the advisory backlog #63 was never reachable there. The test returns early on
macOS only; a creation failure on any other platform is still a hard
failure rather than a quiet pass.
That second one is worth recording beyond this PR: it narrows the bug
#63 fixed to Linux on the Unix side, with Windows reachable separately
through unpaired surrogates. Nothing about the fix changes — encoding
before moving is right everywhere — but the blast radius was smaller
than the issue implied.
All eleven checks now pass, including
test (windows-latest),test (macos-latest)andapp-tauri (windows-latest).Checklist
cargo fmt --allandcargo clippy --workspaceare cleancargo test --workspacepasses (Linux locally; other platforms viathis PR's own matrix)
behaviour change
Review follow-up
A review pass over this PR turned up four things, all fixed here:
--all-targets, leaving every#[cfg(test)]linein core and cli unlinted while CI stayed green
app-tauricovered Windows but not macOS, leaving the one crate whosejob is platform-specific a platform short
Err(_), so a missing temp directoryor a full disk would have reported the test as passing having run
nothing; it now matches EILSEQ (raw OS error 92) specifically
Windows and macOS. It does not —
is_excludedcompares strings andnever opens a file — and the comment read as licence to drop either
the folding or the test