Match rule-template rows by cell text, not direct text node - #1297
Conversation
The Alert Rule Templates name cell now renders the name inside a <div><span>, so `//td[contains(text(), NAME)]` matches nothing and every row-action lookup by name times out. Scope the three *ByName button locators to the row instead, reusing the templateRow() helper that already matches on the cell's string value. PMM-T1514 grabs the first name cell and feeds it back into such a locator, so it now takes only the cell's first line -- an overridable template renders a "Dynamic" badge in the same cell. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KL3FgMrwzE9PjV5dZ6eFUX
WalkthroughThe page object now builds edit, delete, and add-rule locators from the shared Merge Risk: 🔵 Low · up to The change makes rule-template actions work with nested name-cell markup, but the row selector can still match unrelated or partially matching cell text and potentially act on the wrong template when names overlap. The PR is mergeable with explicit owner awareness and follow-up to scope matching to the template-name cell. 🚥 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 |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
- Inline grabFirstTemplateName() at its single call site (PMM-T1514) and
delete the method; the comment explaining the split travels with it.
- Drop the two comments over editButtonByName/deleteButtonByName. They
restated the line, and the first named the wrong button ("Delete
template button" over editButtonByName).
- Drop templateRowByName, a zero-caller pass-through to templateRow()
that the three *ByName locators now call directly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KL3FgMrwzE9PjV5dZ6eFUX
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Team
Run ID: ec70c8ee-46c8-485b-a705-5b0927a12d5f
📒 Files selected for processing (2)
codeceptjs-e2e/tests/ia/pages/ruleTemplatesPage.jscodeceptjs-e2e/tests/ia/ruleTemplates_test.js
🔗 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.
templateRow() matched any cell by substring, so a lookup could resolve to
more than one row: a name contained in another ("Template bulk 1" also
matching "Template bulk 1_updated"), or one that happens to appear in the
Source column ("Built-in" matching every built-in row). Scoping to any
cell was new here, since the previous locators keyed off the name cell's
own text node.
Match td[1] exactly instead, against either the cell's own text or a
nested element's, so both DOM shapes still resolve.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KL3FgMrwzE9PjV5dZ6eFUX
2 participants
Failures fixed (investigator)
@fb-alerting|@fb-settings UI testscodeceptjs-e2e/tests/ia/ruleTemplates_test.js/@fb-alertingWhat failed
13 of 13 actionable failures (Launchable gate:
13 actionable, 0 quarantined; 42 tests, 29 passed)came from one file and one locator shape — a row-action button addressed through the template's
name cell:
Root cause — the pmm-qa locator, not the product
grafana#912(PMM-14912, still an open draft) gives the Name column aCellrenderer: it wraps thevalue in a
Stackholding aspanwith the template name, followed by a conditionalBadgelabelled
Dynamicwhen the template's YAML containsoverridable: true.On
percona/grafanamainthat column has noCellat all, so react-table renders the name as thecell's own text and XPath
text()— which only sees a node's direct text children — matches.With the renderer the name moves one level down, and every
*ByNamelookup matches nothing://td[contains(text(), NAME)]maintdgrafana#912td > div.css-6hq8f5 > spanholding the nameReproduced on a throwaway Linode VM running the FB build
perconalab/pmm-server-fb:PR-4449-3eded28(
3.10.0-PMM-14912-dynamic-thresholds-3eded285c), evaluating locators against the live templatestable on each build:
mainbuild//td[contains(text(), "PMM agent down")]//td[contains(., "PMM agent down")]…/following-sibling::td//a[@data-testid="create-from-template-button"](before)create-from-template-button(this PR)The product is behaving as the feature intends; the assertions are all still right.
Fix
The three
*ByNamebutton locators now resolve throughtemplateRow(), which matches the namecolumn (
td[1]) exactly — against either the cell's own text (themainshape) or a nestedelement's text (the
grafana#912shape). That matches both DOM shapes, so this is not blockedon
grafana#912: it works againstmaintoday and against the FB build.PMM-T1514 needed one more line. It grabs the first name cell and feeds that text back into a name
locator, and the new cell also renders the
Dynamicbadge for an overridable template — on the FBbuild
//tr/td[1]reads as"Node high CPU load\nDynamic"(exactly one template carries it,node_high_cpu_load.yml, which gainedoverridable: trueinpmm#5878). The test now takes thecell's first line, which is a no-op on the current
mainshape.Folded in from review:
templateRow()scopes totd[1]and matches exactly. An earlier revision matched any cell bysubstring, which could resolve to more than one row — a name contained in another
(
Template bulk 1also matchingTemplate bulk 1_updated), or one appearing in the Source column(
Built-inmatching every built-in row). Not reachable by the current tests, sinceclearAllTemplates()runs per scenario inBefore, but the any-cell part was new here and worthclosing.
templateRowByNamewas a zero-caller pass-through to the same helper and is gone.editButtonByName/deleteButtonByNameare gone — they restated the lineand the first named the wrong button.
grabFirstTemplateName()had a single caller and is inlined at it.Deliberately unchanged: the
*BySourcelocators already match throughdescendant::div(verified — 42 rows for
Built-in), andelements.addedTemplateusestd[text()=…]but isreferenced by no test.
Verification
Same VM, same suite (
codeceptjs run -c pr.codecept.js tests/ia/ruleTemplates_test.js --grep '@fb-alerting'),brought up the way
runner-e2e-tests-codeceptjs.ymldoes (codeceptjs-e2e/docker-compose.yml,testdata/db_setup.sh,pmm3-client-setup.sh,pmm-framework --database mysql):perconalab/pmm-server-fb:PR-4449-3eded28perconalab/pmm-server-fb:PR-4449-3eded28perconalab/pmm-server:3-dev-latest(no PMM-14912)The 13 red scenarios are exactly the 13 Launchable reported as actionable on the FB run, and every
one of their step traces names a
//td[contains(text(), …)]locator. The third row is thebackward-compatibility check, and this PR's own CI confirms it independently: the
FB E2E tests / Alerting and Settings UI tests / e2e tests: @fb-alerting|@fb-settingsjob is greenhere, on a build without PMM-14912. So the change is safe to merge before
grafana#912.The later
templateRow()tightening was re-checked against both DOM shapes rebuilt as fixtures fromthe markup captured on each live build, driving the shipped locators: every template name the
suite uses resolves to exactly one row and one button on both shapes,
edit/deletestay at 1 foruser-defined rows and 0 for built-in/file rows (what PMM-T825's
dontSeeElementrelies on), and anabsent name yields 0.
The one skipped scenario is PMM-T510,
Scenario.skiponmainalready.Host port 443 on the repro VM belongs to the exec channel, so PMM's
443:8443publish was remappedto
8443; the tests drivehttp://127.0.0.1/(port 80) exactly as CI does, so nothing under testwas affected.
What only a real FB run can confirm: the local runs executed the whole
ruleTemplates_test.jsfile rather than Launchable's subset, and against a single FB build; the
templateRow()tighteningthat came out of review was verified against DOM fixtures plus this PR's CI, not against a second
live FB server. The FB check going green end-to-end is for the next FB run on a PR carrying
grafana#912.🤖 Generated with Claude Code
https://claude.ai/code/session_01KL3FgMrwzE9PjV5dZ6eFUX