ci: workflow to add comment for e2e test changes#14797
Open
ypolishchuk-ledger wants to merge 1 commit intodevelopfrom
Open
ci: workflow to add comment for e2e test changes#14797ypolishchuk-ledger wants to merge 1 commit intodevelopfrom
ypolishchuk-ledger wants to merge 1 commit intodevelopfrom
Conversation
Contributor
❌ Action Required: Monitored Files ChangedThe following files in monitored folders have been modified:
Action Required: Please rebase your branch against git rebase origin/develop |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR adds a new GitHub Actions workflow that automatically posts an E2E test checklist comment when pull requests modify files in the e2e/ directory. The workflow detects which platforms (Desktop/Mobile) are affected and provides tailored guidance including device coverage requirements, test quality standards, and workflow trigger instructions.
Changes:
- New workflow file that triggers on PR opened/synchronize events for e2e/ path changes
- Detects Desktop vs Mobile E2E changes and creates platform-specific checklists
- Test file includes a placeholder comment (appears to be for testing the workflow trigger)
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
.github/workflows/pr-e2e-template.yml |
New workflow that posts E2E test checklists to PRs with device coverage tables and test quality reminders |
e2e/desktop/tests/specs/earn.spec.ts |
Added a test comment, likely to trigger the workflow for demonstration purposes |
Comments suppressed due to low confidence (3)
.github/workflows/pr-e2e-template.yml:50
- If neither desktopChanged nor mobileChanged is true, the platformLabel will incorrectly default to 'Mobile' due to the ternary operator chain. This could occur if files under e2e/ are changed but not in the desktop/ or mobile/ subdirectories (e.g., shared configuration files). Consider adding a check and either skipping the comment creation when no platform is detected, or using a more appropriate label like 'E2E Infrastructure' for such cases.
const platformLabel = desktopChanged && mobileChanged
? 'Desktop & Mobile'
: desktopChanged ? 'Desktop' : 'Mobile';
e2e/desktop/tests/specs/earn.spec.ts:73
- This test comment should be removed before merging. It appears to be leftover debugging or placeholder code that does not serve any documentation purpose.
//test
.github/workflows/pr-e2e-template.yml:30
- The per_page parameter is set to 100, which may not be sufficient for PRs with many files (the e2e directory could have more than 100 files changed). This could result in missing files during platform detection. Consider either increasing this value or adding pagination similar to how the comment listing is handled later in the workflow (lines 111-117).
const { data: files } = await github.rest.pulls.listFiles({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
per_page: 100
});
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
.github/workflows/pr-e2e-template.yml:30
- The
per_page: 100parameter inlistFilesonly retrieves the first 100 changed files. If a PR modifies more than 100 files, the platform detection logic may miss e2e changes outside the first page. Consider using pagination here as well, similar to how the comment search usesgithub.paginate.iterator, or use the simplergithub.paginateAPI to automatically handle all pages.
const { data: files } = await github.rest.pulls.listFiles({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
per_page: 100
});
.github/workflows/pr-e2e-template.yml:64
- The device names in the table use inconsistent casing compared to the actual device tags shown in line 99. The table shows lowercase/camelCase ("nanoS", "nanoSP", "nanoX", "stax", "flex", "nanoGen5") but the actual tags are PascalCase ("@nanosp", "@nanox", "@Stax", "@flex", "@NanoGen5"). Additionally, "nanoS" is missing from the tag list in line 99. The table should either match the tag format exactly or clearly indicate the mapping between display names and tags to avoid confusion.
const devices = ['nanoS', 'nanoSP', 'nanoX', 'stax', 'flex', 'nanoGen5'];
.github/workflows/pr-e2e-template.yml:135
- The logic for detecting whether to update an existing comment checks if
!botComment.body.includes(platformLabel). This means if a PR initially touches only Desktop e2e files (platformLabel = "Desktop"), then is updated to also touch Mobile files (platformLabel = "Desktop & Mobile"), the check will pass and update correctly. However, if it goes from "Desktop & Mobile" back to just "Desktop" (if mobile changes are reverted), the label "Desktop" will still be found in "Desktop & Mobile" and the comment won't update. Consider a more robust check that compares the actual platform state rather than string inclusion.
if (!botComment.body.includes(platformLabel)) {
e2e/desktop/tests/specs/earn.spec.ts:73
- The test comment "// trigger workflow" appears to be added solely to trigger this new workflow for testing purposes and should not be part of the production code. This seems like debug/test code that was left in by mistake. If this is intentional for demonstration purposes, it should be documented in the PR description or removed before merging.
// trigger workflow
.github/workflows/pr-e2e-template.yml:146
- The workflow lacks error handling around the GitHub API calls. If the API calls to create or update comments fail (due to network issues, rate limits, or permissions), the workflow will fail silently without helpful error messages. Consider adding try-catch blocks with appropriate error logging using
core.error()andcore.setFailed(), similar to the pattern used in other workflows likepr-title-from-labels.yml.
if (!botComment) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: e2eChecklist
});
console.log(`E2E checklist comment added (${platformLabel})`);
} else {
// Update existing comment if platforms changed (e.g., mobile added after desktop-only)
if (!botComment.body.includes(platformLabel)) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: botComment.id,
body: e2eChecklist
});
console.log(`E2E checklist comment updated to ${platformLabel}`);
} else {
console.log('E2E checklist comment already exists and is up to date, skipping');
}
}
.github/workflows/pr-e2e-template.yml:1
- The file name uses
-templatesuffix which is inconsistent with workflow naming conventions in this repository. Looking at similar PR-triggered workflows likepr-title-from-labels.yml, a more appropriate name would bepr-e2e-checklist.ymlto match the workflow's purpose (adding an E2E checklist to PRs) and maintain consistency with existing naming patterns.
name: "E2E PR Checklist"
.github/workflows/pr-e2e-template.yml:84
- The workflow links in the comment use relative URLs (e.g.,
../actions/workflows/...) which won't work in GitHub markdown comments. These should be absolute URLs like the pattern used innotify-e2e-required-reusable.ymlwhere they construct full URLs:https://github.com/LedgerHQ/ledger-live/actions/workflows/...
The current relative links will appear as broken links in the PR comment UI.
workflowLinks.push('- **Desktop:** Trigger [`test-ui-e2e-only-desktop.yml`](../actions/workflows/test-ui-e2e-only-desktop.yml) with `test_filter` set to your test name/tag');
}
if (mobileChanged) {
workflowLinks.push('- **Mobile:** Trigger [`test-mobile-e2e-reusable.yml`](../actions/workflows/test-mobile-e2e-reusable.yml) with `test_filter` set to your test name/tag');
ypolishchuk-ledger
force-pushed
the
e2e/ci/add-pr-comment
branch
2 times, most recently
from
a4e69c6 to
2d4e0a9
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (6)
e2e/desktop/tests/specs/earn.spec.ts:73
- This
// trigger workflowcomment appears to be a temporary change to force E2E file modifications. Please drop it (and revert any non-functional E2E spec edits) so the test suite history stays meaningful.
// trigger workflow
.github/workflows/pr-e2e-template.yml:7
- Using the
pull_requestevent will not be able to post PR comments on contributions from forks (the token is read-only), which defeats the purpose of this workflow for external contributors. Consider switching topull_request_targetand ensure you do not checkout/execute PR code (this workflow only calls the GitHub API, so it can be kept safe).
on:
pull_request:
types: [opened, synchronize]
paths:
- "e2e/**"
.github/workflows/pr-e2e-template.yml:33
pulls.listFilesis paginated, but the script only fetches the first 100 files. For PRs with >100 changed files,desktop/mobiledetection can be wrong. Usegithub.paginate(as you already do for comments) to aggregate all files before computing the flags.
const { data: files } = await github.rest.pulls.listFiles({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
per_page: 100
});
const desktop = files.some(f => f.filename.startsWith('e2e/desktop/'));
const mobile = files.some(f => f.filename.startsWith('e2e/mobile/'));
.github/workflows/pr-e2e-template.yml:51
- If a PR changes files under
e2e/but not undere2e/desktop/ore2e/mobile/(e.g., shared utilities/docs),platformLabelfalls back toMobileand the generated checklist will be incorrect. Handle the!desktopChanged && !mobileChangedcase explicitly (e.g., label asDesktop & Mobile/E2Eor skip posting).
const platformLabel = desktopChanged && mobileChanged
? 'Desktop & Mobile'
: desktopChanged ? 'Desktop' : 'Mobile';
.github/workflows/pr-e2e-template.yml:146
- The update check
botComment.body.includes(platformLabel)can incorrectly skip updates. Example: if the previous comment was forDesktop & Mobileand a later push removes all mobile changes,platformLabelbecomesDesktopand the old body still includes the substringDesktop, so the comment won't be updated and will keep the wrong columns. Prefer always updating the existing checklist onsynchronize, or compare against an unambiguous hidden marker (e.g., an HTML comment containing the computed flags).
} else {
// Update existing comment if platforms changed (e.g., mobile added after desktop-only)
if (!botComment.body.includes(platformLabel)) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: botComment.id,
body: e2eChecklist
});
console.log(`E2E checklist comment updated to ${platformLabel}`);
} else {
console.log('E2E checklist comment already exists and is up to date, skipping');
}
}
e2e/mobile/specs/market.spec.ts:35
- The added
//testcomment looks like a leftover used only to force a diff. It adds noise to an E2E spec without changing behavior; please remove it (and ideally avoid modifying specs just to trigger CI workflows).
await app.walletTabNavigator.navigateToMarket(); //test
2d4e0a9 to
95056a2
Compare
ypolishchuk-ledger
force-pushed
the
e2e/ci/add-pr-comment
branch
from
95056a2 to
7c4c3e9
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/workflows/pr-e2e-template.yml:30
- The listFiles API call may not retrieve all changed files if a PR has more than 100 files. This could cause the workflow to miss Desktop or Mobile changes in large PRs. The pagination pattern used in the second step (lines 117-123) should also be applied here to ensure all files are checked. Alternatively, use github.paginate to automatically handle pagination.
const { data: files } = await github.rest.pulls.listFiles({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
per_page: 100
});
|
ypolishchuk-ledger
force-pushed
the
e2e/ci/add-pr-comment
branch
from
7c4c3e9 to
73c1bfc
Compare
ypolishchuk-ledger
force-pushed
the
e2e/ci/add-pr-comment
branch
from
73c1bfc to
0bf7f6c
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (7)
e2e/desktop/tests/specs/earn.spec.ts:73
- The test comment '// test workflow' appears to be a temporary change solely to trigger the workflow for testing purposes. This should be removed before merging as it adds no value to the test logic and will remain as clutter in the codebase.
// test workflow
.github/workflows/pr-e2e-template.yml:144
- Missing changeset for CI workflow changes. According to the project conventions in CONTRIBUTING.md line 77, every PR must include a changeset. While this is a CI-only change, similar workflow modifications in the codebase (e.g., '.changeset/remove-e2e-continue-on-error.md') include changesets. Run 'pnpm changeset' to add one, selecting the appropriate package scope (likely 'ledger-live-desktop' as a patch).
name: "E2E PR Checklist"
on:
pull_request:
types: [opened, synchronize]
paths:
- "e2e/**"
permissions:
pull-requests: read
contents: read
jobs:
add-e2e-checklist:
name: "Add E2E Checklist"
runs-on: ubuntu-latest
permissions:
pull-requests: write
steps:
- name: Detect changed E2E platforms
id: detect
uses: actions/github-script@v7
with:
script: |
const { data: files } = await github.rest.pulls.listFiles({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
per_page: 100
});
const desktop = files.some(f => f.filename.startsWith('e2e/desktop/'));
const mobile = files.some(f => f.filename.startsWith('e2e/mobile/'));
core.setOutput('desktop', desktop.toString());
core.setOutput('mobile', mobile.toString());
- name: Add E2E checklist comment
uses: actions/github-script@v7
env:
CHANGED_DESKTOP: ${{ steps.detect.outputs.desktop }}
CHANGED_MOBILE: ${{ steps.detect.outputs.mobile }}
with:
script: |
const desktopChanged = process.env.CHANGED_DESKTOP === 'true';
const mobileChanged = process.env.CHANGED_MOBILE === 'true';
const platformLabel = desktopChanged && mobileChanged
? 'Desktop & Mobile'
: desktopChanged ? 'Desktop' : 'Mobile';
// Build device table with proof link columns per platform
const header = ['Device'];
const separator = ['--------'];
if (desktopChanged) {
header.push('Proof (Desktop)');
separator.push('---');
}
if (mobileChanged) {
header.push('Proof (Mobile)');
separator.push('---');
}
const devices = ['nanoS', 'nanoSP', 'nanoX', 'stax', 'flex', 'nanoGen5'];
const rows = devices.map(device => {
const cols = [`| ${device}`];
if (desktopChanged) cols.push('_paste link_');
if (mobileChanged) cols.push('_paste link_');
return cols.join(' | ') + ' |';
});
const deviceTable = [
'| ' + header.join(' | ') + ' |',
'| ' + separator.join(' | ') + ' |',
...rows
].join('\n');
// Build workflow trigger instructions
const workflowLinks = [];
if (desktopChanged) {
workflowLinks.push('- **Desktop:** Trigger [`test-ui-e2e-only-desktop.yml`](../actions/workflows/test-ui-e2e-only-desktop.yml) with `test_filter` set to your test name/tag');
}
if (mobileChanged) {
workflowLinks.push('- **Mobile:** Trigger [`test-mobile-e2e-reusable.yml`](../actions/workflows/test-mobile-e2e-reusable.yml) with `test_filter` set to your test name/tag');
}
const e2eChecklist = `## 🧪 E2E Test Checklist — ${platformLabel}
> This checklist was added automatically because your PR touches \`e2e/\` files.
### General
- [ ] Tests pass locally before pushing
- [ ] Tests are independent and can run in any order
### Test Quality
- [ ] Page Object Model (POM) pattern is followed for new pages/components
- [ ] Appropriate device tags are added: \`@NanoSP\`, \`@NanoX\`, \`@Stax\`, \`@Flex\`, \`@NanoGen5\`
- [ ] Family tags are added where applicable: \`@family-evm\`, \`@family-bitcoin\`, etc.
- [ ] TMS/Xray ticket IDs are linked in test annotations (e.g., \`B2CQA-XXXX\`)
### Device Coverage (Required for new/updated tests)
Run your tests on **all supported devices** using the E2E workflow(s):
${workflowLinks.join('\n')}
${deviceTable}
> 💡 Use \`workflow_dispatch\` to trigger the workflow manually on your branch. Set the \`speculos_device\` input to test each device. Replace *paste link* with a link to the workflow run or Allure report.
`;
// Paginate through all comments to reliably find an existing bot comment
const marker = '## 🧪 E2E Test Checklist';
let botComment = null;
for await (const { data: comments } of github.paginate.iterator(
github.rest.issues.listComments,
{ owner: context.repo.owner, repo: context.repo.repo, issue_number: context.issue.number, per_page: 100 }
)) {
botComment = comments.find(c => c.user.type === 'Bot' && c.body.includes(marker));
if (botComment) break;
}
if (!botComment) {
await github.rest.issues.createComment({
owner: context.repo.owner,
repo: context.repo.repo,
issue_number: context.issue.number,
body: e2eChecklist
});
console.log(`E2E checklist comment added (${platformLabel})`);
} else {
// Update existing comment if platforms changed (e.g., mobile added after desktop-only)
if (!botComment.body.includes(platformLabel)) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: botComment.id,
body: e2eChecklist
});
console.log(`E2E checklist comment updated to ${platformLabel}`);
} else {
console.log('E2E checklist comment already exists and is up to date, skipping');
}
}
.github/workflows/pr-e2e-template.yml:30
- The pagination limit of 100 files may be insufficient for PRs with many file changes. If a PR modifies more than 100 files, some e2e changes could be missed when detecting platform changes. Consider using pagination to retrieve all files, similar to how the comment search is paginated at line 115.
const { data: files } = await github.rest.pulls.listFiles({
owner: context.repo.owner,
repo: context.repo.repo,
pull_number: context.issue.number,
per_page: 100
});
.github/workflows/pr-e2e-template.yml:50
- Missing edge case handling when neither desktop nor mobile is changed. If a PR modifies only shared e2e files outside 'e2e/desktop/' and 'e2e/mobile/' directories, both desktopChanged and mobileChanged will be false, causing platformLabel to be set to 'Mobile' (due to the ternary operator fallback). This will create misleading comments. Consider explicitly checking for this case and either handling it appropriately or skipping the comment creation.
const platformLabel = desktopChanged && mobileChanged
? 'Desktop & Mobile'
: desktopChanged ? 'Desktop' : 'Mobile';
.github/workflows/pr-e2e-template.yml:98
- Missing '@nanos' tag in the device tags checklist. Line 64 includes 'nanoS' in the devices array, and line 98 lists device tags, but '@nanos' is not mentioned in the tag list. This inconsistency could confuse developers about which tags are available. Add '@nanos' to the tag list on line 98 for completeness.
- [ ] Appropriate device tags are added: \`@NanoSP\`, \`@NanoX\`, \`@Stax\`, \`@Flex\`, \`@NanoGen5\`
.github/workflows/pr-e2e-template.yml:143
- The update logic only checks if the platform label has changed, but doesn't update the comment when device support changes or other content in the checklist is modified. If the workflow's checklist template is updated in a future commit, existing PR comments won't reflect the new template unless the platform label changes. Consider adding a version identifier or checksum to ensure comments are refreshed when the template changes.
// Update existing comment if platforms changed (e.g., mobile added after desktop-only)
if (!botComment.body.includes(platformLabel)) {
await github.rest.issues.updateComment({
owner: context.repo.owner,
repo: context.repo.repo,
comment_id: botComment.id,
body: e2eChecklist
});
console.log(`E2E checklist comment updated to ${platformLabel}`);
} else {
console.log('E2E checklist comment already exists and is up to date, skipping');
}
.github/workflows/pr-e2e-template.yml:110
- Potential issue with e2eChecklist template literal indentation. The multi-line string starting at line 88 has extra indentation (spaces) before each line within the template literal, which will be preserved in the final comment output. This may result in the comment text being improperly formatted in the PR. Remove the leading spaces before lines 90-109 to ensure proper markdown rendering.
const e2eChecklist = `## 🧪 E2E Test Checklist — ${platformLabel}
> This checklist was added automatically because your PR touches \`e2e/\` files.
### General
- [ ] Tests pass locally before pushing
- [ ] Tests are independent and can run in any order
### Test Quality
- [ ] Page Object Model (POM) pattern is followed for new pages/components
- [ ] Appropriate device tags are added: \`@NanoSP\`, \`@NanoX\`, \`@Stax\`, \`@Flex\`, \`@NanoGen5\`
- [ ] Family tags are added where applicable: \`@family-evm\`, \`@family-bitcoin\`, etc.
- [ ] TMS/Xray ticket IDs are linked in test annotations (e.g., \`B2CQA-XXXX\`)
### Device Coverage (Required for new/updated tests)
Run your tests on **all supported devices** using the E2E workflow(s):
${workflowLinks.join('\n')}
${deviceTable}
> 💡 Use \`workflow_dispatch\` to trigger the workflow manually on your branch. Set the \`speculos_device\` input to test each device. Replace *paste link* with a link to the workflow run or Allure report.
`;
4b5edd8 to
a65b4f0
Compare
ypolishchuk-ledger
force-pushed
the
e2e/ci/add-pr-comment
branch
from
a65b4f0 to
d53b133
Compare
ypolishchuk-ledger
force-pushed
the
e2e/ci/add-pr-comment
branch
from
d53b133 to
31b387d
Compare
Contributor
🧪 E2E Test Checklist — Desktop
General
Test Quality
Device Coverage (Required for new/updated tests)Run your tests on all supported devices using the E2E workflow(s):
|
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.github/workflows/pr-e2e-template.yml:94
- The workflow links in the comment use relative paths starting with '../actions/', but these paths won't work correctly when viewed in a GitHub PR comment. GitHub PR comments interpret markdown links as relative to the repository root. The correct format should be absolute URLs or use the full path from the root (e.g., '../../actions/workflows/...' or 'https://github.com/LedgerHQ/ledger-live/actions/workflows/...').
Currently, clicking these links from a PR comment would navigate to a non-existent path.
workflowLinks.push('- **Desktop:** Trigger [`test-ui-e2e-only-desktop.yml`](../actions/workflows/test-ui-e2e-only-desktop.yml) with `test_filter` set to your test name/tag');
}
if (mobileChanged) {
workflowLinks.push('- **Mobile:** Trigger [`test-mobile-e2e-reusable.yml`](../actions/workflows/test-mobile-e2e-reusable.yml) with `test_filter` set to your test name/tag');
7b06576 to
31b387d
Compare
martijnhjk
reviewed
Feb 25, 2026
|
|
||
| const platformLabel = desktopChanged && mobileChanged | ||
| ? 'Desktop & Mobile' | ||
| : desktopChanged ? 'Desktop' : 'Mobile'; |
Contributor
There was a problem hiding this comment.
Nested ternary adds to cognitive load, another approach is to keep an array and use join to formulate the final string. It might also improve readability moving this further down to where it is actually being used.
martijnhjk
approved these changes
Feb 25, 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
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.
✅ Checklist
npx changesetwas attached.📝 Description
Replace this text by a clear and concise description of what this pull request is about and why it is needed. Be sure to explain the problem you're addressing and the solution you're proposing.
For libraries, you can add a code sample of how to use it.
For bug fixes, you can explain the previous behaviour and how it was fixed.
In case of visual features, please attach screenshots or video recordings to demonstrate the changes.
❓ Context
🧐 Checklist for the PR Reviewers