migrate(leftNavigation): PMM-T1830 server diagnostics logs to Playwright - #1094
Open
kiranvuyurru wants to merge 3 commits into
Open
migrate(leftNavigation): PMM-T1830 server diagnostics logs to Playwright#1094kiranvuyurru wants to merge 3 commits into
kiranvuyurru wants to merge 3 commits into
Conversation
Append PMM-T1830 to helpCenter.test.ts with @new-navigation execution tag. Add archive.helper readZipArchive and ServerApi.getPmmVersion. Retire leftNavigation_test.js and drop @menu from nightly CodeceptJS grep.
cursor Bot
pushed a commit
that referenced
this pull request
Jul 21, 2026
Update e2e_tests/graphify-out after merging migration branch.
kiranvuyurru
marked this pull request as ready for review
July 21, 2026 14:09
Keeps PMM-T1830 tagged @menu (instead of switching to @new-navigation) so nightly-e2e-tests-matrix.yml's remote-server coverage doesn't silently drop now that the CodeceptJS source is retired. Adds a Playwright equivalent of the CodeceptJS remote-nightly runner and wires it in as a third consumer job, fixing the setup shards' expected_test_jobs count to match. Updates e2e-tests-matrix.yml's new-navigation job grep to '@new-navigation|@menu' so PR-CI coverage for this scenario is unaffected by the tag change. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
WalkthroughThe PR adds diagnostics archive validation, a reusable remote nightly Playwright workflow, and menu test routing. The workflow prepares Launchable subsets, synchronizes setup jobs, runs selected tests, and collects logs and artifacts. ChangesMenu E2E coverage and nightly execution
Sequence Diagram(s)sequenceDiagram
participant NightlyMatrix
participant RemoteWorkflow
participant Launchable
participant SetupJobs
participant RemotePMM
NightlyMatrix->>RemoteWorkflow: invoke with `@menu` and PMM inputs
RemoteWorkflow->>RemotePMM: check readiness
RemoteWorkflow->>Launchable: select test subset
RemoteWorkflow->>SetupJobs: wait for setup jobs
SetupJobs-->>RemoteWorkflow: report readiness
RemoteWorkflow->>RemotePMM: run Playwright tests
RemoteWorkflow->>Launchable: record results
RemoteWorkflow->>RemotePMM: collect logs
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/runner-e2e-tests-playwright-remote-nightly-tests.yml:
- Around line 271-275: Update
.github/workflows/runner-e2e-tests-playwright-remote-nightly-tests.yml lines
271-275 to remove || true so the Run UI tests step preserves the Playwright exit
status. At lines 147-150, make Launchable subset preparation fail when
unavailable, or explicitly fall back to running the full required test suite so
`@menu` coverage is not skipped.
In `@e2e_tests/tests/helpCenter.test.ts`:
- Around line 86-90: Update the PMM version checks in
e2e_tests/tests/helpCenter.test.ts:86-90 and
codeceptjs-e2e/tests/leftNavigation_migrated.js:76-79 to compare both major and
minor versions, treating versions newer than PMM 2.40—including PMM 3.0.x—as
requiring the forbidden Alertmanager entries. Apply the same boundary logic when
constructing forbiddenEntries in both test locations.
- Around line 70-93: Add named pmmTest.step() blocks within the test callback
for the diagnostics download, required archive-entry assertions, and
version-specific forbidden-entry checks. Keep the existing operations and
expectations unchanged, placing each related section under its corresponding
descriptive step.
🪄 Autofix
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: Pro Plus
Run ID: 09d6fc02-5445-49cc-9454-11efdfb3c152
📒 Files selected for processing (8)
.github/workflows/e2e-tests-matrix.yml.github/workflows/nightly-e2e-tests-matrix.yml.github/workflows/runner-e2e-tests-playwright-remote-nightly-tests.ymlcodeceptjs-e2e/tests/leftNavigation_migrated.jse2e_tests/api/server.api.tse2e_tests/helpers/apiEndpoints.tse2e_tests/helpers/archive.helper.tse2e_tests/tests/helpCenter.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Comment on lines
+271
to
+275
| - name: 'Run UI tests: ${{ env.PMM_TEST_FLAG }} with launchable' | ||
| if: ${{ steps.check_launchable_subset.outputs.has_subset == 'true' }} | ||
| working-directory: pmm-qa/e2e_tests | ||
| run: | | ||
| npx playwright test --grep "${{ env.PMM_TEST_FLAG }}" $(cat "${{ env.SUBSET_FILE_NAME }}") || true |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make required failures fail the workflow.
Make it so || true does not convert required failures into success. A Launchable error can leave launchable-subset.txt empty, and Lines 152-166 then skip required @menu coverage. A Playwright failure also leaves the step successful, so failure-only diagnostics and artifacts do not run.
.github/workflows/runner-e2e-tests-playwright-remote-nightly-tests.yml#L271-L275: remove|| trueand preserve the Playwright exit status..github/workflows/runner-e2e-tests-playwright-remote-nightly-tests.yml#L147-L150: fail Launchable preparation, or implement an explicit full-test fallback when Launchable is unavailable.
🧰 Tools
🪛 zizmor (1.29.0)
[warning] 275-275: code injection via template expansion (template-injection): may expand into attacker-controllable code
(template-injection)
📍 Affects 1 file
.github/workflows/runner-e2e-tests-playwright-remote-nightly-tests.yml#L271-L275(this comment).github/workflows/runner-e2e-tests-playwright-remote-nightly-tests.yml#L147-L150
🤖 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/runner-e2e-tests-playwright-remote-nightly-tests.yml
around lines 271 - 275, Update
.github/workflows/runner-e2e-tests-playwright-remote-nightly-tests.yml lines
271-275 to remove || true so the Run UI tests step preserves the Playwright exit
status. At lines 147-150, make Launchable subset preparation fail when
unavailable, or explicitly fall back to running the full required test suite so
`@menu` coverage is not skipped.
Comment on lines
+70
to
+93
| pmmTest( | ||
| 'PMM-T1830 - Verify downloading server diagnostics logs @menu', | ||
| async ({ api, helpPage }) => { | ||
| const download = await helpPage.exportLogs(); | ||
| const path = await download.path(); | ||
|
|
||
| if (!path) { | ||
| throw new Error('Download path is null'); | ||
| } | ||
|
|
||
| const entries = readZipArchive(path); | ||
|
|
||
| expect(entries).toContain('pmm-agent.yaml'); | ||
| expect(entries).toContain('pmm-managed.log'); | ||
| expect(entries).toContain('pmm-agent.log'); | ||
|
|
||
| const version = await api.serverApi.getPmmVersion(); | ||
| const forbiddenEntries = version.minor > 40 ? ['alertmanager.yml', 'alertmanager.base.yml'] : []; | ||
|
|
||
| for (const entry of forbiddenEntries) { | ||
| expect(entries).not.toContain(entry); | ||
| } | ||
| }, | ||
| ); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
printf '%s\n' '--- target test ---'
sed -n '1,130p' e2e_tests/tests/helpCenter.test.ts
printf '%s\n' '--- pmmTest.step usage ---'
rg -n -C 2 'pmmTest\.step|test\.step' e2e_tests --glob '*.ts' --glob '*.tsx' | head -160
printf '%s\n' '--- fixture definition ---'
rg -n -C 4 'pmmTest\s*=|function pmmTest|const pmmTest|export.*pmmTest' e2e_tests --glob '*.ts' --glob '*.tsx' | head -160Repository: percona/pmm-qa
Length of output: 18055
Add named pmmTest.step() blocks for the download, required-entry, and version-specific checks. This keeps diagnostics failures readable, as established by the surrounding tests. Make it so.
🤖 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_tests/tests/helpCenter.test.ts` around lines 70 - 93, Add named
pmmTest.step() blocks within the test callback for the diagnostics download,
required archive-entry assertions, and version-specific forbidden-entry checks.
Keep the existing operations and expectations unchanged, placing each related
section under its corresponding descriptive step.
Source: Coding guidelines
Comment on lines
+86
to
+90
| const version = await api.serverApi.getPmmVersion(); | ||
| const forbiddenEntries = version.minor > 40 ? ['alertmanager.yml', 'alertmanager.base.yml'] : []; | ||
|
|
||
| for (const entry of forbiddenEntries) { | ||
| expect(entries).not.toContain(entry); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the PMM version boundary check.
minor > 40 is false for version 3.0.x, although 3.0.x is newer than PMM 2.40. The test then permits Alertmanager files that must be absent.
e2e_tests/tests/helpCenter.test.ts#L86-L90: compare bothmajorandminorbefore creatingforbiddenEntries.codeceptjs-e2e/tests/leftNavigation_migrated.js#L76-L79: apply the same major-and-minor comparison.
Proposed condition
-const forbiddenEntries = version.minor > 40 ? ['alertmanager.yml', 'alertmanager.base.yml'] : [];
+const alertmanagerFilesRemoved =
+ version.major > 2 || (version.major === 2 && version.minor > 40);
+const forbiddenEntries = alertmanagerFilesRemoved
+ ? ['alertmanager.yml', 'alertmanager.base.yml']
+ : [];📝 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
| const version = await api.serverApi.getPmmVersion(); | |
| const forbiddenEntries = version.minor > 40 ? ['alertmanager.yml', 'alertmanager.base.yml'] : []; | |
| for (const entry of forbiddenEntries) { | |
| expect(entries).not.toContain(entry); | |
| const version = await api.serverApi.getPmmVersion(); | |
| const alertmanagerFilesRemoved = | |
| version.major > 2 || (version.major === 2 && version.minor > 40); | |
| const forbiddenEntries = alertmanagerFilesRemoved | |
| ? ['alertmanager.yml', 'alertmanager.base.yml'] | |
| : []; | |
| for (const entry of forbiddenEntries) { | |
| expect(entries).not.toContain(entry); |
📍 Affects 2 files
e2e_tests/tests/helpCenter.test.ts#L86-L90(this comment)codeceptjs-e2e/tests/leftNavigation_migrated.js#L76-L79
🤖 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_tests/tests/helpCenter.test.ts` around lines 86 - 90, Update the PMM
version checks in e2e_tests/tests/helpCenter.test.ts:86-90 and
codeceptjs-e2e/tests/leftNavigation_migrated.js:76-79 to compare both major and
minor versions, treating versions newer than PMM 2.40—including PMM 3.0.x—as
requiring the forbidden Alertmanager entries. Apply the same boundary logic when
constructing forbiddenEntries in both test locations.
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-tests-matrix.yml (1)
137-147: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winSet least-privilege permissions for
psmdb_integration.The called workflow passes
GITHUB_TOKENto checkout and the status action. Addpermissions: contents: readto this job. Addstatuses: writeonly if the status steps are enabled.🤖 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-tests-matrix.yml around lines 137 - 147, Add job-level permissions to the psmdb_integration reusable-workflow invocation, granting contents: read for checkout and statuses: write only when its status steps are enabled. Keep the existing inputs and secrets 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.
Outside diff comments:
In @.github/workflows/e2e-tests-matrix.yml:
- Around line 137-147: Add job-level permissions to the psmdb_integration
reusable-workflow invocation, granting contents: read for checkout and statuses:
write only when its status steps are enabled. Keep the existing inputs and
secrets unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ae11ff64-37f1-4e93-96d8-f182f522da24
📒 Files selected for processing (2)
.github/workflows/e2e-tests-matrix.ymle2e_tests/tests/helpCenter.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
🚧 Files skipped from review as they are similar to previous changes (1)
- e2e_tests/tests/helpCenter.test.ts
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
Migrates the only active scenario from
codeceptjs-e2e/tests/leftNavigation_test.jsintoe2e_tests/tests/helpCenter.test.ts.@new-navigation(destination file tag; reuses existingnew-navigationPlaywright job)Changes
helpCenter.test.tswith inline zip assertions viareadZipArchivee2e_tests/helpers/archive.helper.tsServerApi.getPmmVersion()and/v1/versionendpointleftNavigation_test.js→leftNavigation_migrated.js@menufrom nightly CodeceptJS grep (no active CodeceptJS@menuscenarios remain)Excluded (per tracker)
Data(...)PMM-T433/T591xScenarioPMM-T1051Validation
CLEAN_ENVIRONMENT=false ./.cursor/scripts/run-migration-single-test.sh tests/helpCenter.test.ts '' false --grep PMM-T1830→ passhelpCenter.test.tsregression (12 tests) → passControl branch follow-up
Tracker and
e2e_tests/graphify-out/update will land onCodeceptJS-migrationafter this PR is opened.