ci: extract test.yml assertions into standalone scripts - #38
Merged
Conversation
test.yml previously embedded every test case as inline bash in the workflow YAML, so adding a test meant editing the workflow. Split the assertions into .github/scripts/functional-tests.sh and integration-tests.sh (using the same run_test-harness pattern as test.sh); the workflow now only provisions CI fixtures (packages, test users) and calls the scripts. New tests are added as a test_* function + run_test line in the appropriate script, no workflow changes needed. Behavior is unchanged — same test users, same fixtures, same pass/fail messages, verified by running both scripts locally end-to-end (including the symlink-attack integration tests) against a live GitHub API/keys endpoint. Also fixes a `set -o pipefail` bug introduced during extraction: piping the (intentionally failing) sync script into `grep -q` made pipefail report the script's exit code instead of grep's match result, flipping an assertion that checks for an expected error message. Updates AGENTS.md, .github/copilot-instructions.md, and TESTING.md references to point at the new script locations. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Two gaps found while reviewing CI coverage: - Self-update's sha256 digest verification (added in #37) had zero test coverage — the existing check only grepped for function names, not behavior. test_self_update_round_trip runs a full --self-update against the real latest GitHub release on a scratch copy of the script, so a broken digest extraction or verification would fail CI. - Integration tests checked authorized_keys/.ssh permission bits (700/600) but never verified chown ownership, even though get_user_gid/chown is its own code path that could silently leave files root-owned. test_file_ownership_correct checks both are owned by the target user. Verified locally end-to-end: 11/11 functional tests and 5/5 integration tests pass, including the new round-trip and ownership checks. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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
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.
Summary
Reviewed the CI testing setup.
test.ymlembedded every test case as inline bash directly in the workflow YAML — adding a single new test case meant editing the workflow file itself, and the ~360-line YAML mixed fixture provisioning (users, packages) with test logic (assertions, grep checks).This splits test logic out into two scripts that mirror the existing
test.shpattern (samerun_testharness style):.github/scripts/functional-tests.sh— checks previously in thetestjob (config validation, invalid method handling, empty user array, function presence, etc.), plus a new self-update round-trip test.github/scripts/integration-tests.sh— checks previously in theintegration-testjob (real sync run, permission checks, both symlink-attack tests), plus a new file ownership checktest.ymlnow only provisions CI fixtures (packages, temporary Linux test users) and calls the scripts. Adding a new test case is now just atest_*function +run_testline in the relevant script — no workflow YAML changes required.Existing behavior is unchanged: same test users, same fixtures, same assertions and pass/fail messages.
New test coverage
While reviewing what else was worth adding:
test_self_update_round_trip— runs a full--self-updateagainst the real latest GitHub release on a scratch copy of the script. The sha256 digest verification added in security: verify self-update checksum and stop leaking GitHub token via argv #37 previously had zero behavioral coverage; the old check only grepped for function names, not that verification actually works.test_file_ownership_correct— verifies.ssh/authorized_keysarechowned to the target user. Only permission bits (700/600) were checked before; ownership is a separate code path (get_user_gid/chown) that could silently leave files root-owned.Considered and skipped as too flaky/heavy for the value:
psoutput — timing-dependent.curl/GitHub API responses.Testing
bash -non both new scripts,shellcheck -S warning(matches CI's lint severity) — cleanfunctional-tests.shlocally end-to-end against real test users, the livegithub.com/locus313.keysendpoint, and the live self-update round trip — all 11 passintegration-tests.shlocally end-to-end, including both symlink-attack tests against real/root/.sshcanaries and the new ownership check — all 5 passbash test.sh— all 10 existing unit tests still passBug found during extraction
The scripts originally had
set -o pipefail. Several tests intentionally run the sync script expecting it to fail, then pipe its output intogrep -q "expected error message". Withpipefail, the pipeline's exit status was the sync script's nonzero code rather thangrep's (matching) result, which flipped those assertions to fail even when the correct error was printed. Fixed by droppingpipefail(eachtest_*function's own return code is whatrun_testchecks, so it isn't needed).Docs updated
AGENTS.md,.github/copilot-instructions.md(maintenance matrix + CI/CD section), andTESTING.mdnow point at the new script locations instead oftest.yml.Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com