feat(ci): add e2e test result notifications to PR comments#1552
Open
B-Whitt wants to merge 1 commit into
Open
Conversation
📝 WalkthroughWalkthroughAdds a ChangesE2E Test Result Notification
Sequence Diagram(s)sequenceDiagram
participant Trigger as PR Comment (/run-e2e)
participant GH_Actions as GitHub Actions Workflow
participant E2E1 as API E2E Tests
participant E2E2 as Multinode E2E Tests
participant GH_API as GitHub API (Status/Deployment/Comments)
Trigger->>GH_Actions: dispatch workflow
GH_Actions->>E2E1: start api-e2e-tests
GH_Actions->>E2E2: start api-e2e-multinode-tests
E2E1-->>GH_Actions: result (success/failure/skip)
E2E2-->>GH_Actions: result (success/failure/skip)
GH_Actions->>GH_API: set commit status (context: e2e-tests) rgba(0,128,0,0.5) / rgba(128,0,0,0.5)
GH_Actions->>GH_API: update deployment status (if deployment_id)
GH_Actions->>GH_API: post PR comment with results table and conditional "Why"
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Author
|
/run-e2e |
Codecov Report✅ All modified and coverable lines are covered by tests. @@ Coverage Diff @@
## main #1552 +/- ##
=======================================
Coverage 91.99% 91.99%
=======================================
Files 241 241
Lines 10958 10958
=======================================
Hits 10081 10081
Misses 877 877
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/e2e-dispatch.yml (1)
356-361: ⚡ Quick win
triggerResult === 'failure'produces no "Why" diagnosticThe reasons block handles
'skipped'(body mismatch) but has no branch fortriggerResult === 'failure'(e.g., when the commenter lacks write access andcore.setFailedis called). The table will show:x: Failedfor Trigger Check with an empty "Why" section.🔧 Proposed fix
if (triggerResult === 'skipped') { const body = context.payload.comment.body; if (body !== '/run-e2e') { reasons.push(`Comment must be exactly \`/run-e2e\` with no extra characters. Received: \`${JSON.stringify(body)}\``); } } + + if (triggerResult === 'failure') { + reasons.push(`**Trigger Check** failed — the commenter may not have write access, or another check step failed. See [workflow run logs](${runUrl}).`); + }🤖 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-dispatch.yml around lines 356 - 361, Add a branch to handle triggerResult === 'failure' so the reasons array is populated when the trigger check fails: inside the same conditional that checks triggerResult, detect 'failure' and push a clear diagnostic (e.g., "Trigger check failed: commenter likely lacks write/permission or core.setFailed was called; see workflow logs") into reasons; if the code exposes an error variable from the trigger step (e.g., triggerError or similar), include that error text in the pushed reason; reference the existing triggerResult and reasons variables to locate where to add this branch.
🤖 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 @.github/workflows/e2e-dispatch.yml:
- Around line 334-336: The workflow currently grants only issues: read which
prevents github.rest.issues.createComment from succeeding; update the
permissions block so that issues is set to write (e.g., change "issues: read" to
"issues: write") so the GITHUB_TOKEN has permission for the Issues API used by
github.rest.issues.createComment; keep pull-requests: write unchanged.
- Around line 329-332: The notify-result-e2e job's if condition only checks
github.event_name and startsWith(...) so it still runs on non-PR issue comments;
update the notify-result-e2e job's if expression to include the same PR guard
used by check-trigger (github.event.issue.pull_request) so the job only runs for
PR-linked comments (i.e., change the if to require always() && github.event_name
== 'issue_comment' && github.event.issue.pull_request &&
startsWith(github.event.comment.body, '/run-e2e')); alternatively, if you
intentionally want notifications for non-PR comments, add a clear "non-PR"
reason into the triggerResult === 'skipped' branch so the posted table includes
a diagnostic message.
---
Nitpick comments:
In @.github/workflows/e2e-dispatch.yml:
- Around line 356-361: Add a branch to handle triggerResult === 'failure' so the
reasons array is populated when the trigger check fails: inside the same
conditional that checks triggerResult, detect 'failure' and push a clear
diagnostic (e.g., "Trigger check failed: commenter likely lacks write/permission
or core.setFailed was called; see workflow logs") into reasons; if the code
exposes an error variable from the trigger step (e.g., triggerError or similar),
include that error text in the pushed reason; reference the existing
triggerResult and reasons variables to locate where to add this branch.
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 5e61c764-6f25-4f9a-bc53-c158474060c3
📒 Files selected for processing (2)
.github/workflows/e2e-dispatch.yml.gitignore
B-Whitt
force-pushed
the
fix/e2e-notify-result
branch
4 times, most recently
from
c308afa to
6140dc3
Compare
Add combined e2e test result reporting to the notify-result-e2e job: - Deployment status: shows "All e2e tests have passed" box on PR conversation tab after both test jobs complete - Commit status: adds e2e-tests line item to the PR checks list, enabling branch protection rules for e2e gating - Reply to every /run-e2e PR comment with a status table Assisted-by: Claude Opus
B-Whitt
force-pushed
the
fix/e2e-notify-result
branch
from
6140dc3 to
e4dbec2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/e2e-dispatch.yml:
- Around line 340-358: The workflow unconditionally calls
github.rest.repos.createCommitStatus using needs.check-trigger.outputs.pr_sha
which can be empty when check-trigger is skipped; update the "Set commit status"
step to first check that needs.check-trigger.outputs.pr_sha (the PR SHA) is
present and only call createCommitStatus when it is truthy (otherwise skip
creating the commit status and proceed to post the result comment/diagnostics);
use the existing variables (e2e, multinode, allPassed) and keep the same payload
construction for createCommitStatus when invoked.
- Around line 423-431: The current logic only appends the status table to body
when allPassed is false; change the block so the three status rows using
statusIcon(triggerResult), statusIcon(e2eResult), and
statusIcon(multinodeResult) are appended unconditionally (always add to body),
and keep the conditional that appends the “Why” section only when reasons.length
> 0; update the code around the allPassed check to remove the table from the
conditional while preserving the existing reasons conditional and using the same
variables (allPassed, body, triggerResult, e2eResult, multinodeResult, reasons,
statusIcon).
🪄 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: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: d5b2a35f-e414-4a99-9a1e-fe6567bf9ab5
📒 Files selected for processing (1)
.github/workflows/e2e-dispatch.yml
Comment on lines
+340
to
+358
| - name: Set commit status | ||
| uses: actions/github-script@v9 | ||
| with: | ||
| script: | | ||
| const e2e = '${{ needs.api-e2e-tests.result }}'; | ||
| const multinode = '${{ needs.api-e2e-multinode-tests.result }}'; | ||
| const allPassed = e2e === 'success' && multinode === 'success'; | ||
|
|
||
| await github.rest.repos.createCommitStatus({ | ||
| owner: context.repo.owner, | ||
| repo: context.repo.repo, | ||
| sha: '${{ needs.check-trigger.outputs.pr_sha }}', | ||
| state: allPassed ? 'success' : 'failure', | ||
| context: 'e2e-tests', | ||
| description: allPassed | ||
| ? 'All e2e tests passed' | ||
| : 'E2e tests failed', | ||
| target_url: `https://github.com/${context.repo.owner}/${context.repo.repo}/actions/runs/${context.runId}` | ||
| }); |
There was a problem hiding this comment.
Guard commit-status creation when trigger check is skipped.
When check-trigger is skipped (e.g., /run-e2e ), needs.check-trigger.outputs.pr_sha is empty, so createCommitStatus can fail before the result comment is posted. That defeats the “always reply with diagnostics” flow.
🔧 Suggested fix
- name: Set commit status
+ if: needs.check-trigger.outputs.pr_sha != ''
uses: actions/github-script@v9
with:
script: |🤖 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-dispatch.yml around lines 340 - 358, The workflow
unconditionally calls github.rest.repos.createCommitStatus using
needs.check-trigger.outputs.pr_sha which can be empty when check-trigger is
skipped; update the "Set commit status" step to first check that
needs.check-trigger.outputs.pr_sha (the PR SHA) is present and only call
createCommitStatus when it is truthy (otherwise skip creating the commit status
and proceed to post the result comment/diagnostics); use the existing variables
(e2e, multinode, allPassed) and keep the same payload construction for
createCommitStatus when invoked.
Comment on lines
+423
to
+431
| if (!allPassed) { | ||
| body += `| Trigger Check | ${statusIcon(triggerResult)} |\n`; | ||
| body += `| API E2E Tests | ${statusIcon(e2eResult)} |\n`; | ||
| body += `| Multinode E2E Tests | ${statusIcon(multinodeResult)} |\n`; | ||
|
|
||
| if (reasons.length > 0) { | ||
| body += `\n**Why:**\n\n${reasons.map(r => '- ' + r).join('\n')}\n`; | ||
| } | ||
| } |
There was a problem hiding this comment.
Render the status table on successful runs too.
The table is currently omitted when all checks pass, but this feature is expected to always post the results matrix. Make the table unconditional and keep “Why” conditional.
🔧 Suggested fix
- if (!allPassed) {
- body += `| Trigger Check | ${statusIcon(triggerResult)} |\n`;
- body += `| API E2E Tests | ${statusIcon(e2eResult)} |\n`;
- body += `| Multinode E2E Tests | ${statusIcon(multinodeResult)} |\n`;
+ body += `| Check | Result |\n`;
+ body += `| --- | --- |\n`;
+ body += `| Trigger Check | ${statusIcon(triggerResult)} |\n`;
+ body += `| API E2E Tests | ${statusIcon(e2eResult)} |\n`;
+ body += `| Multinode E2E Tests | ${statusIcon(multinodeResult)} |\n`;
+ if (!allPassed) {
if (reasons.length > 0) {
body += `\n**Why:**\n\n${reasons.map(r => '- ' + r).join('\n')}\n`;
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (!allPassed) { | |
| body += `| Trigger Check | ${statusIcon(triggerResult)} |\n`; | |
| body += `| API E2E Tests | ${statusIcon(e2eResult)} |\n`; | |
| body += `| Multinode E2E Tests | ${statusIcon(multinodeResult)} |\n`; | |
| if (reasons.length > 0) { | |
| body += `\n**Why:**\n\n${reasons.map(r => '- ' + r).join('\n')}\n`; | |
| } | |
| } | |
| body += `| Check | Result |\n`; | |
| body += `| --- | --- |\n`; | |
| body += `| Trigger Check | ${statusIcon(triggerResult)} |\n`; | |
| body += `| API E2E Tests | ${statusIcon(e2eResult)} |\n`; | |
| body += `| Multinode E2E Tests | ${statusIcon(multinodeResult)} |\n`; | |
| if (!allPassed) { | |
| if (reasons.length > 0) { | |
| body += `\n**Why:**\n\n${reasons.map(r => '- ' + r).join('\n')}\n`; | |
| } | |
| } |
🤖 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-dispatch.yml around lines 423 - 431, The current logic
only appends the status table to body when allPassed is false; change the block
so the three status rows using statusIcon(triggerResult), statusIcon(e2eResult),
and statusIcon(multinodeResult) are appended unconditionally (always add to
body), and keep the conditional that appends the “Why” section only when
reasons.length > 0; update the code around the allPassed check to remove the
table from the conditional while preserving the existing reasons conditional and
using the same variables (allPassed, body, triggerResult, e2eResult,
multinodeResult, reasons, statusIcon).
|
wfealdel
reviewed
May 7, 2026
| deployments: write | ||
| statuses: write | ||
| steps: | ||
| - name: Set commit status |
There was a problem hiding this comment.
@B-Whitt
This step uses same variables as the Update deployment status, consider if it's possible to merge to both steps into one script that does both the commit status and deployment status API calls
Member
|
/run-atf-tests |
Member
|
@B-Whitt we got a PR merged a few days ago, #1544 , which adds the ability of running ATF tests on demand, by similarly commenting Maybe we could have something similar for |
|
| Metric | Count |
|---|---|
| Total Tests | 0 |
| ✅ Passed | 0 |
| ❌ Failed | 0 |
| 0 | |
| ⏭️ Skipped | 0 |
| ⏱️ Duration | 1.00s |
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
5 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
notify-result-e2ejob to thee2e-dispatch.ymlworkflow that replies to every/run-e2ePR comment with the outcomestartsWithon the comment body so near-miss triggers (e.g.,/run-e2e\r) still get feedbackTest plan
/run-e2eon a PR and verify the result comment is posted after all jobs complete/run-e2e(with trailing space) and confirm the reply explains the body mismatchcheck-trigger,api-e2e-tests, andapi-e2e-multinode-testsjobs are unaffected🤖 Generated with Claude Code
Summary by CodeRabbit