Skip to content

Tests: run ShellCheck over every shell script the tests write - #86

Merged
krishhgg merged 4 commits into
mainfrom
ci/shellcheck-test-fakes
Oct 12, 2026
Merged

krishhgg merged 4 commits into
mainfrom
ci/shellcheck-test-fakes

Conversation

@krishhgg

@krishhgg krishhgg commented Oct 12, 2026 •

Copy link
Copy Markdown
Owner

Phase 1 of the testing plan, item 3: lint the shell scripts the tests write. It was stacked on #85. #85 and #89 have merged, the base is now main, and 74a580d merges main in.

Why

Fakes of sudo, pmset, launchctl and the other tools decide what a test sees. A broken fake makes a test pass or fail for the wrong reason. A fake gh that used $10, which bash reads as ${1}0, faked 16 failures on #51. The ShellCheck job lints scripts/*.sh only. The fakes are strings inside Swift tests, and nothing linted them.

What changes

  • One writer. TestScripts.write(_:to:executable:) (Tests/InsomniaTests/TestScripts.swift) writes a script and makes it executable, unless the test runs it through /bin/bash. The 30 call sites that write fakes, stand-in backstops, wrapper scripts and the fake swiftc's icon generator now go through it. Two kinds of script don't, because the ShellCheck job lints the originals: patched copies of the real scripts in scripts/, and test harnesses built from functions cut out of them.
  • A copy of each script. When INSOMNIA_TEST_SCRIPT_DUMP names a folder, each script is also copied there, once per distinct text. UUIDs (the per-test temp folders) are zeroed first. That way the RecoveryScriptTests fixture's fakes, written for each of its 231 tests, land once. index.tsv names the test source line that first wrote each copy.
    • A script with no #! line is one a test runs with /bin/bash. Its copy (never the real file) starts with # shellcheck shell=bash, so ShellCheck lints it as bash instead of stopping at SC2148.
    • The index line is written before the copy. A copy can't exist without being linted, and an index line whose copy failed to write fails ShellCheck.
  • The CI step. The Swift job sets the variable. A new last step downloads ShellCheck 0.11.0, checks its sha256 against the digest GitHub lists for the release asset, and lints the folder at ShellCheck's default severity, the same as the scripts job. It runs whenever the tests passed, so a lint failure doesn't hide the release build's result. An empty folder fails the step, so a renamed variable can't pass by checking nothing.
  • Four deliberate constructs. ShellCheck flagged four things, and all are intended. Each gets a # shellcheck disable= line with the reason:
    • SC2053 in RecoveryScriptTests' mv fake: its mv.fail patterns are globs.
    • SC2254 in PackagingTests' mv fake: its mv-fails patterns are globs.
    • SC2015 in the sudo fake's visudo branch: A && B || reject means "reject unless both pass".
    • SC2034 in capture.sh: the test checks that the command substitution sees end of file, not what it captured.

Checks

  • Local run at 2ca0534. The tree's diff hash matched before the run, after it, and at commit. The seven classes that write scripts ran with the variable set: RecoveryScriptTests, PackagingTests, LidSimulationGateScriptTests, PrivilegedCommandLockTests, CommandCancellationTests, DockerRuleEndpointTests and ReleaseWorkflowTests.
    • 288 tests, 1 failure. The failure is testTheSupervisorOutlivesItsRunAndGroupSignalsAndHoldsTheLockUntilItReapsTheCommand, the fake sudo race on main that Tests: let the fake sudo wait on SECONDS so one signal is logged once #89 fixes. This PR doesn't change that fake's text. The Mac did not sleep during the run.
    • The folder held 73 distinct scripts, up from 69 at b9c5ea5. The four new ones are the icon generator, the backstop stand-in, the sleep sentinel and the umask wrapper.
    • The CI step's script, run against that folder with the pinned ShellCheck, exited 0.
  • Earlier local run at b9c5ea5. Clean tree before and after: 288 tests, 0 failures, 69 distinct scripts, step exit 0.
  • The step fails when it should.
    • Before the annotations, the step failed on the five warnings and notes above.
    • With a $10 fake added to the folder, it failed with SC1037 ("Braces are required for positionals over 9").
    • With $10 planted in the umask wrapper's copy, one of the scripts without #!, it also failed with SC1037. The same copy without the shell=bash line fails with SC2148.
    • With an empty folder, it failed with "the tests wrote no scripts".
  • CI. At 2ca0534 both jobs passed: macOS 26.6.2 (25G83) and macOS 27.0.1 (26A434), 1223 tests, 10 skipped, 0 failures each. On both, the ShellCheck step found 73 distinct scripts and passed. At 74a580d, which adds Tests: let the fake sudo wait on SECONDS so one signal is logged once #89's fake sudo text, both jobs passed again with the same totals.
  • Reviews. Codex (gpt-6.1-sol, xhigh, read-only) reviewed a88d05c and found three gaps. The fake swiftc's icon generator sat in a heredoc that ShellCheck read as text. Four scripts without #! skipped the helper; Greptile's comment flags the same thing. A copy could exist without its index line. 2ca0534 fixes all three. Codex found nothing at 2ca0534, or in the merge at 74a580d, which adds exactly Tests: let the fake sudo wait on SECONDS so one signal is logged once #89's change.
  • Not run locally. UIStatusTests, UIStartupTests and KeychainStoreTests (none write scripts). CI runs the whole suite on macOS 26 and 27.
  • actionlint 1.7.12 passes locally.

Tests and CI only: no Mac report needed.

🤖 Generated with Claude Code

RetriggerConfidence Score: 5/5

The PR appears safe to merge; no actionable new issues remain.

Summary

The PR sends test-written shell scripts through TestScripts.write and adds a CI step to lint their copies.

  • CI now checks every distinct shell script that the tests write.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  Tests[Test script writers] --> Writer[TestScripts.write]
  Writer --> Original[Script used by the test]
  Writer --> Dump[Normalized lint copy and index.tsv]
  Dump --> Lint[CI ShellCheck step]
Loading

Reviews (3) · Last reviewed commit: "Merge main into ci/shellcheck-test-fakes" · Reviewed by Greptile

krishhgg and others added 2 commits October 11, 2026 16:53
Fakes of sudo, pmset, launchctl and the other tools decide what a test
sees. A broken one makes a test pass or fail for the wrong reason (a
fake gh that used $10, which bash reads as ${1}0, faked 16 failures on
#51), and nothing linted them.

TestScripts.write now writes every script a test authors. When
INSOMNIA_TEST_SCRIPT_DUMP names a folder, it also copies each one there,
once per distinct text (UUIDs zeroed first), with index.tsv naming the
source line that wrote it. CI sets the variable and, at the end of the
Swift job, runs ShellCheck 0.11.0 (pinned and checked against its
digest) over the folder. An empty folder fails the step.

ShellCheck found four deliberate constructs: glob patterns in two mv
fakes, an A && B || C in the sudo fake, and capture.sh's unused output.
Each gets a disable line with the reason.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
…lcheck-test-fakes

# Conflicts:
#	.github/workflows/ci.yml
Comment thread .github/workflows/ci.yml
Codex's review of a88d05c and Greptile (comment 4243820315) found
scripts the ShellCheck step never saw.

- The fake swiftc wrote the icon generator from a heredoc inside its
  own text, so ShellCheck read the generator as a string. The generator
  is now its own file, written through TestScripts.write, and the fake
  copies it to swiftc's -o path.
- Four RecoveryScriptTests scripts have no #! line because the test runs
  them with /bin/bash: the installed-backstop stand-in (two writers, one
  text), the sleep sentinel and the umask wrapper. They now go through
  TestScripts.write with executable: false. A dump copy of a script with
  no #! line starts with `# shellcheck shell=bash`, so ShellCheck lints
  it as bash instead of stopping at SC2148.
- TestScripts wrote the copy before its index line. A failure between
  the two left a copy the index never named, and a later write of the
  same text found the copy and skipped it, so it was never linted. The
  index line now goes first, and a missing copy fails ShellCheck.

The dump of the seven script-writing classes went from 69 to 73
distinct scripts, and ShellCheck 0.11.0 passes on all of them. A `$10`
planted in the umask wrapper's copy fails with SC1037; the same copy
without the directive fails with SC2148.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@krishhgg
krishhgg changed the base branch from ci/macos-27-runner to main October 12, 2026 02:12
Brings in #89's fake sudo change, so CI lints the fake's new text, and #85's merge.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
@krishhgg
krishhgg merged commit ffefca7 into main Oct 12, 2026
5 checks passed
krishhgg added a commit to 0xRafie/Insomnia that referenced this pull request Oct 12, 2026
Brings in krishhgg#86, which runs ShellCheck over the scripts the tests write.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
krishhgg added a commit to D4TBot/Insomnia that referenced this pull request Oct 12, 2026
Brings in krishhgg#86, which runs ShellCheck over the scripts the tests write.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
krishhgg added a commit to nsd97/Insomnia that referenced this pull request Oct 12, 2026
Brings in krishhgg#86's TestScripts.write, which the next commit uses for this
PR's test scripts, and the other merges since bb5cf2b.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
krishhgg added a commit to nsd97/Insomnia that referenced this pull request Oct 12, 2026
This branch started before krishhgg#86, so the new test wrote both scripts
directly and CI's ShellCheck step never saw them. They now go through
TestScripts.write like the sibling hand-down test's wrapper.

Co-Authored-By: Claude Opus 5.5 (1M context) <[email protected]>
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.

1 participant