fix(ci): skip Playwright tests in insider publish (covered by Squad CI)#852
fix(ci): skip Playwright tests in insider publish (covered by Squad CI)#852tamirdresher wants to merge 2 commits intodevfrom
Conversation
The insider publish workflow had two issues causing 80+ test failures: 1. The test job ran on a separate VM from the build job with no artifact transfer. Since dist/ is gitignored, the test job had no compiled output. Vitest failed to resolve workspace package exports (e.g. @bradygaster/squad-sdk entry, squad-cli/commands/rc) because the dist files they point to didn't exist. 2. All three jobs used Node 20, but both packages declare engines.node >= 22.5.0. The CLI's version gate rejected Node 20 at runtime, causing additional failures. Fix: Add 'npm run build' step to the test job, and update all jobs from node-version [20] to [22] to match the engines requirement and the main CI workflow. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
🟢 Impact Analysis — PR #852Risk tier: 🟢 LOW 📊 Summary
🎯 Risk Factors
📦 Modules Affectedci-workflows (1 file)
This report is generated automatically for every PR. See #733 for details. |
🏗️ Architectural Review
Automated architectural review — informational only. |
🛫 PR Readiness Check
|
| Status | Check | Details |
|---|---|---|
| ❌ | Single commit | 2 commits — consider squashing before review |
| ✅ | Not in draft | Ready for review |
| ✅ | Branch up to date | Up to date with dev |
| ❌ | Copilot review | No Copilot review yet — it may still be processing |
| ✅ | Changeset present | No source files changed — changeset not required |
| ✅ | Scope clean | No .squad/ or docs/proposals/ files |
| ✅ | No merge conflicts | No merge conflicts |
| ✅ | Copilot threads resolved | No Copilot review threads |
| ❌ | CI passing | 1 check(s) failing: release |
This check runs automatically on every push. Fix any ❌ items and push again.
See CONTRIBUTING.md and PR Requirements for details.
There was a problem hiding this comment.
Pull request overview
Updates the insider publish GitHub Actions workflow to avoid running Playwright/Docker-dependent integration coverage during publish, relying on the main Squad CI workflow for full integration testing.
Changes:
- Bump the workflow’s Node.js matrix from 20 to 22 across jobs.
- Add an explicit build step in the test job.
- Replace
npm testwith a Vitest invocation that excludestest/aspire-integration.test.ts.
| - name: Build | ||
| run: npm run build | ||
|
|
There was a problem hiding this comment.
The test job now runs npm run build even though there is already a separate build job and no artifacts are being passed between jobs. Since each job runs on a fresh runner, this means the project is built twice with no shared output. Consider either removing the standalone build job, or removing this duplicate build step (or uploading build artifacts from the build job and downloading them here if tests truly need the built output).
| - name: Build | |
| run: npm run build |
| run: npm run build | ||
|
|
||
| - name: Run tests (skip Playwright — covered by Squad CI) | ||
| run: npx vitest run --exclude 'test/aspire-integration.test.ts' |
There was a problem hiding this comment.
This workflow runs tests via npx vitest ..., which bypasses the repo’s npm test script (package.json defines test as vitest run). Using npm test (passing the exclude via --) keeps CI aligned with the standard entrypoint and avoids any behavioral differences from npx resolution.
| run: npx vitest run --exclude 'test/aspire-integration.test.ts' | |
| run: npm test -- --exclude 'test/aspire-integration.test.ts' |
| strategy: | ||
| matrix: | ||
| node-version: [20] | ||
| node-version: [22] |
There was a problem hiding this comment.
The PR description says the change is swapping the test command, but this workflow also bumps the Node version from 20 to 22 for multiple jobs. Please update the PR description to mention the Node upgrade (or split it into a separate PR) so reviewers understand the full CI surface-area change.
| run: npm run build | ||
|
|
||
| - name: Run tests (skip Playwright — covered by Squad CI) | ||
| run: npx vitest run --exclude 'test/aspire-integration.test.ts' |
There was a problem hiding this comment.
Skipping the Docker/Playwright integration via a hardcoded --exclude test/aspire-integration.test.ts is brittle (a rename/move could accidentally re-enable it in the publish workflow). Since the test already supports SKIP_DOCKER_TESTS=1, consider setting that env var for this step so the workflow intent stays stable even if the file path changes.
| run: npx vitest run --exclude 'test/aspire-integration.test.ts' | |
| env: | |
| SKIP_DOCKER_TESTS: '1' | |
| run: npx vitest run |
|
Closing as stale. Reason: CI fix PR with merge conflicts and failures. The fixes have likely been applied via other PRs. |
The insider publish workflow was running all tests including \�spire-integration.test.ts\ which requires Playwright/Docker — unnecessary since Squad CI already covers these.
Change: In \squad-insider-publish.yml, changed
pm test\ to
px vitest run --exclude 'test/aspire-integration.test.ts'.
Why: The Playwright-dependent test (\�spire-integration.test.ts) needs Docker + Chromium. The publish workflow only needs to verify core functionality before publishing. Full integration coverage is handled by Squad CI.
Release workflow: \squad-insider-release.yml\ uses
ode --test test/*.test.cjs\ (different runner, no Playwright dependency) — no change needed.
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com