Skip to content

test(symlink): say what a local skip means — CI is not skipping these - #116

Merged
askalf merged 2 commits into
masterfrom
docs/symlink-skip-message
Jul 25, 2026
Merged

test(symlink): say what a local skip means — CI is not skipping these#116
askalf merged 2 commits into
masterfrom
docs/symlink-skip-message

Conversation

@askalf

@askalf askalf commented Jul 24, 2026

Copy link
Copy Markdown
Owner

A local run on Windows without Developer Mode reports # skipped 4 for test/symlink-skills.test.mjs, and that count reads as a Windows coverage hole.

It is not one. The skip has always been a runtime capability probe rather than a platform check, and all three CI runners create symlinks fine:

job skipped
windows-latest, node 20 0
windows-latest, node 22 0
ubuntu / macos (20 & 22) 1 — the DPAPI test, Windows-only by design

Every test in that file already runs on every platform. The only machines that skip are developer boxes without Developer Mode.

I misread that count myself and proposed rebuilding these tests around Windows junctions to "un-skip them in CI" — which would have added a second set of fixtures for coverage that already exists. Checking the runner logs first is what killed that idea.

So the fix is to stop the count from misleading the next person: the comment now says where the skip bites and where it does not, and the skip message tells you how to run them locally rather than only stating that a privilege is missing.

# SKIP needs symlink privilege — enable Windows Developer Mode to run these
#      locally (CI runs them on all three OSes)

No behaviour change, no new coverage, tests only. Independent of #110-#115.

A local run on Windows without Developer Mode reports `# skipped 4` for this
file, and that count reads as a Windows coverage hole. It is not one.

The skip has always been a runtime capability probe rather than a platform
check, and all three CI runners create symlinks fine — the windows-latest jobs
report `# skipped 0` on node 20 and 22 alike, so every test here runs on every
platform. The only machines that skip are developer boxes without Developer
Mode.

I misread that count myself and proposed rebuilding these tests around Windows
junctions to "un-skip them in CI", which would have added a second set of
fixtures for coverage that already exists. The comment now says where the skip
bites and where it does not, and the skip message tells you how to run them
locally instead of just stating the privilege is missing.

No behaviour change, no new coverage, tests only.

@sprayberry-reviewer sprayberry-reviewer left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Automated review from the Sprayberry Labs fleet code reviewer.

Verdict: Clean — no blocking issues. Comment/message-only change, approving.

What I checked:

  • Full diff (test/symlink-skills.test.mjs, +17/-6): every hunk either rewrites the top-of-file comment explaining the symlink-privilege skip, or swaps four call sites' inline 'symlinks unprivileged here' skip string for a shared NO_LINKS constant with a more actionable message. No test logic, assertions, or control flow changed — confirmed by re-reading each hunk context (link(...) calls, t.skip(...) calls) around the changed lines.
  • CI check-rollup: all 8 checks green (test on ubuntu/macos/windows × node 20/22, CodeQL analyze, CodeQL run, "verify pinned skills"), completed ~21:15Z, none pending/failing.
  • PR body's claim (Windows CI runs report # skipped 0, i.e. the skip only bites on unprivileged local dev boxes) is consistent with the CI rollup showing the windows-latest jobs passing same as others.

What's good: Extracting the repeated skip-message string into NO_LINKS (test/symlink-skills.test.mjs) removes duplication across the four call sites and the new message tells a developer how to unblock locally instead of just stating a fact. Comment rewrite is accurate and scoped — no new coverage claimed, none needed.

No correctness, security, reuse, or test-coverage findings on this diff.

@askalf
askalf merged commit c21e3a5 into master Jul 25, 2026
9 checks passed
@askalf
askalf deleted the docs/symlink-skip-message branch July 25, 2026 14:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants