chore(e2e): Add TSIO reporting to desktop repo - #3891
Merged
Conversation
Throwaway workflow_dispatch that runs a 2-shard Playwright suite and uploads via test-system-io-report-upload + finalizes via test-system-io-summary against TSIO staging. Validates the only unproven action in the planned CMT->TSIO migration (report-upload has zero production callers today) before wiring it into compatibility-matrix-testing.yml. Safe to delete once the spike passes and the real CMT is wired to TSIO.
|
@yasserfaraazkhan: Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsI understand the commands that are listed here |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughWorkflows now generate TSIO identities, upload Playwright JSON results, and consolidate status reporting. E2E helpers and specifications add retry logic, readiness synchronization, scoped selectors, main-process evaluation, and display-bounded resize assertions. ChangesTSIO reporting workflows
E2E stability fixes
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.github/workflows/tsio-spike.yml (2)
40-62: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winHarden the composite-identity script against silent failure.
The
run:block has noset -euo pipefail. Ifjqfails (e.g. malformed args),COMPOSITE_IDENTITYwould be empty/unset and the script would still write an emptycomposite-identity-jsonoutput rather than failing the job — silently corrupting the one contract this spike exists to validate.🛡️ Proposed fix
run: | + set -euo pipefail # `name` groups the report on the TSIO dashboard. Distinct from any real # CMT/PR context so the spike never collides with production report groups. COMPOSITE_IDENTITY=$(jq -nc \🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/tsio-spike.yml around lines 40 - 62, The Build composite identity step can fail silently if jq or any variable expansion breaks, so add strict shell handling to the run block. Update the Build composite identity step in the tsio-spike workflow to use fail-fast shell options before constructing COMPOSITE_IDENTITY, and keep the existing jq generation and GITHUB_OUTPUT write path intact. This ensures identity outputs from the identity step are not emitted when the composite identity JSON cannot be generated.
42-42: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
total-reports-expectedis decoupled from the matrix shard count.
total-reports-expected: "2"is a hardcoded literal inprepare, while the actual shard count lives in theshardsjob'smatrix.shard: [1, 2](line 75). Since the PR's explicit goal is to validate group finalization (the summary job waits for the expected report count before rendering), if either value is edited without the other, the summary would either finalize prematurely or never finalize — exactly the failure mode this spike is meant to catch. At minimum, a comment tying the two together would prevent silent drift.♻️ Proposed fix (comment-only, low risk for a throwaway spike)
outputs: composite-identity-json: ${{ steps.identity.outputs.composite-identity-json }} + # Must equal the number of entries in `shards.strategy.matrix.shard` below. total-reports-expected: "2"Also applies to: 74-75
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/tsio-spike.yml at line 42, The prepare job’s total-reports-expected value is hardcoded and can drift from the shards job matrix.shard count, so tie these two settings together to avoid premature or stalled finalization. Update the tsio-spike workflow near prepare and the shards matrix to keep the expected report count aligned with the shard list, or add an explicit comment/documentation link between total-reports-expected and matrix.shard so future edits preserve the relationship.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/tsio-spike.yml:
- Around line 40-62: The Build composite identity step can fail silently if jq
or any variable expansion breaks, so add strict shell handling to the run block.
Update the Build composite identity step in the tsio-spike workflow to use
fail-fast shell options before constructing COMPOSITE_IDENTITY, and keep the
existing jq generation and GITHUB_OUTPUT write path intact. This ensures
identity outputs from the identity step are not emitted when the composite
identity JSON cannot be generated.
- Line 42: The prepare job’s total-reports-expected value is hardcoded and can
drift from the shards job matrix.shard count, so tie these two settings together
to avoid premature or stalled finalization. Update the tsio-spike workflow near
prepare and the shards matrix to keep the expected report count aligned with the
shard list, or add an explicit comment/documentation link between
total-reports-expected and matrix.shard so future edits preserve the
relationship.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: d3c15e3e-202c-4ff4-ae7a-c811e407caaf
📒 Files selected for processing (1)
.github/workflows/tsio-spike.yml
Replaces the hand-rolled cmt-leg-*/node-rollup summary in compatibility-matrix-testing.yml with test-system-io-report-upload (per leg) + test-system-io-summary (one consolidated report per CMT run, context e2e/compatibility-matrix-testing unchanged). Adds the same TSIO reporting to e2e-functional.yml (PR/master runs) additively, under its own commit-status context (e2e-test/desktop-playwright) — does not touch the existing update-final-status/E2E-label flow or its required-checks contract. Adds the native Playwright `json` reporter output (results.json) that test-system-io-report-upload's playwright parser requires; the existing blob/line/junit reporters are unchanged. Not yet run against real TSIO. test-system-io-report-upload has zero production callers anywhere (only TSIO's own self-test) — needs one real workflow_dispatch run on a throwaway branch against a real ephemeral test server before this replaces the proven hand-rolled rollup in the actual CMT gating path.
PR #3891 reproduced this: a same-second, unrelated `labeled` event (mm-cloud-bot's release-notes label) canceled the `opened` event's in-progress run via cancel-in-progress before add-e2e-label could run, and the winning run's own job conditions didn't match that label (add-e2e-label needs action in [opened,reopened,ready_for_review, synchronize]; honor-e2e-override needs label.name == 'E2E/Override'). Net effect: E2E/Run never got added, silently. Fix: bucket the concurrency group by event instead of PR number alone. opened/reopened/ready_for_review/synchronize still share one group (preserves "only the most recent push proceeds" for rapid pushes); every other labeled/unlabeled event gets its own action+label-keyed group so it can't cancel a real trigger run.
…input cap
Run 28899437045 (Electron Playwright Tests, tsio-spike) failed with
startup_failure and zero jobs — a workflow-definition-level rejection,
not a test failure. e2e-functional-template.yml's workflow_call/
workflow_dispatch inputs had hit exactly 10 (8 existing + the 2 new
tsio-composite-identity/tsio-total-reports-expected), which is
GitHub's documented cap for reusable workflows. actionlint doesn't
check this — it's a GitHub API-side limit, not a syntax rule.
Fix: collapse the two new inputs into one JSON-packed `tsio-config`
({composite_identity, total_reports_expected}), unpacked via
fromJSON() in the report-upload step. Brings the template back to 9
inputs. Callers (compatibility-matrix-testing.yml, e2e-functional.yml)
now build that JSON with format() instead of passing two separate
`with:` fields. Jobs that call the TSIO actions directly (not through
the workflow_call boundary) are unaffected and still use the unpacked
job outputs.
github-actions
Bot
added
E2E/Run
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/e2e-functional-template.yml (1)
440-459: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard TSIO upload on the report file. Add
hashFiles('e2e/test-results/results.json') != ''to theif:so this step skips when the run fails before Playwright writes its JSON report. Thejson-pathalready matchese2e/playwright.config.ts.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-functional-template.yml around lines 440 - 459, Guard the e2e/upload-report-to-tsio step so it only runs when the Playwright JSON report exists; update the existing if condition to also check that hashFiles('e2e/test-results/results.json') is not empty, alongside always() and inputs.tsio-config != ''. Use the step name and its with block (especially json-path and gh-job-name) to locate the upload action and prevent it from running when Playwright fails before writing results.json.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/e2e-functional-template.yml:
- Around line 440-459: Guard the e2e/upload-report-to-tsio step so it only runs
when the Playwright JSON report exists; update the existing if condition to also
check that hashFiles('e2e/test-results/results.json') is not empty, alongside
always() and inputs.tsio-config != ''. Use the step name and its with block
(especially json-path and gh-job-name) to locate the upload action and prevent
it from running when Playwright fails before writing results.json.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 42982d8e-44ca-40c2-80da-9268046f64db
📒 Files selected for processing (3)
.github/workflows/compatibility-matrix-testing.yml.github/workflows/e2e-functional-template.yml.github/workflows/e2e-functional.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/e2e-functional.yml
… callers
Three issues from the tsio-spike CI run and review:
1. eslint lines-around-comment: missing blank line before the new json
reporter comment in e2e/playwright.config.ts.
2. e2e/upload-report-to-tsio could run against a results.json that was
never written (e.g. Playwright crashes before any reporter flushes).
Gate on hashFiles('e2e/test-results/results.json') != '' in addition
to the existing tsio-config check, same fix already needed in the
tsio-spike.yml spike for the identical failure mode.
3. Real blocker: "Invalid workflow file ... nested job 'e2e' is
requesting 'actions: read, id-token: write', but is only allowed
'actions: none, id-token: none'". A reusable workflow's job can only
receive permissions the CALLING job already holds. e2e-tests
(e2e-functional.yml) and e2e (compatibility-matrix-testing.yml) both
call e2e-functional-template.yml without granting those scopes
themselves. Added explicit `permissions: contents: read, actions:
read, id-token: write` to both calling jobs — exactly matching what
the template's own e2e job declares, no more. Fixed both callers
even though only the PR path has actually been exercised yet, since
compatibility-matrix-testing.yml has the identical structure and
would hit the identical error on its first real CMT dispatch.
github-actions
Bot
added
E2E/Run
"Can't find 'action.yml' ... under .github/actions/cancel-e2e-runs" — pre-existing bug, surfaced now that the concurrency-race fix lets unlabeled events actually reach this job for real. Root cause: two checkouts into the same default path. The first does a full checkout of base.ref (includes .github/actions/cancel-e2e-runs/). The second sparse-checks out head.sha with only e2e/utils/github-actions.js — which replaces the whole working tree per its own sparse rules, deleting cancel-e2e-runs/ moments before the next step tries to `uses:` it locally. Fix: sparse-checkout the untrusted head ref into its own path (untrusted-head/) instead of the default workspace, then copy just that one file over the base-ref version. Preserves the original security intent (only that one file ever comes from the untrusted PR branch; this job runs with actions:write/statuses:write) while no longer clobbering the trusted local action definition.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
.github/workflows/e2e-functional.yml (1)
165-170: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon this checkout.Same as the compat-matrix summary job: only the helper file is fetched, and the token isn't used for git in the subsequent
github-scriptstep. Disabling persistence resolves the zizmor artipacked warning.🛡️ Proposed change
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: ref: ${{ inputs.version_name }} + persist-credentials: false sparse-checkout: | e2e/utils/tsio-report-status.js sparse-checkout-cone-mode: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-functional.yml around lines 165 - 170, The checkout in this workflow step should not persist git credentials because only the helper file is needed and the later github-script step does not use git auth. Update the actions/checkout usage in this job to set persist-credentials to false, matching the same pattern used in the compat-matrix summary job, and keep the sparse-checkout behavior unchanged.Source: Linters/SAST tools
.github/workflows/compatibility-matrix-testing.yml (1)
170-175: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winSet
persist-credentials: falseon this checkout.This checkout only fetches a single helper file; the persisted
GITHUB_TOKENis never used for git operations in the followinggithub-scriptstep (which supplies its own token). Disabling credential persistence closes the artipacked posture gap flagged by zizmor.🛡️ Proposed change
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: ref: ${{ inputs.DESKTOP_VERSION }} + persist-credentials: false sparse-checkout: | e2e/utils/tsio-report-status.js sparse-checkout-cone-mode: false🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/compatibility-matrix-testing.yml around lines 170 - 175, The checkout step for the helper file should disable credential persistence to avoid keeping an unused GITHUB_TOKEN around. Update the actions/checkout usage in the compatibility matrix workflow to set persist-credentials to false alongside the existing ref and sparse-checkout settings, since the following github-script step uses its own token and does not need git auth from this checkout.Source: Linters/SAST tools
.github/workflows/e2e-pr-trigger.yml (1)
224-239: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winFix is correctly scoped; consider
persist-credentials: falseon the untrusted-head checkout.Isolating the untrusted head into
untrusted-head/and copying onlygithub-actions.jsback correctly preserves the base-ref.github/actions/cancel-e2e-runs/tree that the step at Line 242 consumes — and it's rightly applied only here (theadd-e2e-label/honor-e2e-overridejobs invoke the util viagithub-script, not a local action, so their in-workspace sparse checkout is harmless).Since this checkout targets untrusted PR head and no git operation follows, adding
persist-credentials: falseavoids leaving the token on disk alongside untrusted code (zizmor artipacked).🛡️ Proposed change
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 if: github.event.pull_request.head.repo.full_name == github.repository with: ref: ${{ github.event.pull_request.head.sha }} + persist-credentials: false sparse-checkout: | e2e/utils/github-actions.js sparse-checkout-cone-mode: false path: untrusted-head🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/e2e-pr-trigger.yml around lines 224 - 239, The untrusted-head checkout in the e2e-pr-trigger workflow should disable credential persistence so the token is not written to disk next to untrusted PR code. Update the actions/checkout step that uses github.event.pull_request.head.sha and sparse-checkout for e2e/utils/github-actions.js to include persist-credentials: false, keeping the rest of the isolated-copy flow unchanged.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In @.github/workflows/compatibility-matrix-testing.yml:
- Around line 170-175: The checkout step for the helper file should disable
credential persistence to avoid keeping an unused GITHUB_TOKEN around. Update
the actions/checkout usage in the compatibility matrix workflow to set
persist-credentials to false alongside the existing ref and sparse-checkout
settings, since the following github-script step uses its own token and does not
need git auth from this checkout.
In @.github/workflows/e2e-functional.yml:
- Around line 165-170: The checkout in this workflow step should not persist git
credentials because only the helper file is needed and the later github-script
step does not use git auth. Update the actions/checkout usage in this job to set
persist-credentials to false, matching the same pattern used in the
compat-matrix summary job, and keep the sparse-checkout behavior unchanged.
In @.github/workflows/e2e-pr-trigger.yml:
- Around line 224-239: The untrusted-head checkout in the e2e-pr-trigger
workflow should disable credential persistence so the token is not written to
disk next to untrusted PR code. Update the actions/checkout step that uses
github.event.pull_request.head.sha and sparse-checkout for
e2e/utils/github-actions.js to include persist-credentials: false, keeping the
rest of the isolated-copy flow unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7b3e19f0-9b9a-48e8-8a68-80fd3d12ac3c
📒 Files selected for processing (4)
.github/workflows/compatibility-matrix-testing.yml.github/workflows/e2e-functional.yml.github/workflows/e2e-pr-trigger.ymle2e/utils/tsio-report-status.js
github-actions
Bot
added
E2E/Run
mm-cloud-bot
added
release-note-none
and removed
do-not-merge/release-note-label-needed
labels
yasserfaraazkhan
changed the title
Contributor
Author
|
@CodeRabbit approve |
Contributor
✅ Action performedComments resolved and changes approved. |
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
Playwright kept waiting up to 90s when app.close() never settled after SIGKILL on the fast-teardown path. Request app.exit() first on Linux, wait for close() to settle after force-kill, pkill stray Electron PIDs at worker/global cleanup, use one CI worker on Linux, and register focus suite PIDs for reaping. Co-authored-by: Cursor <cursoragent@cursor.com>
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
Build commit-status and rollup links as
/reports/desktop/{branch}/{shortSha}/desktop-pr instead of
pr-{number} paths with gh_run_id query params.
Co-authored-by: Cursor <cursoragent@cursor.com>
devinbinnie
approved these changes
Jul 13, 2026
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
4 participants
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
Desktop E2E runs used to surface several small, easy-to-miss status checks on a PR, and there was no single place to see that tests were still in progress. Reviewers had to open multiple jobs to see what passed or failed. Playwright E2E reports are uploaded to Test System IO (TSIO). Each run now publishes one commit status
e2e-test/desktop-playwright.Similar reporting has been applied to
CMTandmasterruns. PR and master sharee2e-test/desktop-playwright. CMT usese2e/compatibility-matrix-testingstatus check. The TSIO report name (desktop-pr vs desktop-master vs cmt-desktop) is what tells them apart inside Test System IO.E2E app-readiness is registered earlier in main-window startup to avoid flaky waitForAppReady timeouts, Linux worker teardown no longer hangs after a green run, and new Playwright coverage was added for tray restore and the Help menu “Learn More” flow.
Release Note
Change Impact: Medium 🟠
Regression Risk: Medium—primarily CI/workflow contract changes: TSIO opt-in gating/config plumbing, consolidated final status reporting via new OIDC-based polling, removal of previous per-OS flaky/JUnit analysis, Playwright reporter switch (JUnit→JSON), and a shared commit-status utility behavior change (single status context). These can break or misreport status checks/links without affecting app runtime, but the blast radius spans multiple E2E workflows and job paths.
QA Recommendation: Validate end-to-end on the affected workflow variants (desktop functional + policy, and PR trigger) for both TSIO-enabled and TSIO-disabled legs. Specifically confirm: pending→final commit status transitions, correct commit-status context name, TSIO report link/rollup for pass/fail (including upstream-failure scenarios), and that Playwright non-zero exits fail only under the intended TSIO-disabled conditions. Only light manual QA is recommended beyond reviewing CI logs/artifacts and resulting commit statuses.
Generated by CodeRabbitAI