chore(e2e): Send Release e2e report to the channel#3902
Open
yasserfaraazkhan wants to merge 7 commits into
Open
chore(e2e): Send Release e2e report to the channel#3902yasserfaraazkhan wants to merge 7 commits into
yasserfaraazkhan wants to merge 7 commits into
Conversation
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:
📝 WalkthroughWalkthroughAdds a CMT notification utility that aggregates TSIO results, formats Mattermost messages, posts them best-effort, and wires webhook configuration into compatibility and E2E workflows. ChangesCMT Mattermost notifications
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant reportTsioStatus
participant notifyCmtChannel
participant fetchPerJobCountsFromConsolidated
participant postMattermostWebhook
reportTsioStatus->>notifyCmtChannel: invoke notification after status update
notifyCmtChannel->>fetchPerJobCountsFromConsolidated: fetch per-job counts
fetchPerJobCountsFromConsolidated-->>notifyCmtChannel: return aggregated counters
notifyCmtChannel->>postMattermostWebhook: post formatted markdown
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
Actionable comments posted: 4
🧹 Nitpick comments (1)
e2e/utils/cmt-channel-notify.js (1)
326-350: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo timeout on either
fetchcall.
fetchPerJobCountsFromConsolidatedandpostMattermostWebhookboth await barefetchwith no abort/timeout. This is a best-effort notification path — a hung TSIO or Mattermost endpoint would stall the summary step indefinitely instead of degrading gracefully.♻️ Proposed fix
- const res = await fetch(`${baseUrl}/api/v1/reports/consolidated?${params}`); + const res = await fetch(`${baseUrl}/api/v1/reports/consolidated?${params}`, { + signal: AbortSignal.timeout(10_000), + });const res = await fetch(webhookUrl, { method: 'POST', headers: {'Content-Type': 'application/json'}, body: JSON.stringify(body), + signal: AbortSignal.timeout(10_000), });Also applies to: 391-412
🤖 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 `@e2e/utils/cmt-channel-notify.js` around lines 326 - 350, Update fetch calls in fetchPerJobCountsFromConsolidated and postMattermostWebhook to use an AbortController-based timeout, ensuring hung TSIO or Mattermost requests abort after a bounded interval and the existing best-effort error handling allows the summary flow to continue.
🤖 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.
Inline comments:
In `@e2e/utils/cmt-channel-notify.js`:
- Around line 194-203: Update formatLegResultText to handle legs with passed ===
0, failed === 0, and skipped > 0 before the passed-status formatting, rendering
them as not executed rather than ✅ 0/0. Preserve the existing missing,
no-results, and normal passed/failed output behavior.
- Around line 250-264: The formatCmtChannelMessage flow must account for failed
TSIO shards when computing overallFailed. Thread the existing hasFailures or
overallState value from tsio-report-status.js into formatCmtChannelMessage and
include it in the failure condition, preserving the current checks for test
failures, incomplete status, and upstream job failures.
In `@e2e/utils/cmt-channel-notify.test.js`:
- Around line 4-6: Move the cmt-channel-notify test from the utils location into
the Playwright-discovered e2e/specs tree, and rename it with a .test.ts
extension so the existing testDir and testMatch configuration includes it.
Preserve the test’s current behavior and assertions.
In `@e2e/utils/tsio-report-status.js`:
- Around line 239-261: Wrap the entire channel-notify wiring in a try/catch,
including require('./cmt-channel-notify.js'), resolveWebhookUrl,
channelReportUrl computation, and notifyCmtChannel invocation. Preserve the
existing notification behavior on success, but swallow and optionally log any
notification setup or delivery error so reportTsioStatus remains successful
after the commit status is written.
---
Nitpick comments:
In `@e2e/utils/cmt-channel-notify.js`:
- Around line 326-350: Update fetch calls in fetchPerJobCountsFromConsolidated
and postMattermostWebhook to use an AbortController-based timeout, ensuring hung
TSIO or Mattermost requests abort after a bounded interval and the existing
best-effort error handling allows the summary flow to continue.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 257ed8f2-df58-4025-865f-cea26f87b977
📒 Files selected for processing (5)
.github/workflows/compatibility-matrix-testing.yml.github/workflows/e2e-functional.ymle2e/utils/cmt-channel-notify.jse2e/utils/cmt-channel-notify.test.jse2e/utils/tsio-report-status.js
Comment on lines
+4
to
+6
| const {describe, it} = require('node:test'); | ||
| const assert = require('node:assert/strict'); | ||
|
|
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd package.json e2e
cat e2e/package.json 2>/dev/null
echo "--- test.js files under e2e ---"
fd -e test.js . e2e
echo "--- any jest config in e2e ---"
fd -i jest.config . e2eRepository: mattermost/desktop
Length of output: 1396
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "--- e2e Playwright config files ---"
fd -a -H 'playwright.config.*' e2e
echo "--- root Playwright config files ---"
fd -a -H 'playwright.config.*' .
echo "--- inspect likely config ---"
for f in $(fd -a -H 'playwright.config.*' e2e .); do
echo "### $f"
cat -n "$f"
doneRepository: mattermost/desktop
Length of output: 12187
Move this test into Playwright’s test tree e2e/package.json runs playwright test with testDir: './specs' and testMatch: '**/*.test.ts', so e2e/utils/cmt-channel-notify.test.js is not picked up. Rename or move it under e2e/specs, or update the config if it needs to run.
🤖 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 `@e2e/utils/cmt-channel-notify.test.js` around lines 4 - 6, Move the
cmt-channel-notify test from the utils location into the Playwright-discovered
e2e/specs tree, and rename it with a .test.ts extension so the existing testDir
and testMatch configuration includes it. Preserve the test’s current behavior
and assertions.
Source: Coding guidelines
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
e2e/utils/cmt-channel-notify.js (1)
490-501: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winUse ES module exports instead of
module.exports.This JavaScript source violates the repository rule requiring
import/exportsyntax. Replace the CommonJS export block with namedexportstatements and update callers consistently.🤖 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 `@e2e/utils/cmt-channel-notify.js` around lines 490 - 501, Replace the CommonJS module.exports block with ES module named exports for parseCmtJobName, resolveWebhookUrl, buildIndividualReportUrl, buildLegSummaries, formatLegResultText, reportTitleForIdentity, formatCmtChannelMessage, fetchPerJobCountsFromConsolidated, postMattermostWebhook, and notifyCmtChannel; update all callers of this module to use ES module import syntax consistently.Source: Coding guidelines
🤖 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.
Inline comments:
In `@e2e/utils/cmt-channel-notify.js`:
- Around line 283-286: Update the headline aggregation in buildLegSummaries to
account for detail.test_stats.flaky, matching the existing behavior that folds
flaky tests into passed totals or otherwise surfaces flaky separately. Preserve
the current defaults for passed, failed, and skipped.
- Around line 25-31: Update fetchWithTimeout so the abort timeout remains active
until the response body has been consumed, not merely until fetch() returns
headers. Adjust the flow around fetchWithTimeout and its callers to clear the
timer only after res.json()/res.text() completes, while preserving existing
timeout cancellation and response handling.
---
Outside diff comments:
In `@e2e/utils/cmt-channel-notify.js`:
- Around line 490-501: Replace the CommonJS module.exports block with ES module
named exports for parseCmtJobName, resolveWebhookUrl, buildIndividualReportUrl,
buildLegSummaries, formatLegResultText, reportTitleForIdentity,
formatCmtChannelMessage, fetchPerJobCountsFromConsolidated,
postMattermostWebhook, and notifyCmtChannel; update all callers of this module
to use ES module import syntax consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 5466aea3-d076-49e5-aa7c-f27147b0ff5c
📒 Files selected for processing (3)
e2e/utils/cmt-channel-notify.jse2e/utils/cmt-channel-notify.test.jse2e/utils/tsio-report-status.js
🚧 Files skipped from review as they are similar to previous changes (2)
- e2e/utils/cmt-channel-notify.test.js
- e2e/utils/tsio-report-status.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/utils/cmt-channel-notify.js (1)
290-347: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
overallFailedcan be true with no explanation shown to readers.
passedcorrectly foldsflakyin now (line 292), addressing the earlier aggregation gap. However,overallFailed(line 295-298) can flip totrueviahasFailuresordetail?.status !== 'completed', but only the!upstreamJobsSucceededbranch (line 345-347) renders explanatory text. If a TSIO shard fails without incrementingtest_stats.failed, the message shows "❌ Failed" withFailed: 0and no leg-level breakdown reason — leaving on-call viewers with an unexplained failure signal.♻️ Proposed fix
if (!upstreamJobsSucceeded) { lines.push('_One or more CI jobs failed outside tracked tests (install/build/teardown)._', ''); + } else if (hasFailures && failed === 0) { + lines.push('_TSIO reported failed shard(s) not reflected in the test totals; check the full report._', ''); + } + if (detail?.status && detail.status !== 'completed' && upstreamJobsSucceeded) { + lines.push(`_TSIO report status: ${detail.status}._`, ''); }🤖 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 `@e2e/utils/cmt-channel-notify.js` around lines 290 - 347, Update the summary-building flow around overallFailed and the final explanatory messages so every non-test failure cause is surfaced to readers. When overallFailed is true because detail?.status is not completed or hasFailures is true, add clear explanatory text alongside the existing upstreamJobsSucceeded message, while preserving the current test-failure and per-leg output.
🤖 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 `@e2e/utils/cmt-channel-notify.js`:
- Around line 290-347: Update the summary-building flow around overallFailed and
the final explanatory messages so every non-test failure cause is surfaced to
readers. When overallFailed is true because detail?.status is not completed or
hasFailures is true, add clear explanatory text alongside the existing
upstreamJobsSucceeded message, while preserving the current test-failure and
per-leg output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 361c35b6-1506-4863-90fb-ee53b57b280a
📒 Files selected for processing (2)
e2e/utils/cmt-channel-notify.jse2e/utils/cmt-channel-notify.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e/utils/cmt-channel-notify.test.js
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
github-actions
Bot
added
E2E/Run
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
2 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
Posts a small Mattermost summary after TSIO finishes for CMT, master, and PR E2E runs.
Channels
MM_DESKTOP_CMT_WEBHOOK_URLMM_DESKTOP_E2E_WEBHOOK_URL(already working)Reports include overall pass/fail, OS sections with per-leg counts, and links to the full TSIO report plus each leg. Policy legs show for PR/master. Notify is best-effort and runs even when the run failed.
Secret to add:
MM_DESKTOP_CMT_WEBHOOK_URLReport examples
❌ Desktop PR E2E
PR: #3891 · Branch:
fix-notifications· Commit:deadbee🔴 3 failing tests
11.9.011.9.0Detailed results
11.9.011.9.011.9.0➡️ Full report: ...
❌ Desktop CMT
Branch:
v6.2.0-rc.1· Commit:55afc0b🔴 2 failing tests
11.9.011.9.0Detailed results
10.5.1411.9.010.5.1411.9.010.5.1411.9.0➡️ Full report: ...
✅ Desktop Master E2E
Branch:
master· Commit:a1b2c3dDetailed results
11.9.011.9.011.9.0➡️ Full report: ...
Change Impact: 🟡 Medium
Regression Risk: Channel notifications are added as best-effort steps after TSIO commit status writing, and are gated to specific
compositeIdentity.namevalues; failures are caught and only emit warnings. Risk is mainly limited to workflow wiring, webhook env resolution, and notifier parsing/report-link logic in a sharede2e/utils/tsio-report-statusutility path.QA Recommendation: Skip manual QA beyond one representative run for each of CMT, master, and PR TSIO summaries, plus a check with
MM_DESKTOP_CMT_WEBHOOK_URLunset to confirm notifications are skipped while commit status flipping still succeeds. Generated by CodeRabbitAI