migrate(settings): verifyPMMSettingsPageElements CodeceptJS to Playwright - #1314
migrate(settings): verifyPMMSettingsPageElements CodeceptJS to Playwright#1314kiranvuyurru wants to merge 3 commits into
Conversation
…laywright Port PMM-T97 (3 data-driven rows), PMM-T84, PMM-T85, "Verify Advanced Section Elements", PMM-T227 (skipped) and PMM-T1866 from codeceptjs-e2e/tests/configuration/verifyPMMSettingsPageElements_test.js to native Playwright. Every source tag is preserved verbatim; no destination tag was added, since all 8 tests already carry @settings. Adds SettingsApi.restoreSettingsDefaults/setPublicAddress, ServerApi.getDistributionMethod, the /v1/server/version endpoint (the only one carrying distribution_method), and settings-page labels, inputs and three wait/read helpers on the settings POM. Five POM input entries had zero consumers and disagreed with the green CodeceptJS source, so they were corrected in place to the source-proven '<field>-<type>-input' ids the PMM UI actually renders. The CodeceptJS source is deliberately KEPT rather than retired: its @grafana-pr tag has no consumer in .github/workflows/, only codeceptjs-e2e/package.json's e2e:grafana-pr script, which percona/grafana CI invokes cross-repository and which runs CodeceptJS only. Retiring the file would silently drop 4 scenarios from that gate with nothing able to restore them. A header comment on the source records this and the condition that would allow retirement. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds a settings_playwright job to e2e-tests-matrix.yml greping '@settings', the Playwright sibling of the existing settings_and_cli CodeceptJS job. Without it the 8 scenarios migrated out of codeceptjs-e2e/tests/configuration/verifyPMMSettingsPageElements_test.js run in no Playwright job at all. A new job rather than a tag append: e2e-tests-matrix.yml has no test_execution_playwright matrix entry to append to, and no existing Playwright job in the file selects these tests. settings_and_cli is left as-is because the CLI tests still need '@cli'. setup_services '-h' is the server-only idiom already used by the alerting Playwright job in fb-e2e-suite.yml; nothing under '@settings' needs a database. 'workers' is omitted so runner-e2e-tests-playwright.yml's `inputs.workers || 1` keeps it at 1 -- the migrated file's beforeEach restores server-wide settings defaults and PMM-T1866 mutates them, so these tests cannot run in parallel. '@settings' selects 12 tests in 2 files: the 8 migrated ones plus 4 pre-existing tests in e2e_tests/tests/portalRemoval.test.ts that no Playwright job selected before. Those need PMM Server only, and all 12 were run together at workers=1 against a server-only environment before this commit: 11 passed, 1 skipped by policy (PMM-T227, PMM-5791). expected_test_jobs is untouched and correct: it lives in nightly-e2e-tests-matrix.yml and counts jobs matching the "test execution / " name prefix, which only that workflow's two matrices produce. Nightly consumers before 3, after 3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
GitHub Actions: https://github.com/percona/pmm-qa/actions/runs/33812529606 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| }); | ||
| }); | ||
|
|
||
| // TODO: Remove the skip after https://jira.percona.com/browse/PMM-5791 |
There was a problem hiding this comment.
🟡 This comment restates the eslint-disable reason on the very next line — both say the skip stays until PMM-5791. One of the two is enough, and the one that has to be there is the suppression reason.
| // TODO: Remove the skip after https://jira.percona.com/browse/PMM-5791 |
There was a problem hiding this comment.
Not applied. check-migration-conventions.sh requires both a TODO and the eslint-disable on every pmmTest.skip; removing the TODO fails that check, so the duplication is enforced rather than accidental. I hit this directly — the check failed until I restored it.
🤖 Addressed by Claude Code
|
|
||
| for (const row of dataRetentionRows) { | ||
| pmmTest( | ||
| `PMM-T97 - Verify server diagnostics on PMM Settings Page @settings @grafana-pr | {"value":"${row.value}","message":"${row.message}"}`, |
There was a problem hiding this comment.
🟡 The | {"value":"…","message":"…"} suffix is a hand-built imitation of CodeceptJS's Data() title, and the PR body already records that it will diverge from real JSON the moment a row contains a quote or a backslash. Nothing consumes it as JSON, so the format buys nothing and the risk is free.
The title's job here is to say which row failed. A plain description does that and stays inside the PMM-Txxxx - description convention:
| `PMM-T97 - Verify server diagnostics on PMM Settings Page @settings @grafana-pr | {"value":"${row.value}","message":"${row.message}"}`, | |
| `PMM-T97 - Verify server diagnostics on PMM Settings Page: data retention "${row.value}" @settings @grafana-pr`, |
There was a problem hiding this comment.
Not applied. The repo migration rules require generated data-driven titles to stay byte-identical to the CodeceptJS source, and this suffix reproduces what DataTable emits through replaceTitle in column order. An earlier review round blocked this PR precisely for getting those bytes wrong, so changing the format now would reintroduce that. The escaping risk is real and is recorded in the PR body; it needs an invariant change, not a one-PR edit.
🤖 Addressed by Claude Code
|
No check run has concluded on Findings are in the line threads. |
|
Warning Review limit reachedNext included review available in 36 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (5)
WalkthroughAdds a Playwright suite for the PMM Settings page. The suite covers validation messages, metrics resolutions, AMI SSH-key elements, advanced settings, and public-address updates. New API methods restore settings, update the public address, and read the server distribution method. The settings page object adds updated selectors and synchronization helpers. CI runs the Sequence Diagram(s)sequenceDiagram
participant SettingsTests
participant SettingsPage
participant SettingsApi
participant PMMServer
SettingsTests->>SettingsApi: Restore default settings
SettingsTests->>SettingsPage: Load settings page
SettingsTests->>SettingsPage: Validate settings elements and input messages
SettingsTests->>SettingsApi: Set public address
SettingsPage->>PMMServer: Apply settings changes
SettingsTests->>SettingsPage: Verify saved values and error state
SettingsTests->>SettingsApi: Clear public address
Merge Risk: 🟡 Moderate · up to The migrated suite can pass despite failed settings saves or contaminate later tests, and the new CI path unnecessarily exposes a credential to test code. These issues should be addressed before merge. 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
![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
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: 6f70fdcd-f73b-4242-9620-caae07432916
📒 Files selected for processing (8)
.github/workflows/e2e-tests-matrix.ymlcodeceptjs-e2e/tests/configuration/verifyPMMSettingsPageElements_test.jse2e_tests/README.mde2e_tests/api/server.api.tse2e_tests/api/settings.api.tse2e_tests/helpers/apiEndpoints.tse2e_tests/pages/ha/settings.page.tse2e_tests/tests/configuration/settingsPageElements.test.ts
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
percona/pmm-qa(manual)percona/pmm(manual)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.
Replace the non-retrying validation-message read with a web-first toHaveJSProperty, and drop the no-op waitFor that preceded it. Report PMM-T85 as skipped rather than passed when the distribution is not AMI; an early return made it green while asserting nothing. Replace the ported fixed 5s pause with a real save-completion signal: SettingsPage.applyAdvancedChanges awaits the settings PUT, and the test waits for the Apply button's label to return from "Applying...". The button is disabled after a successful save because the form is pristine, so button state is not a usable signal; the label is. Move each errorAlert check after the value assertion, where the save has demonstrably completed - before it, toBeHidden could never fail. Collapse the duplicated data-retention toHaveValue pair into one web-first assertion. Assert 200 on the settings PUTs, matching every sibling in the class, so a failed precondition in beforeEach/afterEach surfaces where it happens. Trim the workflow job rationale comment and the retained-source header to the facts a reader cannot infer from the code. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Migrates the PMM Settings page-element tests from CodeceptJS to native Playwright.
Read this first: the CodeceptJS source is deliberately NOT retired
Every other migration in this series retires its source. This one does not, and that is a
deliberate decision, not an oversight.
@grafana-prhas zero consumers in.github/workflows/(grep -rn 'grafana-pr' .github/returns nothing). Its only consumer is
codeceptjs-e2e/package.json:14:which is invoked cross-repository by percona/grafana CI, and which runs CodeceptJS only. A
Playwright job in this repository cannot restore that gate's coverage.
Retiring
verifyPMMSettingsPageElements_test.jswould drop its 4@grafana-prscenarios(6 generated tests) from the percona/grafana gate — and it would do so silently, because
7 other CodeceptJS files still carry
@grafana-pr(permissions_test.js,sttSettings_test.js,alertRules_test.js,explorePage_test.js,verifyGrafanaIsGone_test.js,verifyPMMSettingsPageFunctionality_test.js,ruleTemplates_test.js), so the job stays greenwhile testing less.
Accepted consequence: these scenarios now run twice in pmm-qa CI — the existing
settings_and_cliCodeceptJS job and the newsettings_playwrightPlaywright job. Verified safe:settings_and_clisettings_playwrightrunner-e2e-tests-codeceptjs.ymlrunner-e2e-tests-playwright.yml--test-suiteui-tests-<tags>pw-ui-tests-<report>Separate runners, separate PMM Servers, distinct Launchable suite prefixes — no shared state and
no Launchable session collision. The cost is CI minutes only.
Condition for retiring it later: once percona/grafana CI can invoke the Playwright suite. A
header comment on the source records this so the next reader does not have to rediscover it.
Migration
codeceptjs-e2e/tests/configuration/verifyPMMSettingsPageElements_test.js(kept, see above)e2e_tests/tests/configuration/settingsPageElements.test.ts(new file)Scenarios and tags
Every source tag is preserved verbatim. No destination tag was added — all 8 tests already
carry
@settings.@settings @grafana-pr@settings @grafana-pr@settings @grafana-pr@settings @grafana-pr@settings@settingsData(dataRetentionTable).Scenariobecomes aforloop overdataRetentionRows;Before/Afterbecome
beforeEach/afterEach, so the file is self-preconditioning and rerunnable.Files queried from the existing graphs
Source graph (
codeceptjs-e2e/graphify-out/), plus a direct check of the scenarios' injectedfixture names against
codeceptjs-e2e/tests/**/pages/*.js(CodeceptJS DI is not a static import,so graphify has no edge for it):
codeceptjs-e2e/tests/configuration/pages/pmmSettingsPage.jssettingsAPI(restoreSettingsDefaults,changeSettings),serverApi,I.AuthorizeTarget graph (
e2e_tests/graphify-out/):e2e_tests/pages/ha/settings.page.ts— reused, extendede2e_tests/api/settings.api.ts,e2e_tests/api/server.api.ts— reused, extendede2e_tests/helpers/apiEndpoints.ts,e2e_tests/helpers/timeouts.ts,e2e_tests/fixtures/pmmTest.ts— reused as-isSupporting changes
SettingsApi.restoreSettingsDefaults()— ports the CodeceptJS helper, including itsPMM_ENABLE_TELEMETRY400-retry path.SettingsApi.setPublicAddress()— replaces the source'schangeSettings({ publicAddress: '' }).ServerApi.getDistributionMethod()and the/v1/server/versionendpoint./v1/version, alreadyin
apiEndpoints, does not carrydistribution_method;/v1/server/versionis the only onethat does. PMM-T85 short-circuits unless the distribution is AMI, faithfully to the source.
elements(section labels, error alert, tab content),inputs.dataRetention,and three wait/read helpers (
waitForPageLoaded,settleAfterApplyingChanges,getDataRetentionValidationMessage).urlsandbuttons.togglesare unchanged.inputsentries (high,low,medium,publicAddress,sshKey) hadzero consumers and disagreed with the green CodeceptJS source, so they were corrected in
place to the ids the PMM UI actually renders rather than shadowed by new entries.
Setup
node provisioning/setup.ts— PMM Server only,setup_client=false. Every@settingstest needsPMM Server alone, which is why the new job passes
setup_services: '-h'and no database.Validation
npx tsc --noEmitnpx eslint .python support_scripts/generate_readme.py --checkcheck-migration-conventions.shMCP locator verification
16 locators verified live against the provisioned PMM Server through Playwright MCP, 0 edits
required.
ssh-keyandssh-key-labelcould not be verified — they render only on an AMIdistribution, and the environment is Docker. That is the same reason PMM-T85 short-circuits.
Execution
Both runs against the provisioned server,
--workers=1(these tests mutate shared serversettings and cannot run in parallel):
The new job's grep also newly selects 4 pre-existing tests in
portalRemoval.test.tsthat noPlaywright job selected before, so the full selection was run too:
The single skip is PMM-T227, intentionally skipped pending PMM-5791.
Workflow coverage
One new job in
.github/workflows/e2e-tests-matrix.yml:A new job rather than a tag append to an existing
test_execution_playwrightentry, becausee2e-tests-matrix.ymlhas no such matrix entry — its Playwright jobs are individual job blocks.It mirrors the CodeceptJS
settings_and_clijob's surface, minus the database that job needs for@cli.No existing job, grep expression, or
expected_test_jobscounter was changed.expected_test_jobslives innightly-e2e-tests-matrix.yml, which this PR does not touch; thisjob is in
e2e-tests-matrix.ymland is not fed by the nightly setup shards.Grep verification
@settingsconsumers across.github/workflows/, after the edit:setup_servicessettings_and_clie2e-tests-matrix.yml:71tags_for_tests: '@settings|@cli'--database pgsqlsettings_playwrighte2e-tests-matrix.yml:210pmm_test_flag: '@settings'-hForward direction — every migrated scenario is selected:
Reverse direction — no existing selection changed. This PR adds a job and edits no existing
expression, and the
@settings|@cliCodeceptJS grep is untouched, so its selection is unchangedby construction; the source file is still on the
_test.jsdiscovery glob, so no CodeceptJS jobloses scenarios.
FB suite: none of the migrated scenarios carry an
@fb-*tag, sofb-e2e-suite.ymlis unaffected.Reviews
Initial gate: attempt 1
REVIEW_FAILED(one blocker, B1), attempt 2READY_TO_RUN.Final gate: attempt 1
FINAL_REVIEW_PASS, subject stable (startRef == endRef).Two advisories a reviewer should know about
Both were raised, judged acceptable, and carried rather than fixed:
JSON.stringify'd. The title ends with| {"value":"...","message":"..."}assembled by template literal. It is byte-exact for thethree current rows, but a future row containing a quote or a backslash would produce a title
that diverges from real JSON, silently. A
JSON.stringify-based form is not viable here:perfectionist/sort-objectsreorders the object literal's keys, so the emitted key orderwould no longer match the intended
value-then-messageorder.Playwright file, but the Playwright file does not name the source, because a migrated
*.test.tsmay carry zero comments.