Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
124 changes: 124 additions & 0 deletions .github/workflows/spm-smoke-test.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,124 @@
# Smoke-tests the `a11y-scan` SwiftPM command plugin end-to-end on every PR:
# builds the plugin and runs a real accessibility scan against the tests/spm
# harness (sample SwiftUI sources with intentional a11y issues). It reuses the
# repository's own gated integration test (testA11yScanPluginRuns) so the scan
# invocation stays defined in exactly one place.
#
# The scan downloads the BrowserStack CLI and makes authenticated network calls,
# so it needs BROWSERSTACK_USERNAME / BROWSERSTACK_ACCESS_KEY repo secrets. Those
# secrets are never exposed to fork PRs, so that job is gated to same-repo PRs
# (and manual dispatch); fork PRs skip it. The scan step is itself guarded on the
# secrets being present, so if they are not configured the scan is skipped and the
# job still passes on the build step alone.
#
# A second job (scripts-lint) syntax-checks every launcher script under scripts/.
# It needs no secrets, so it runs on all PRs including forks.
name: SPM plugin smoke test

on:
pull_request:

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Low] No paths: filter and no push: main trigger (drift from the sibling workflow)

Two divergences from verify-selfupdate-checksums.yml: there is no paths: filter, so a 25-minute macOS job making real authenticated scans runs on every PR including docs-only ones; and there is no push: branches: [main], so nothing re-verifies the plugin after merge and main can break unnoticed until the next PR.

Suggestion: add a paths: filter (Plugins/**, tests/**, Package.swift, and this workflow) plus a push: branches: [main] trigger to match the sibling workflow.

Reviewer: stack:devtools-review-changes

branches: [main, master]
workflow_dispatch:

permissions:
contents: read

concurrency:
group: spm-smoke-${{ github.ref }}
cancel-in-progress: true

jobs:
spm-smoke:
name: a11y-scan end-to-end (SwiftPM)
runs-on: macos-14
timeout-minutes: 25
# Secrets are unavailable to fork PRs, so the authenticated scan can only run
# on same-repo PRs or a manual dispatch. Fork PRs skip this job.
if: github.event_name == 'workflow_dispatch' || github.event.pull_request.head.repo.fork == false
env:
BROWSERSTACK_USERNAME: ${{ secrets.BROWSERSTACK_USERNAME }}
BROWSERSTACK_ACCESS_KEY: ${{ secrets.BROWSERSTACK_ACCESS_KEY }}
# Un-gates tests/spm/Tests/A11yDemoLibTests/testA11yScanPluginRuns, which is
# skipped unless RUN_A11Y_SCAN=1 and BrowserStack credentials are present.
RUN_A11Y_SCAN: "1"
steps:
- name: Checkout
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

- name: Swift toolchain
run: swift --version

# The repo root is a plugin-only package (no buildable target), so it is
# not built directly. Building the tests/spm harness compiles both the
# a11y-scan command plugin (via the path dependency) and the sample sources.
- name: Build harness (compiles the a11y-scan plugin)
working-directory: tests/spm
run: swift build

# Guarded on the secrets actually being set: GitHub exposes an unset secret
# as an empty string (present, not nil), so without this guard the scan would
# run with empty credentials and fail. When the secrets are absent this step
# is skipped and the job stays green on the build step alone.
#
# The scan hits BrowserStack (network + auth + CLI download), so a transient
# upstream hiccup should not red-block a PR. Retry the scan up to 3 times with
# backoff; a consistent failure still fails the gate. `swift test` reuses the
# first attempt's build, so retries only re-run the scan.
- name: End-to-end scan smoke (tests/spm)
if: env.BROWSERSTACK_USERNAME != '' && env.BROWSERSTACK_ACCESS_KEY != ''
working-directory: tests/spm
run: |
set -uo pipefail
attempts=3
for i in $(seq 1 "$attempts"); do

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Medium] Retry re-runs deterministic assertion failures three times

The loop cannot distinguish a transient upstream hiccup from a deterministic assertion failure, so a genuine regression is retried three times. Each attempt runs two full scans (non-strict + strict), each downloading/exec'ing the CLI and making authenticated round trips, plus 2x20s backoff — all inside timeout-minutes: 25. If a scan pair exceeds ~8 minutes, a real regression surfaces as an opaque job timeout with no assertion message instead of the intended diagnostic.

Suggestion: retry only on transient signatures (grep the captured output for network/download/5xx markers) and fail fast on an XCTest assertion failure; or drop to 2 attempts and raise timeout-minutes so the retry budget provably fits.

Reviewer: stack:devtools-review-changes

echo "::group::a11y-scan smoke attempt $i/$attempts"
if swift test; then

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Medium] Gate passes if the E2E test XCTSkips — nothing asserts it ran

swift test exits 0 when testA11yScanPluginRuns throws XCTSkip, and nothing here asserts the test actually executed. Today the guards line up and the scan provably runs, but any drift — the env var renamed on one side, the test renamed or moved, an extra guard added — silently turns this gate into a swift build check that still shows green. That is the exact silent-pass class this workflow exists to prevent.

Suggestion: scope and assert execution, e.g. swift test --filter 'A11yDemoLibTests/testA11yScanPluginRuns' 2>&1 | tee out.log, then fail the step if the log contains skipped or lacks a passed line for that test. A --filter that matches nothing also exits non-zero, which catches a rename.

Reviewer: stack:devtools-review-changes

echo "::endgroup::"
exit 0
fi
echo "::endgroup::"
if [ "$i" -lt "$attempts" ]; then
echo "Attempt $i failed; retrying in 20s (occasional transient upstream failures are expected)."
sleep 20
fi
done
echo "::error::a11y-scan smoke failed after $attempts attempts."
exit 1

scripts-lint:
name: Launcher scripts (bash syntax)
runs-on: ubuntu-latest
timeout-minutes: 5
steps:
- name: Checkout
uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4.2.2

# Every script under scripts/ — the bash, zsh and fish variants alike — is
# a bash script (`#!/usr/bin/env bash -il`); the variants differ only in which
# login shell they source BrowserStack creds from. So all of them are
# syntax-checked with `bash -n`. The scripts self-update, register git
# hooks and need credentials, so they are not executed here — this is a
# static syntax gate. (Checksum integrity is covered separately by
# verify-selfupdate-checksums.yml.)
- name: Syntax-check all launcher scripts (bash -n)
run: |
set -uo pipefail
shopt -s globstar nullglob
scripts=(scripts/**/*.sh)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Medium] scripts-lint glob misses the scripts this PR actually changes

scripts/**/*.sh matches only the 6 launchers under root scripts/. The two scripts this PR modifies — tests/spm/scripts/run-a11y-scan.sh and tests/xcode-app/scripts/run-a11y-scan.sh — fall outside the glob, so this job would not have caught a syntax error in the very files being changed. tests/xcode-app/scripts/run-a11y-scan.sh (wired into the Xcode build phase via project.yml) has no coverage from either job.

Suggestion: broaden to scripts=(scripts/**/*.sh tests/**/scripts/*.sh) or git ls-files '*.sh', and soften the comment above to match real coverage. Scoping to scripts/ was the original request, so widening is a judgement call.

Reviewer: stack:devtools-review-changes

if [ ${#scripts[@]} -eq 0 ]; then
echo "::error::No .sh scripts found under scripts/ — checkout or glob is wrong."
exit 1
fi
status=0
for script in "${scripts[@]}"; do
# Plain log lines, not ::notice file=/::error file= workflow commands:
# scripts/ filenames are attacker-controllable on fork PRs, and
# interpolating them into a workflow command is an injection vector.
if bash -n "$script"; then
echo "OK $script"
else
echo "FAILED $script (bash -n syntax error above)"
status=1
fi
done
exit "$status"
2 changes: 1 addition & 1 deletion tests/spm/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ The script runs:
swift package plugin \
--allow-writing-to-directory ~/.cache \
--allow-writing-to-package-directory \
--allow-network-connections 'all(ports: [])' \
--allow-network-connections all:80,443 \
scan --include "**/*.swift" --include "**/*.xib" --include "**/*.storyboard"
```

Expand Down
42 changes: 37 additions & 5 deletions tests/spm/Tests/A11yDemoLibTests/A11yDemoLibTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,11 @@ final class A11yDemoLibTests: XCTestCase {
guard env["RUN_A11Y_SCAN"] == "1" else {
throw XCTSkip("Set RUN_A11Y_SCAN=1 (with BrowserStack creds) to run the plugin end-to-end.")
}
guard env["BROWSERSTACK_USERNAME"] != nil, env["BROWSERSTACK_ACCESS_KEY"] != nil else {
// Treat an empty value as absent: CI exposes an unset secret as "" (present,
// not nil), and running the scan with empty credentials fails at auth rather
// than skipping.
guard env["BROWSERSTACK_USERNAME"]?.isEmpty == false,
env["BROWSERSTACK_ACCESS_KEY"]?.isEmpty == false else {
throw XCTSkip("BROWSERSTACK_USERNAME / BROWSERSTACK_ACCESS_KEY are required for the scan.")
}

Expand All @@ -30,15 +34,43 @@ final class A11yDemoLibTests: XCTestCase {
.deletingLastPathComponent()
let script = packageDir.appendingPathComponent("scripts/run-a11y-scan.sh")

// Run the scan twice and use the tool's own exit-code contract to prove it
// not only ran but actually detected the intentional issues in
// SampleViews.swift:
// * --non-strict -> exit 0 (CLI downloaded, authenticated, ran cleanly;
// issues do not fail the run)
// * strict -> exit != 0 (issues were found; strict fails on issues)
// Asserting only the non-strict exit 0 would also pass if the scan
// authenticated but found nothing -- a silent no-op. Requiring the strict
// run to fail closes that gap.
let clean = try runScan(script: script, packageDir: packageDir, strict: false)
XCTAssertEqual(
clean.status, 0,
"a11y-scan did not run cleanly in --non-strict mode (exit \(clean.status)).\n\(clean.output)")

let strict = try runScan(script: script, packageDir: packageDir, strict: true)
XCTAssertNotEqual(

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Medium] Strict assertion accepts any non-zero exit, not just "issues found"

The plugin forwards several distinct non-zero codes — exit 2 (unwritable cache), exit 4 (RBAC denial), the curl status on a failed CLI download, exit 1 on abnormal termination — all of which satisfy != 0. Since the strict run is a separate process from the non-strict one, a transient failure in that second run is indistinguishable from "the planted issues were detected", so this fidelity check can pass for the wrong reason.

Suggestion: assert the specific lint-failure code (strict.status == 1) rather than any non-zero, and additionally assert strict.output references SampleViews.swift or a known rule id, so the test proves the planted issues caused the failure.

Reviewer: stack:devtools-review-changes

strict.status, 0,
"a11y-scan ran but reported no issues against SampleViews.swift (strict exit 0) -- possible silent no-op or engine regression.\n\(strict.output)")
}

/// Runs `scripts/run-a11y-scan.sh` (optionally strict) and returns its exit
/// status plus combined stdout/stderr. Draining to EOF before `waitUntilExit`
/// avoids a full-pipe-buffer deadlock without a background reader.
private func runScan(script: URL, packageDir: URL, strict: Bool) throws -> (status: Int32, output: String) {
let process = Process()
process.executableURL = URL(fileURLWithPath: "/bin/bash")
process.arguments = [script.path, "--non-strict"]
process.arguments = strict ? [script.path] : [script.path, "--non-strict"]
process.currentDirectoryURL = packageDir

let pipe = Pipe()
process.standardOutput = pipe
process.standardError = pipe

try process.run()
let collected = pipe.fileHandleForReading.readDataToEndOfFile()

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

[Low] Child process has no timeout; output only surfaces on failure

standardInput is left at the default (unlike the plugin's own runCLI, which wires it explicitly), so if the CLI stalls or waits on stdin, readDataToEndOfFile() blocks and the job emits zero log output until the 25-minute timeout — an empty ::group:: and no diagnostic to debug from.

Suggestion: set process.standardInput = FileHandle.nullDevice, add a watchdog that terminates the process after N minutes, and/or print the captured output unconditionally so the scan log reaches CI on success and on hang.

Reviewer: stack:devtools-review-changes

process.waitUntilExit()

// --non-strict makes the scan exit 0 even when issues are found, so a
// clean exit means the plugin downloaded, authenticated, and ran.
XCTAssertEqual(process.terminationStatus, 0, "a11y-scan plugin failed to run")
return (process.terminationStatus, String(data: collected, encoding: .utf8) ?? "")
}
}
2 changes: 1 addition & 1 deletion tests/spm/scripts/run-a11y-scan.sh
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ cd "$(dirname "$0")/.."
swift package plugin \
--allow-writing-to-directory "$HOME/.cache" \
--allow-writing-to-package-directory \
--allow-network-connections 'all(ports: [])' \
--allow-network-connections all:80,443 \
scan \
--include "**/*.swift" \
--include "**/*.xib" \
Expand Down
2 changes: 1 addition & 1 deletion tests/xcode-app/scripts/run-a11y-scan.sh
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ fi
swift package plugin \
--allow-writing-to-directory "$HOME/.cache" \
--allow-writing-to-package-directory \
--allow-network-connections 'all(ports: [])' \
--allow-network-connections all:80,443 \
scan \
--include "**/*.swift" \
--include "**/*.xib" \
Expand Down
Loading