Skip to content

fix(e2e): fix Settings link and catalog timestamp failures on release-1.8#4518

Draft
zdrapela wants to merge 2 commits intoredhat-developer:release-1.8from
zdrapela:fix/e2e-settings-link-and-nightly-failures
Draft

fix(e2e): fix Settings link and catalog timestamp failures on release-1.8#4518
zdrapela wants to merge 2 commits intoredhat-developer:release-1.8from
zdrapela:fix/e2e-settings-link-and-nightly-failures

Conversation

@zdrapela
Copy link
Copy Markdown
Member

@zdrapela zdrapela commented Apr 1, 2026

Summary

  • goToSettingsPage(): use getByRole('menuitem') instead of clickLink('Settings') since the profile dropdown renders Settings as a menuitem, not an <a> link
  • default-global-header: replace verifyLinkVisible('Settings') with menuitem check to avoid strict mode violation (5 matching <a> elements)
  • catalog-timestamp: clear search filter from previous test, use semantic selectors (getByRole) instead of brittle CSS nth-child selectors
  • .gitignore / eslint.config.js: add .local-test to ignores to prevent prettier/eslint from scanning deployment artifacts

These fixes are backported from main branch (commit bba55b4).

Fixes failing tests in release-1.8 OCP Helm nightly:

  • default-global-header.spec.ts — "Verify Profile Dropdown behaves as expected"
  • guest-signin-happy-path.spec.ts — "Verify Profile is Guest in Settings page" & "Sign Out"
  • catalog-timestamp.spec.ts — "Toggle CREATED AT sorting"

…-1.8

- goToSettingsPage(): use getByRole('menuitem') instead of clickLink('Settings')
  since the profile dropdown renders Settings as a menuitem, not an <a> link
- default-global-header: replace verifyLinkVisible('Settings') with menuitem
  check to avoid strict mode violation (5 matching <a> elements)
- catalog-timestamp: clear search filter from previous test, use semantic
  selectors (getByRole) instead of brittle CSS nth-child selectors
- .gitignore: add .local-test to prevent prettier from scanning deployment
  artifacts (backported from main)
- eslint.config.js: add .local-test to ignores to prevent ESLint from scanning
  deployment artifacts

These fixes are backported from main branch (commit bba55b4).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@zdrapela
Copy link
Copy Markdown
Member Author

zdrapela commented Apr 1, 2026

/agentic_review

@openshift-ci openshift-ci bot requested review from psrna and subhashkhileri April 1, 2026 07:28
@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge bot commented Apr 1, 2026

Code Review by Qodo

🐞 Bugs (3) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX Issues (0)

Grey Divider


Action required

1. Filter clear not verified 🐞 Bug ☼ Reliability
Description
catalog-timestamp.spec.ts attempts to clear the previous test’s catalog search via an optional
“clear search” button click but never asserts the filter is actually removed, so the sort test can
run against a single filtered row and stop validating sort behavior reliably. This can yield false
positives or flakiness depending on whether the clear affordance exists/works in that UI variant.
Code

e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[R64-69]

+    // Clear search filter from previous test to show all components
+    const clearButton = page.getByRole("button", { name: "clear search" });
+    if (await clearButton.isVisible()) {
+      await clearButton.click();
+    }
+
Evidence
The first test sets a search filter and the suite reuses the same Page across tests; beforeEach
navigates but does not clear the search input. The second test only clicks a presumed clear control
if it is visible and does not verify the input value is empty afterward, so the subsequent sorting
assertions may execute with the stale filter still applied.

e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[40-46]
e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[48-57]
e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[63-73]
e2e-tests/playwright/support/page-objects/page-obj.ts[14-17]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`catalog-timestamp.spec.ts` relies on an optional click of a "clear search" control to reset state from the prior test, but never verifies that the search field is actually cleared. If the control is absent or the click doesn’t clear the input (UI variant/accessibility name differences), the sorting test may run with the old filter still applied.

### Issue Context
The previous test sets a search filter via `uiHelper.searchInputPlaceholder(...)`, and the suite reuses the same `page` across tests; `beforeEach` does not clear the field.

### Fix Focus Areas
- e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[48-93]
- e2e-tests/playwright/support/page-objects/page-obj.ts[14-17]

### Suggested change
After attempting to clear, explicitly clear the known search input (using `SEARCH_OBJECTS_COMPONENTS.placeholderSearch`/`ariaLabelSearch`) and assert it is empty (e.g., `await expect(page.locator(SEARCH_OBJECTS_COMPONENTS.placeholderSearch)).toHaveValue('')`) before proceeding to row/sort assertions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Remediation recommended

2. Unscoped table role selectors 🐞 Bug ☼ Reliability
Description
catalog-timestamp.spec.ts queries rows and the “Created At” columnheader via page.getByRole(...)
without scoping to the catalog table, which can cause strict-mode errors or target the wrong table
if additional tables/rowgroups exist on the page. This makes the test brittle and can hide real
regressions by interacting with unintended elements.
Code

e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[R70-85]

+    // Wait for the table to have data rows
+    await expect(
+      page.getByRole("row").filter({ has: page.getByRole("cell") }),
+    ).not.toHaveCount(0);
+
+    // Get the first data row’s "Created At" cell using semantic selectors
+    const firstRow = page
+      .getByRole("row")
+      .filter({ has: page.getByRole("cell") })
+      .first();
+    const createdAtCell = firstRow.getByRole("cell").nth(7); // 0-indexed, 8th column = index 7
+
+    const column = page.getByRole("columnheader", {
+      name: "Created At",
+      exact: true,
+    });
Evidence
The new test uses global role queries for row, cell, and columnheader. The repo already
defines a canonical Material UI table selector (table.MuiTable-root) and has helper logic that
scopes to table tbody tr, but the updated test bypasses those patterns and therefore risks
matching elements outside the catalog results table.

e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[70-85]
e2e-tests/playwright/support/page-objects/global-obj.ts[6-28]
e2e-tests/playwright/utils/ui-helper.ts[678-692]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The test uses unscoped `page.getByRole('row'|'cell'|'columnheader')` locators. If any other table/grid appears on the page (now or in the future), Playwright strict mode can fail or the test may interact with the wrong elements.

### Issue Context
The codebase already has a stable table selector (`UI_HELPER_ELEMENTS.MuiTable = 'table.MuiTable-root'`) and helper methods that scope to `table tbody tr`.

### Fix Focus Areas
- e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[70-92]
- e2e-tests/playwright/support/page-objects/global-obj.ts[20-22]

### Suggested change
Introduce a `const table = page.locator(UI_HELPER_ELEMENTS.MuiTable);` (or `page.getByRole('table').first()` if appropriate) and then query rows/headers beneath it, e.g. `table.getByRole('row')...` and `table.getByRole('columnheader', { name: 'Created At' })`.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Advisory comments

3. Hardcoded Settings label 🐞 Bug ⚙ Maintainability
Description
goToSettingsPage() and default-global-header.spec.ts now require the menuitem accessible name to be
exactly “Settings”, which will break when LOCALE is set to a non-English value or when the tracked
translation issue is resolved. This introduces locale-dependent brittleness across any tests that
reuse goToSettingsPage().
Code

e2e-tests/playwright/utils/ui-helper.ts[R254-260]

+    // TODO: [RHDHBUGS-2552](https://redhat.atlassian.net/browse/RHDHBUGS-2552) - Strings not getting translated
+    // The profile dropdown renders Settings as a menuitem, not an <a> link
+    const settingsItem = this.page.getByRole("menuitem", {
+      name: "Settings",
+    });
+    await expect(settingsItem).toBeVisible();
+    await settingsItem.click();
Evidence
The E2E framework is locale-aware (LOCALE env var) and most assertions use translations via
t[...]/lang. Hardcoding a single English string for navigation is inconsistent with that design
(even if currently justified by the TODO) and will require follow-up changes when localization
behavior changes.

e2e-tests/playwright/e2e/localization/locale.ts[20-23]
e2e-tests/playwright/e2e/default-global-header.spec.ts[9-11]
e2e-tests/playwright/e2e/default-global-header.spec.ts[116-139]
e2e-tests/playwright/utils/ui-helper.ts[251-261]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The Settings menu item is matched by the hardcoded name "Settings". This will fail under non-English locales or when translations are fixed.

### Issue Context
This appears intentional (TODO [RHDHBUGS-2552](https://redhat.atlassian.net/browse/RHDHBUGS-2552)), so this is not merge-blocking, but making it future-proof reduces churn.

### Fix Focus Areas
- e2e-tests/playwright/utils/ui-helper.ts[251-261]
- e2e-tests/playwright/e2e/default-global-header.spec.ts[116-136]

### Suggested change
Match either the hardcoded string *or* the translated key when available (e.g., `name: /^(Settings|<translated>)$/`), or centralize the label in UIhelper using `t[...]` with a fallback to "Settings".

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

ⓘ The new review experience is currently in Beta. Learn more

Grey Divider

Qodo Logo

@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge bot commented Apr 1, 2026

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🔒 No security concerns identified
⚡ Recommended focus areas for review

Fragile Selector

The new approach improves robustness by using roles, but it still relies on a hard-coded cell index (nth(7)) for the "Created At" column. Any column reordering/conditional columns could silently break the test or validate the wrong cell. Consider locating the "Created At" cell by first resolving the column header index (or using a row locator scoped to a known header-to-cell mapping) to avoid coupling to column position.

// Get the first data row’s "Created At" cell using semantic selectors
const firstRow = page
  .getByRole("row")
  .filter({ has: page.getByRole("cell") })
  .first();
const createdAtCell = firstRow.getByRole("cell").nth(7); // 0-indexed, 8th column = index 7

const column = page.getByRole("columnheader", {
  name: "Created At",
  exact: true,
});

// Click twice to sort descending — newest entries first
await column.click();
await column.click();

// After sorting descending, the first row should have a non-empty "Created At"
await expect(createdAtCell).not.toBeEmpty();
Localization Drift

goToSettingsPage() now hardcodes name: "Settings" for the menuitem. This will fail once translations are fixed/enabled or if the label changes. If possible, centralize the selector to use the same translation source (or a stable test id) so tests don’t need another update when RHDHBUGS-2552 is resolved.

async goToSettingsPage() {
  await expect(this.page.locator("nav[id='global-header']")).toBeVisible();
  await this.openProfileDropdown();
  // TODO: RHDHBUGS-2552 - Strings not getting translated
  // The profile dropdown renders Settings as a menuitem, not an <a> link
  const settingsItem = this.page.getByRole("menuitem", {
    name: "Settings",
  });
  await expect(settingsItem).toBeVisible();
  await settingsItem.click();
📄 References
  1. redhat-developer/rhdh/e2e-tests/playwright/e2e/settings.spec.ts [1-81]
  2. redhat-developer/rhdh/e2e-tests/playwright/e2e/github-happy-path.spec.ts [15-230]

@rhdh-qodo-merge
Copy link
Copy Markdown

PR Type

Bug fix, Tests


Description

  • Fix Settings link selector in profile dropdown using semantic menuitem role

  • Replace brittle CSS nth-child selectors with semantic getByRole selectors

  • Clear search filter before catalog timestamp sorting test

  • Add .local-test directory to ESLint and gitignore exclusions


File Walkthrough

Relevant files
Bug fix
catalog-timestamp.spec.ts
Replace CSS selectors with semantic getByRole selectors   

e2e-tests/playwright/e2e/catalog-timestamp.spec.ts

  • Remove unused UI_HELPER_ELEMENTS import
  • Replace brittle CSS nth-child selectors with semantic getByRole
    selectors
  • Add search filter clearing before sorting test to ensure consistent
    state
  • Use getByRole for table navigation and column header interaction
+29/-11 
default-global-header.spec.ts
Fix Settings verification using menuitem role selector     

e2e-tests/playwright/e2e/default-global-header.spec.ts

  • Replace verifyLinkVisible() with getByRole('menuitem') check for
    Settings
  • Update Settings click interaction to use menuitem role instead of link
  • Add TODO comment referencing translation issue RHDHBUGS-2552
+11/-4   
ui-helper.ts
Update Settings navigation to use menuitem selector           

e2e-tests/playwright/utils/ui-helper.ts

  • Update goToSettingsPage() to use getByRole('menuitem') instead of
    clickLink()
  • Replace link-based selector with semantic menuitem selector
  • Add explanatory comment about Settings being rendered as menuitem
+7/-5     
Configuration changes
eslint.config.js
Add .local-test to ESLint ignores                                               

e2e-tests/eslint.config.js

  • Add .local-test directory to ESLint ignores array
  • Prevent ESLint from scanning deployment artifacts
+6/-1     

@rhdh-qodo-merge
Copy link
Copy Markdown

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Properly validate list sorting logic

Improve the sorting test by comparing the timestamps of the first two rows to
confirm they are in the correct descending order, instead of just checking if
the first cell is not empty.

e2e-tests/playwright/e2e/catalog-timestamp.spec.ts [91-92]

 // After sorting descending, the first row should have a non-empty "Created At"
 await expect(createdAtCell).not.toBeEmpty();
 
+const dataRows = page.getByRole("row").filter({ has: page.getByRole("cell") });
+if ((await dataRows.count()) > 1) {
+  const firstTimestamp = await createdAtCell.innerText();
+  const secondTimestamp = await dataRows.nth(1).getByRole("cell").nth(7).innerText();
+
+  // Assuming timestamps are in a format parsable by new Date()
+  expect(new Date(firstTimestamp).getTime()).toBeGreaterThanOrEqual(
+    new Date(secondTimestamp).getTime(),
+  );
+}
+
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies that the test does not validate the sorting order, only that a cell is not empty, and provides a robust solution to properly check the sorting logic.

Medium
Verify descending sort attribute

Add an assertion to verify the column header's aria-sort attribute is set to
"descending" after clicking it, ensuring the UI state correctly reflects the
sort order.

e2e-tests/playwright/e2e/catalog-timestamp.spec.ts [88-92]

 await column.click();
 await column.click();
-// After sorting descending, the first row should have a non-empty "Created At"
+await expect(column).toHaveAttribute("aria-sort", "descending");
 await expect(createdAtCell).not.toBeEmpty();
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: This is a valuable suggestion that improves test robustness by adding an assertion for the UI's accessibility state (aria-sort), which directly confirms the sort order is reflected in the UI.

Medium
General
Add translation fallback for label

Instead of a hardcoded "Settings" string, use the translation key with a
fallback (t[...] || "Settings") to automatically use the translation when it
becomes available.

e2e-tests/playwright/e2e/default-global-header.spec.ts [121-123]

-// TODO: RHDHBUGS-2552 - Strings not getting translated
-// t["plugin.global-header"][lang]["profile.settings"],
-name: "Settings",
+const settingsLabel = t["plugin.global-header"][lang]["profile.settings"] || "Settings";
+name: settingsLabel,

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 6

__

Why: The suggestion provides a clean, maintainable solution for a known translation issue noted in a TODO, making the test code more robust and future-proof.

Low
Remove redundant visibility check before click

Remove the redundant await expect(settingsItem).toBeVisible() check before
.click(), as Playwright's click action already includes an implicit wait for
visibility.

e2e-tests/playwright/utils/ui-helper.ts [259-260]

-const settingsItem = this.page.getByRole("menuitem", {
+// TODO: RHDHBUGS-2552 - Strings not getting translated
+// The profile dropdown renders Settings as a menuitem, not an <a> link
+await this.page.getByRole("menuitem", {
   name: "Settings",
-});
-await expect(settingsItem).toBeVisible();
-await settingsItem.click();
+}).click();

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 3

__

Why: The suggestion correctly points out that the visibility check is redundant due to Playwright's auto-waiting mechanism, offering a minor improvement to code conciseness.

Low
  • More

Comment on lines +64 to +69
// Clear search filter from previous test to show all components
const clearButton = page.getByRole("button", { name: "clear search" });
if (await clearButton.isVisible()) {
await clearButton.click();
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action required

1. Filter clear not verified 🐞 Bug ☼ Reliability

catalog-timestamp.spec.ts attempts to clear the previous test’s catalog search via an optional
“clear search” button click but never asserts the filter is actually removed, so the sort test can
run against a single filtered row and stop validating sort behavior reliably. This can yield false
positives or flakiness depending on whether the clear affordance exists/works in that UI variant.
Agent Prompt
### Issue description
`catalog-timestamp.spec.ts` relies on an optional click of a "clear search" control to reset state from the prior test, but never verifies that the search field is actually cleared. If the control is absent or the click doesn’t clear the input (UI variant/accessibility name differences), the sorting test may run with the old filter still applied.

### Issue Context
The previous test sets a search filter via `uiHelper.searchInputPlaceholder(...)`, and the suite reuses the same `page` across tests; `beforeEach` does not clear the field.

### Fix Focus Areas
- e2e-tests/playwright/e2e/catalog-timestamp.spec.ts[48-93]
- e2e-tests/playwright/support/page-objects/page-obj.ts[14-17]

### Suggested change
After attempting to clear, explicitly clear the known search input (using `SEARCH_OBJECTS_COMPONENTS.placeholderSearch`/`ariaLabelSearch`) and assert it is empty (e.g., `await expect(page.locator(SEARCH_OBJECTS_COMPONENTS.placeholderSearch)).toHaveValue('')`) before proceeding to row/sort assertions.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

@zdrapela
Copy link
Copy Markdown
Member Author

zdrapela commented Apr 1, 2026

/test e2e-ocp-helm

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 1, 2026

Image was built and published successfully. It is available at:

@zdrapela
Copy link
Copy Markdown
Member Author

zdrapela commented Apr 1, 2026

/test e2e-ocp-helm

@zdrapela
Copy link
Copy Markdown
Member Author

zdrapela commented Apr 1, 2026

/test e2e-ocp-helm-nightly

@zdrapela
Copy link
Copy Markdown
Member Author

zdrapela commented Apr 7, 2026

/test e2e-ocp-helm-nightly

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 7, 2026

Image was built and published successfully. It is available at:

@openshift-ci
Copy link
Copy Markdown

openshift-ci bot commented Apr 7, 2026

@zdrapela: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/e2e-ocp-helm-nightly 9e783c0 link false /test e2e-ocp-helm-nightly

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant