ci: stop swallowing test failures in Build + CI workflows#212
Conversation
|
Heads-up on the two failing checks ( Fix is queued as #215 which adds |
github-actions
Bot
added
the
maintainer-engaged
|
Good catch Jay, This is important to get right before we add anything else, can you resolve the build issues and I'll merge |
The Build (`build.yml`) and CI (`main.yml`) workflows ran `npm ci` at the repo root and then `npm test`, which delegates to `cd server && npm run test`. That test script exec's `node_modules/.bin/jest` inside `server/` — but the root `npm ci` only installs the root package.json's deps, so `server/node_modules/` was empty on the runner and jest was never present. The failure only surfaced now because the same `npm test` line carried a `|| echo "No tests configured"` fallback (removed in #212), which made the test step exit 0 even though jest failed to resolve. The macos-latest cell of the Build matrix has been failing silently on main for the last 12+ days for this same reason. The `pr_request.yml` "test" job already does the two-step install (`npm ci` at root, then `npm ci` inside `server/`), and its test job passes on every PR. This PR ports that pattern to `build.yml` and `main.yml` so their test steps have the same dependency state. Publish job in `build.yml` is unchanged: it only runs `npm run build --if-present` and does not exercise the server test suite, so it does not need server-side install. Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
Both `.github/workflows/main.yml` and `.github/workflows/build.yml` ran `npm test || echo "No tests configured"`. The `|| echo` makes the shell exit 0 whenever the test suite fails, so a red test run reports success and the failing job goes green. The Node-version + OS build matrix in `build.yml` (Node 22.x/24.x on ubuntu/macos/windows) magnifies the blindspot to six matrix cells at a time. `npm test` at the repo root delegates to `cd server && npm run test` which is defined and passes locally; there is no scenario where the safety net is doing useful work. Dropping the `|| echo` on both workflows so a failing test suite fails the job. Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
aec45a6 to
7705a48
Compare
|
Thanks @niallroche. Rebased on post-#215 main so the server-install fix is in the base now. CI running fresh, should clear green this time. Ready for merge whenever. |
Previously `server/npm test` was:
node --experimental-vm-modules node_modules/.bin/jest
On Windows runners, `node_modules/.bin/jest` is a Git-Bash shell-script
wrapper. Passing it as an entry point to `node` fails with:
SyntaxError: missing ) after argument list
basedir=$(dirname "$(echo "$0" | sed -e 's,\\,/,g')")
because Node tries to parse the shell script as JavaScript. macOS and
Linux runners had it working only because their `.bin/jest` is a symlink
to the actual JS file.
Point directly at `node_modules/jest/bin/jest.js` (the JS entry the
shell wrapper eventually delegates to). Same runtime behavior on all
three OSes, no more wrapper confusion.
This is the actual failure that the rest of this PR (removing
`|| echo "No tests configured"`) surfaced on the Windows matrix cells.
It was silently swallowed on main for the last 12+ days.
Signed-off-by: Jay Guwalani <guwalanijj@gmail.com>
|
Pushed the actual Windows fix. |
2 participants
Summary
Both
.github/workflows/main.ymland.github/workflows/build.ymlrannpm test || echo "No tests configured". The|| echomakes the shell exit 0 whenever the test suite fails, so a red test run reports success and the failing job goes green.Why this matters
build.ymlruns the tests across a Node-version × OS matrix (Node 22.x + 24.x on ubuntu/macos/windows, six cells). Every one of those cells has been reporting success on failing tests. If a real test regression landed onmain, no cell would fail the build.What this PR does
Drops the
|| echo "No tests configured"fallback on both workflow files.npm testat the repo root delegates tocd server && npm run test, which is defined and passes locally on the current main. There is no scenario where the safety net is doing useful work.Validation
npm testat the repo rootnpm testmaps tocd server && npm run test, currently 121 tests passing across 9 suitesAuthor Checklist
npm testverified to run and pass on current main