Skip to content

fix(e2e): verify and enable settings test#4686

Open
teknaS47 wants to merge 1 commit intoredhat-developer:release-1.8from
teknaS47:settings-page-test
Open

fix(e2e): verify and enable settings test#4686
teknaS47 wants to merge 1 commit intoredhat-developer:release-1.8from
teknaS47:settings-page-test

Conversation

@teknaS47
Copy link
Copy Markdown
Member

Description

Verify that the PR images render the settings page correctly and enable the settings page e2e tests.

Which issue(s) does this PR fix

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

@rhdh-qodo-merge
Copy link
Copy Markdown

rhdh-qodo-merge Bot commented Apr 23, 2026

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📎 Requirement gaps (0)

Grey Divider


Advisory comments

1. Unsupported LOCALE crashes test 🐞 Bug ☼ Reliability
Description
getCurrentLanguage() type-asserts process.env.LOCALE as a supported locale, but only {en, fr}
are actually loaded; when LOCALE is unsupported, getTranslations() falls back to en while the
test still indexes translations by the unsupported lang, throwing at runtime. Enabling the
Settings test makes this failure path immediately reachable when running Playwright with locales
like de (as documented).
Code

e2e-tests/playwright/e2e/settings.spec.ts[R31-34]

+  test(`Verify settings page`, async ({ page }) => {
    await page
      .getByRole("button", {
        name: t["plugin.quickstart"][lang]["footer.hide"],
Relevance

⭐ Low

Repo already loads de/es translations and Locale includes them; unsupported-locale crash scenario
likely obsolete/incorrect.

PR-#4519

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The enabled test indexes translations as t[plugin][lang][key]. lang comes from
getCurrentLanguage() which returns process.env.LOCALE cast to the Locale union, but locales
only includes en and fr. For an unsupported LOCALE (e.g. de), getLocale() falls back to
en, so t contains only ...['en']... keys while the test still indexes ...['de']...,
producing undefined[...] at runtime. The repo docs explicitly suggest running with LOCALE=de,
making this a realistic execution mode.

e2e-tests/playwright/e2e/settings.spec.ts[10-11]
e2e-tests/playwright/e2e/settings.spec.ts[31-40]
e2e-tests/playwright/e2e/localization/locale.ts[17-27]
docs/e2e-tests/e2e-i18n-guide.md[149-156]

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

### Issue description
`getCurrentLanguage()` returns `process.env.LOCALE` with a type assertion even when that locale is not supported by the loaded `locales` map. Tests then index translation JSON by this unvalidated `lang`, which can throw at runtime.

### Issue Context
- `locales` only contains `{ en, fr }`, but docs suggest running with `LOCALE=de`.
- When LOCALE is unsupported, `getLocale()` correctly falls back to `en`, but the separate `lang` variable remains unsupported, causing `t[...][lang]` to be `undefined`.

### Fix Focus Areas
- e2e-tests/playwright/e2e/localization/locale.ts[17-27]
- e2e-tests/playwright/e2e/settings.spec.ts[31-40]
- docs/e2e-tests/e2e-i18n-guide.md[149-156]

### Suggested fix
- Update `getCurrentLanguage()` to normalize/validate `process.env.LOCALE`:
 - If `process.env.LOCALE` is not a key in `locales`, return `'en'`.
- (Optional) Update docs to list only actually-supported locales, or add the missing locale(s) to `locales` if `de` is intended to be supported.

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


Grey Divider

Qodo Logo

@rhdh-qodo-merge
Copy link
Copy Markdown

Review Summary by Qodo

Enable and fix settings page e2e tests

🧪 Tests

Grey Divider

Walkthroughs

Description
• Enable settings page e2e test by removing fixme marker
• Add test component annotation for test categorization
• Fix French language text verification to use locale data
• Uncomment and correct internationalized text assertions
Diagram
flowchart LR
  A["Settings Test Suite"] -->|Add beforeAll hook| B["Component Annotation"]
  A -->|Remove fixme marker| C["Enable Test Execution"]
  A -->|Fix assertions| D["Use Locale Data"]
  D -->|Replace hardcoded text| E["French Translations"]
Loading

Grey Divider

File Changes

1. e2e-tests/playwright/e2e/settings.spec.ts 🧪 Tests +9/-4

Enable settings test and fix locale assertions

• Added beforeAll hook with component annotation for test categorization
• Changed test from test.fixme() to test() to enable execution
• Replaced hardcoded "Profile" text with locale-based French translation
• Uncommented and corrected French language text verification assertions

e2e-tests/playwright/e2e/settings.spec.ts


Grey Divider

Qodo Logo

@github-actions
Copy link
Copy Markdown
Contributor

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

@openshift-ci
Copy link
Copy Markdown

openshift-ci Bot commented Apr 23, 2026

@teknaS47: 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 4f1759b link true /test e2e-ocp-helm

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