Fix never-run Windows assertions in TestLocationEquality; un-skip on Windows#349
Merged
Conversation
Not for merge. Evaluates every TestLocationEquality locationMatch case without short-circuiting and dumps filepath.Abs/filepath.Match internals so one Windows CI run reveals all divergences at once.
…Windows Investigation (via a temporary Windows CI diagnostic) showed locationMatch itself is correct on Windows. All Windows failures were incorrect, never-executed test expectations: - //path vs /path was asserted equal unconditionally, but that is a POSIX-only assumption: on Windows // is a UNC root, so it is legitimately not equal to the drive-relative /path. locationMatch intentionally uses OS-native local-path semantics. Now asserted per-OS. - The Windows block under "Not equal paths with globs" asserted True for dir\* vs dir\ and dir\? vs dir\123\. Both are wrong and inconsistent with the POSIX cases: dir\* does not match the bare dir (cf. /path/* vs /path), and ? matches exactly one character so it does not match "123". Corrected to False. Remove the Windows t.Skip; locationMatch is now covered on all three platforms. (Diagnostic test and temp workflow are removed before merge.)
Investigation complete: TestLocationEquality passes on Windows with the corrected assertions (verified via this workflow before removal).
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.
Resolves the deferred follow-up from #347:
TestLocationEqualitywast.Skip-ped on Windows becausecmd/desynchad never run in CI and its never-executed Windows assertions were failing.Investigation method
Added a temporary diagnostic (a non-short-circuiting test that evaluated all 63
locationMatchcases and dumped thefilepath.Abs/filepath.Matchinternals) plus a throwaway Windows-only workflow, so a single Windows CI run revealed every divergence at once instead of peeling onerequirefailure per run. Both the diagnostic and the temp workflow were removed after the analysis; the net change here is test-only.Finding:
locationMatchis correct — the test was wrongOnly 5 of 63 cases failed on Windows, all in the local-path branch, and in every case the implementation behaves correctly and consistently with the test's own POSIX assertions:
//pathvs/pathtrue(unconditional)//is a UNC root, so it is legitimately different from the drive-relative/path.locationMatchintentionally uses OS-native local-path semantics (its documented contract).c:\path\to\*vsc:\path\to\true(win block)/path/*vs/path== false (already in this test).dir\*must not match the baredir./path/to/*vs\path\to\true(win block)c:\path\to\?vsc:\path\to\123\true(win block)?matches exactly one character;123is three./path/to/?vs\path\to\123\true(win block)Change
//pathvs/path: asserted per-OS —trueon POSIX,falseon Windows — with a comment explaining the UNC semantics.require.Truein the "Not equal paths with globs" Windows block →require.False(where they always belonged).t.Skip.locationMatchis now covered on Linux, macOS, and Windows.No change to
location.go— it was never buggy.Verified:
TestLocationEqualitypasses on Windows (confirmed via the temporary workflow before its removal, including theif runtime.GOOS == "windows"blocks); passes locally on Linux. This PR's CI exercises all three OSes.