Skip to content

Fix: don't cancel the sibling matrix legs when one OS fails (Part 2/2) - #652

Open
AmaadMartin wants to merge 1 commit into
fix/app-loader-discovery-test-timeoutfrom
fix/app-loader-discovery-test-timeout-ci-failfast
Open

Fix: don't cancel the sibling matrix legs when one OS fails (Part 2/2)#652
AmaadMartin wants to merge 1 commit into
fix/app-loader-discovery-test-timeoutfrom
fix/app-loader-discovery-test-timeout-ci-failfast

Conversation

@AmaadMartin

@AmaadMartin AmaadMartin commented Aug 4, 2026

Copy link
Copy Markdown
Owner

Please ensure you have read the contribution guide before creating a pull request.

Link to Issue or Description of Change

  1. Link to an existing issue (if applicable):
    No public issue. Stacked on Fix: stop the app_loader discovery test billing its fixture setup to the first it() (Part 1/2) #506 (Part 1/2), which fixes the app_loader integration-test timeouts themselves; this part is the CI-side half and is split out so it can be judged on its own merits.
  2. Or, if no issue exists, describe the change:

Problem: run-tests runs a three-OS matrix (ubuntu-latest, windows-latest, macos-latest) under GitHub's default fail-fast: true. The first leg to fail cancels the other two, so an intermittent per-OS failure costs the whole matrix result rather than its own leg: the run reports nothing about the OSes that were fine, and the usual response — re-run everything — pays for all three again.

This is what made the app_loader discovery flake so expensive. It failed on macos-latest and windows-latest while ubuntu-latest was healthy, and each occurrence cancelled the healthy leg too, leaving an inconclusive run.

Solution: set fail-fast: false on the run-tests strategy. Each OS now reports its own result, so an intermittent failure on one runner is visible as an intermittent failure on one runner, and a re-run is only needed for the leg that failed.

The tradeoff, stated plainly: a genuinely broken PR now burns all three runners to completion instead of being cut short after the first failure. That is the intended trade — one flake no longer destroys the signal from the other two OSes — but it is a repo-wide policy change with a real cost, which is exactly why it is its own PR instead of a hunk inside a test fix.

Nothing else in the workflow changes: the OS list, the env block and every step are untouched.

Testing Plan

Please describe the tests that you ran to verify your changes. This is required for all PRs that are not small documentation or typo fixes.

Unit Tests:
[x] I have added or updated unit tests for my change.
[x] All unit tests pass locally.

There is no code here to unit-test — the change is three lines of GitHub Actions configuration (one key plus a two-line comment), and matrix cancellation semantics cannot be exercised from a test runner. It is verified structurally instead: the workflow parses, and the run-tests strategy resolves to exactly

{"fail-fast":false,"matrix":{"os":["ubuntu-latest","windows-latest","macos-latest"]}}

with the OS list and the env block byte-identical to main. npm run format:check globs **/*.ts only, so this file is outside it; its existing quoting is left exactly as-is rather than reflowed into the diff.

CI on this PR is absent, not green. The validation workflow is on: pull_request: branches: [main], and this PR targets the Part 1 branch, so no test job triggers for it — only auto-assign, which validates nothing. Validated locally on the pushed commit 14ce6cc3 instead:

git diff fix/app-loader-discovery-test-timeout --stat
  -> .github/workflows/validation.yaml | 3 +++      (1 file, +3, nothing else)

node -e '<parse both revisions, compare with fail-fast removed>'
  -> strategy: {"fail-fast":false,"matrix":{"os":["ubuntu-latest","windows-latest","macos-latest"]}}
  -> everything except fail-fast is byte-identical to base b390217e

npm run lint          -> clean (eslint "**/*.ts", no output)
npm run build         -> succeeds, all workspaces
npx vitest run --project integration tests/integration/app_loader/app_loader_test.ts
  -> Test Files 1 passed (1) | Tests 6 passed (6)   Duration 283.37s
     app_ts 5396ms | app_js 6142ms | app_default 5460ms | discovery 3ms / 1ms / 1ms

The behavioural proof is the CI run on the parent PR, whose tree differs from this one by exactly the three YAML lines above: Part 1's validation run is green on all three legs (macos-latest 6m42s, ubuntu-latest 5m39s, windows-latest 9m27s), so this change is stacked on a matrix that is currently healthy, and the next intermittent failure will show the difference — one red leg and two conclusive green ones.

Manual End-to-End (E2E) Tests:
Please provide instructions on how to manually test your changes, including any necessary setup or configuration.

Because this PR targets the Part 1 branch rather than main, the validation workflow (on: pull_request: branches: [main]) does not trigger for it — there are no CI checks to read on this PR, and that is expected rather than a failure. To observe the change directly once it is on a main-targeted branch: make one OS fail deliberately (e.g. a process.platform-guarded failing assertion) and confirm the other two legs still run to completion and report their own results, where previously they were cancelled.

Checklist

[x] I have read the CONTRIBUTING.md document.
[x] I have performed a self-review of my own code.
[x] I have commented my code, particularly in hard-to-understand areas.
[x] I have added tests that prove my fix is effective or that my feature works.
[x] New and existing unit tests pass locally with my changes.

The run-tests matrix runs three OSes with GitHub's default fail-fast, so the
first leg to fail cancels the other two and the run yields no signal from the
OSes that were fine. An intermittent per-OS timeout therefore costs the whole
matrix result, not just its own leg.

The trade is that a genuinely broken PR now burns all three runners instead of
being cut short after the first.
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