test: added extensive help panel tests#307
Open
jjaquish wants to merge 7 commits into
Open
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplaces the external Playwright globalSetup with a local proxy-aware ChangesHelp Panel E2E Testing Infrastructure and Test Suite
sequenceDiagram
participant Setup as Playwright Global Setup
participant Browser as Browser
participant App as Help Panel App
participant API as API Server
participant Storage as Storage State
participant Test as Playwright Test
Setup->>Browser: launch Chromium (with proxy if configured)
Browser->>App: create context(baseURL, ignoreHTTPSErrors, proxy)
Setup->>App: intercept & disable cookie-consent requests
Setup->>App: navigate to login page
Setup->>App: fill credentials from E2E_USER/E2E_PASSWORD
App->>API: authenticate request
API-->>App: set auth cookies/session
Setup->>App: navigate to landing page (/)
App-->>Setup: render greeting visible
Setup->>Storage: write context.storageState(path)
Test->>Browser: start test using saved storageState
Test->>App: navigate to dashboard
Test->>App: open help panel
Test->>App: select a tab (Learn/API/Search/Support/Feedback)
App->>API: fetch tab content
API-->>App: return data
Test->>App: interact & assert expected UI/behavior
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 |
jjaquish
changed the title
apinkert
reviewed
May 5, 2026
Collaborator
apinkert
left a comment
apinkert
left a comment
There was a problem hiding this comment.
Hey Jack, this is looking pretty good! Looks like there are some failures in the CI related to the search panel e2e test. Also, we can remove all the knowledgebase tests as we're going to be removing it from the help panel.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (1)
playwright/global-setup-with-proxy.ts (1)
7-17: ⚡ Quick winReplace
anywith proper Playwright types.
disableCookiePromptandloginuseanyfor all Playwright-specific parameters, losing type safety in test infrastructure.Page,Route, andRequestare all named exports from'playwright'.♻️ Proposed fix
-import { chromium, type FullConfig } from 'playwright'; +import { chromium, type FullConfig, type Page, type Route, type Request } from 'playwright'; -async function disableCookiePrompt(page: any) { - await page.route('**/*', async (route: any, request: any) => { +async function disableCookiePrompt(page: Page) { + await page.route('**/*', async (route: Route, request: Request) => { if (request.url().includes('consent.trustarc.com') && request.resourceType() !== 'document') { await route.abort(); } else { await route.continue(); } }); } -async function login(page: any, user: string, password: string) { +async function login(page: Page, user: string, password: string) {🤖 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 `@playwright/global-setup-with-proxy.ts` around lines 7 - 17, The functions disableCookiePrompt and login currently use any for Playwright types; update their signatures to use Playwright's types by importing and using Page for the page parameter and typing the route handler parameters as (route: Route, request: Request) so the call to page.route has properly typed callback args; ensure you add the named imports (Page, Route, Request) from 'playwright' and replace all occurrences of any in disableCookiePrompt and login with these concrete types to restore type safety.
🤖 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 `@playwright/debug-global-setup.ts`:
- Around line 6-7: The current console.log calls print raw sensitive values from
config.projects[0].use.proxy and config.projects[0].use.storageState; replace
these with presence-only or redacted summaries: for proxy, log only whether a
proxy is configured and which keys are present (e.g., list keys like "server",
"username" but not their values) or mask values (e.g., replace characters with
asterisks), and for storageState log only that a storage state file/string
exists and its filename or size (not contents). Update the debug output in
debug-global-setup.ts to use a small helper (e.g., logProxySummary or redact) to
produce these summaries when referencing config and config.projects[0].use.* so
no secrets or full paths are emitted to CI logs.
In `@playwright/debug-login.spec.ts`:
- Around line 3-32: The test "debug login page" currently contains only logging
calls and no assertions, so it will always pass; either mark it as skipped
(test.skip/test.fixme) or add explicit Playwright assertions to validate the
critical login state (e.g., import expect from '@playwright/test' and assert
things like page.title() includes expected text, page.getByLabel('Red Hat
login').count() is > 0, or that specific "Lockdown" or error counts are 0);
update the test declaration test('debug login page', ...) accordingly and
replace the console checks with expect(...) assertions (or change to
test.skip/test.fixme if you intend to keep it diagnostic).
In `@playwright/E2E_SETUP_NOTES.md`:
- Around line 7-9: Replace the hardcoded credentials and proxy entries
(E2E_USER, E2E_PASSWORD, E2E_PROXY) with non-sensitive placeholder values and
update every occurrence including inline command examples and references in
playwright/README.md; change examples to point to an internal secrets store
(e.g., Vault or team wiki) and document how to fetch them, remove the plaintext
password 'PlatformExperience!1' everywhere, and ensure the exposed stage account
credentials are rotated immediately after committing this change.
In `@playwright/help-panel-feedback-tab.spec.ts`:
- Around line 227-236: The breadcrumb-click step is non-deterministic because it
only clicks when isVisible is truthy; instead assert the breadcrumb is visible
then perform the click to fail fast if missing. Replace the conditional around
breadcrumb/isVisible with an explicit assertion using
expect(breadcrumb).toBeVisible(), then call breadcrumb.click(),
waitForTimeout(500) and assert the heading via expect(page.getByRole('heading',
{ name: /tell us about your experience/i })).toBeVisible(); keep the same
locators (breadcrumb = page.locator(...)) and waits.
In `@playwright/help-panel-learn-tab.spec.ts`:
- Around line 69-102: The test "toggles between All and bundle scope" currently
silently passes when the scope toggle is missing; change the early conditional
so that when scopeToggle (locator
'[data-ouia-component-id="help-panel-scope-toggle"]') is not visible you call
test.skip('scope toggle not present - skipping bundle-specific assertions')
instead of simply returning, otherwise proceed with the existing flow (use
allToggle and bundleToggle locators and the toolbar check as before); ensure you
await scopeToggle.isVisible() then skip via test.skip to make non-applicability
explicit.
In `@playwright/help-panel-search-tab.spec.ts`:
- Around line 203-229: The test "toggles recommended content scope" currently
returns a vacuous pass when the recommendedToggle locator isn't visible; after
awaiting recommendedToggle.isVisible() call test.skip(true, 'recommended scope
toggle not present on this page') (or test.skip(!isVisible, ...)) and return so
the test is reported as skipped instead of passing; update the block around the
recommendedToggle locator
(data-ouia-component-id="help-panel-recommended-scope-toggle") to perform the
visibility await, call test.skip if false, and only proceed with the existing
clicks/assertions (allToggle, bundleToggle, recommendedItems) when visible.
- Around line 231-276: The two Playwright tests "bookmarks a learning resource
from search results" and "favorites a service from search results" contain no
assertions and therefore should be skipped until proper assertions are added;
update the test declarations for those cases (the test(...) calls with those
exact titles) to test.skip(...) so they are not executed and give false
confidence, and add a TODO comment referencing the need to assert the post-click
state (e.g., icon/state change or API response) before re-enabling them.
In `@playwright/help-panel-support-tab.spec.ts`:
- Around line 41-78: Both tests ("displays empty state when no support cases
exist" and "opens Customer Portal when clicking \"Open a support case\"")
currently short-circuit with if (isEmptyVisible) { ... } causing vacuous passes;
replace the conditional checks on the emptyState locator with explicit
assertions (e.g., await expect(emptyState).toBeVisible() or
expect(isEmptyVisible).toBeTruthy()) so the test fails when the empty state is
not present, and then proceed to interact with openCaseButton and popupPromise
(or convert the check into a clear precondition using test.skip/test.fail with a
message) to ensure those branches are validated rather than silently skipped.
- Around line 24-26: Replace the fixed sleep (await page.waitForTimeout(2000))
with a state-based wait that checks the support panel UI using the same
selectors and pattern used in the following test: remove the page.waitForTimeout
call and wrap a check for the panel elements (the exact locators used in the
next test) in an expect(async () => { /* await
page.locator(...).isVisible()/count()/textContent() */ }).toPass() so the test
waits for either the empty-state or table to appear instead of relying on a
timeout.
In `@playwright/README.md`:
- Around line 18-21: Replace the hardcoded credentials (E2E_USER, E2E_PASSWORD)
and the cross-repo .env pointer in the PLAYWRIGHT README with neutral
placeholders and a reference to the internal team credentials document;
specifically, update the lines exporting E2E_USER and E2E_PASSWORD to use
placeholder values (e.g., <E2E_USER_PLACEHOLDER>, <E2E_PASSWORD_PLACEHOLDER>),
keep or parameterize E2E_PROXY if needed, remove the reference to
insights-rbac-ui/e2e/auth/.env.v1-orgadmin, and add a short note directing
readers to the internal credentials store or team doc for obtaining real test
account credentials.
---
Nitpick comments:
In `@playwright/global-setup-with-proxy.ts`:
- Around line 7-17: The functions disableCookiePrompt and login currently use
any for Playwright types; update their signatures to use Playwright's types by
importing and using Page for the page parameter and typing the route handler
parameters as (route: Route, request: Request) so the call to page.route has
properly typed callback args; ensure you add the named imports (Page, Route,
Request) from 'playwright' and replace all occurrences of any in
disableCookiePrompt and login with these concrete types to restore type safety.
🪄 Autofix (Beta)
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: Enterprise
Run ID: be23765d-eb96-4512-87fc-641b920b77b6
📒 Files selected for processing (16)
playwright.config.tsplaywright.debug-global.config.tsplaywright/E2E_SETUP_NOTES.mdplaywright/HELP_PANEL_E2E_TESTS.mdplaywright/README.mdplaywright/all-learning-resources.spec.tsplaywright/debug-global-setup.tsplaywright/debug-login.spec.tsplaywright/global-setup-with-proxy.tsplaywright/help-panel-api-tab.spec.tsplaywright/help-panel-feedback-tab.spec.tsplaywright/help-panel-learn-tab.spec.tsplaywright/help-panel-search-tab.spec.tsplaywright/help-panel-support-tab.spec.tsplaywright/help-panel.spec.tsplaywright/playwright.debug.config.ts
Comment on lines
+6
to
+7
| console.log('Proxy config:', JSON.stringify(config.projects[0].use.proxy, null, 2)); | ||
| console.log('StorageState:', config.projects[0].use.storageState); |
There was a problem hiding this comment.
Do not log raw proxy/storage configuration values.
At Line 6–7, logging full proxy/storage values can leak credentials or sensitive paths in CI logs. Redact or log presence-only metadata.
Suggested change
- console.log('Proxy config:', JSON.stringify(config.projects[0].use.proxy, null, 2));
- console.log('StorageState:', config.projects[0].use.storageState);
+ const hasProxy = Boolean(config.projects[0].use.proxy);
+ const hasStorageState = Boolean(config.projects[0].use.storageState);
+ console.log('Proxy configured:', hasProxy);
+ console.log('StorageState configured:', hasStorageState);📝 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
| console.log('Proxy config:', JSON.stringify(config.projects[0].use.proxy, null, 2)); | |
| console.log('StorageState:', config.projects[0].use.storageState); | |
| const hasProxy = Boolean(config.projects[0].use.proxy); | |
| const hasStorageState = Boolean(config.projects[0].use.storageState); | |
| console.log('Proxy configured:', hasProxy); | |
| console.log('StorageState configured:', hasStorageState); |
🤖 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 `@playwright/debug-global-setup.ts` around lines 6 - 7, The current console.log
calls print raw sensitive values from config.projects[0].use.proxy and
config.projects[0].use.storageState; replace these with presence-only or
redacted summaries: for proxy, log only whether a proxy is configured and which
keys are present (e.g., list keys like "server", "username" but not their
values) or mask values (e.g., replace characters with asterisks), and for
storageState log only that a storage state file/string exists and its filename
or size (not contents). Update the debug output in debug-global-setup.ts to use
a small helper (e.g., logProxySummary or redact) to produce these summaries when
referencing config and config.projects[0].use.* so no secrets or full paths are
emitted to CI logs.
Comment on lines
+3
to
+32
| test('debug login page', async ({ page }) => { | ||
| // Navigate to the base URL | ||
| await page.goto('/', { waitUntil: 'load', timeout: 60000 }); | ||
|
|
||
| // Take a screenshot | ||
| await page.screenshot({ path: 'playwright/debug-login-page.png', fullPage: true }); | ||
|
|
||
| // Get page title | ||
| const title = await page.title(); | ||
| console.log('Page title:', title); | ||
|
|
||
| // Get page URL | ||
| console.log('Page URL:', page.url()); | ||
|
|
||
| // Check for "Lockdown" text | ||
| const lockdownCount = await page.locator('text=Lockdown').count(); | ||
| console.log('Lockdown text count:', lockdownCount); | ||
|
|
||
| // Check for login form | ||
| const loginFieldCount = await page.getByLabel('Red Hat login').count(); | ||
| console.log('Login field count:', loginFieldCount); | ||
|
|
||
| // Get visible text on page | ||
| const bodyText = await page.locator('body').textContent(); | ||
| console.log('First 500 chars of body:', bodyText?.substring(0, 500)); | ||
|
|
||
| // Try to find any error messages | ||
| const errorText = await page.locator('text=/error|invalid|failed|denied/i').count(); | ||
| console.log('Error-like text count:', errorText); | ||
| }); |
There was a problem hiding this comment.
This diagnostic spec always passes and can hide failures.
At Line 3, the test contains no assertions, so it cannot fail on broken login states. Mark it test.skip/test.fixme or add explicit pass/fail criteria.
🤖 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 `@playwright/debug-login.spec.ts` around lines 3 - 32, The test "debug login
page" currently contains only logging calls and no assertions, so it will always
pass; either mark it as skipped (test.skip/test.fixme) or add explicit
Playwright assertions to validate the critical login state (e.g., import expect
from '@playwright/test' and assert things like page.title() includes expected
text, page.getByLabel('Red Hat login').count() is > 0, or that specific
"Lockdown" or error counts are 0); update the test declaration test('debug login
page', ...) accordingly and replace the console checks with expect(...)
assertions (or change to test.skip/test.fixme if you intend to keep it
diagnostic).
d0c580b to
799bfa2
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (4)
playwright/help-panel-search-tab.spec.ts (3)
254-276:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
favorites a servicestill has zero assertions.Like the bookmark test, this test clicks the favorite button if visible but doesn't verify the state changed. Both tests should be skipped until assertions are implemented.
🛡️ Proposed fix to skip until assertions added
- test('favorites a service from search results', async ({ page }) => { + test.skip('favorites a service from search results', async ({ page }) => { + // TODO: Add assertion that favorite icon toggles state after click // Search for services🤖 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 `@playwright/help-panel-search-tab.spec.ts` around lines 254 - 276, The test case "favorites a service from search results" currently has no assertions (see the test declared as test('favorites a service from search results', ...)), so update it to be skipped until assertions are implemented by replacing it with a skipped test (e.g., use test.skip with the same title) to match the bookmark test approach; keep the existing steps (searchInput, searchResults, favoriteButton) intact so it's easy to re-enable later and add proper assertions validating the favorite state change.
203-229:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
toggles recommended content scopestill has the vacuous-pass pattern.When
recommendedToggleis not visible, the test passes with zero assertions inside the conditional block. This should usetest.skipto explicitly mark the test as skipped rather than passing.🛡️ Proposed fix to add explicit skip
test('toggles recommended content scope', async ({ page }) => { // Check if we're on a bundle page (toggle should be visible) const recommendedToggle = page.locator('[data-ouia-component-id="help-panel-recommended-scope-toggle"]'); const isVisible = await recommendedToggle.isVisible(); + + test.skip(!isVisible, 'recommended scope toggle not present on this page'); if (isVisible) {🤖 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 `@playwright/help-panel-search-tab.spec.ts` around lines 203 - 229, The test "toggles recommended content scope" currently can vacuously pass when recommendedToggle (locator variable) is not visible; change it to explicitly skip instead of silently passing: after obtaining recommendedToggle and computing isVisible, call test.skip(!isVisible, 'recommended toggle not present on this page') so the test is marked skipped when the toggle is absent, then proceed with the existing awaits/assertions (allToggle, bundleToggle, recommendedItems) only when not skipped.
231-252:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
bookmarks a learning resourcestill has zero assertions.This test clicks the bookmark button if visible but includes no assertion to verify the bookmark state changed. The inline comment acknowledges this limitation. Until proper assertions are added, the test creates false confidence.
🛡️ Proposed fix to skip until assertions added
- test('bookmarks a learning resource from search results', async ({ page }) => { + test.skip('bookmarks a learning resource from search results', async ({ page }) => { + // TODO: Add assertion that bookmark icon toggles state after click // Search for quickstarts🤖 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 `@playwright/help-panel-search-tab.spec.ts` around lines 231 - 252, The test "bookmarks a learning resource from search results" currently performs actions but has no assertions; either add a real assertion verifying the bookmark state (e.g., after clicking the locator bookmarkButton check a state change via an accessible attribute like aria-pressed or an icon/class change, or confirm the backend API result), referencing the existing locator variable bookmarkButton and the searchInput flow, or temporarily disable the test by changing test('bookmarks a learning resource from search results', ...) to test.skip(...) until you implement the proper assertion logic; ensure you wait for the API/result update before asserting (use page.waitForResponse or explicit attribute/state checks rather than fixed timeouts).playwright/help-panel-support-tab.spec.ts (1)
186-192:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReplace fixed sleep with state-based wait for panel closure.
The
waitForTimeout(500)at line 189 introduces timing brittleness. After closing the panel, wait for the close button to become hidden rather than using a fixed delay.🔄 Proposed fix
const closeButton = page.locator('[data-ouia-component-id="help-panel-close-button"]'); await closeButton.click(); - await page.waitForTimeout(500); + await expect(closeButton).toBeHidden(); // Reopen await page.getByLabel('Toggle help panel').click();🤖 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 `@playwright/help-panel-support-tab.spec.ts` around lines 186 - 192, The test currently uses a fixed sleep after clicking closeButton which is brittle; replace the waitForTimeout(500) with a state-based wait on the closeButton locator (closeButton) so the test waits for the panel to actually close (e.g., wait until closeButton is hidden/removed) before proceeding to reopen via page.getByLabel('Toggle help panel').click(); ensure you remove the hardcoded timeout and use the locator's wait-for-state API to detect closure.
🧹 Nitpick comments (3)
playwright/help-panel-search-tab.spec.ts (2)
36-36: 🏗️ Heavy liftReplace hardcoded
waitForTimeoutcalls with event-driven waits.The test suite uses many hardcoded delays (e.g., lines 36, 57, 72, 128, 156, 174, 182, 210, 215, 224, 247, 271, 282). Playwright's event-driven waiting strategies (
waitForResponse,waitForLoadState, element visibility) are more reliable and faster than arbitrary timeouts.💡 Example refactoring approaches
For API-driven updates, wait for the network response instead of timeout:
// Instead of: await searchInput.fill('Insights'); await page.waitForTimeout(SEARCH_DEBOUNCE_MS + 500); // Use: await searchInput.fill('Insights'); await page.waitForResponse(resp => resp.url().includes('/api/search') && resp.status() === 200 );For UI state changes after interactions, rely on element visibility:
// Instead of: await allToggle.click(); await page.waitForTimeout(1000); // Use: await allToggle.click(); await expect(recommendedItems.first()).toBeVisible({ timeout: 10000 }); // (already present after the wait - just remove the waitForTimeout)For debounced input, a single short wait followed by visibility check is acceptable:
await searchInput.fill('query'); await page.waitForTimeout(SEARCH_DEBOUNCE_MS + 100); // reasonable buffer await expect(searchResults).toBeVisible({ timeout: 15000 });Also applies to: 57-57, 72-72, 128-128, 156-156, 174-174, 182-182, 210-210, 215-215, 224-224, 247-247, 271-271, 282-282
🤖 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 `@playwright/help-panel-search-tab.spec.ts` at line 36, Replace hardcoded await page.waitForTimeout(...) usages in help-panel-search-tab.spec.ts with event-driven waits: after searchInput.fill(...) wait for the network response (waitForResponse matching '/api/search' and status 200) or a short debounce buffer using SEARCH_DEBOUNCE_MS followed by expect(searchResults).toBeVisible(...); after interactions like allToggle.click() or other UI clicks wait for element state changes with expect(recommendedItems.first()).toBeVisible(...) or page.waitForSelector(...) and for navigation use page.waitForLoadState('networkidle') or waitForResponse as appropriate; update every occurrence of page.waitForTimeout (lines referencing searchInput, allToggle, recommendedItems, searchResults, SEARCH_DEBOUNCE_MS) to use these event-driven waits for reliability and speed.
300-302: 💤 Low valueRemove console.log from test code.
Console output in tests can clutter CI logs and doesn't provide value in automated runs. Consider removing this log statement or replacing it with a proper assertion if type indicator validation is important.
♻️ Proposed fix
- if (labelCount > 0) { - console.log('✓ Found result type indicators (labels/badges)'); - } + // Results include type indicators (labels/badges) when present + // labelCount > 0 indicates diverse result types🤖 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 `@playwright/help-panel-search-tab.spec.ts` around lines 300 - 302, Remove the console.log line that prints '✓ Found result type indicators (labels/badges)' from the test and replace it with a proper assertion using the existing labelCount variable (for example assert labelCount > 0 using the test framework's expect/should). Locate the statement in help-panel-search-tab.spec.ts where labelCount is computed and remove the console.log(...) and add an assertion (e.g., expect(labelCount).toBeGreaterThan(0) or similar) to ensure the type indicator validation is enforced in the test.playwright/E2E_SETUP_NOTES.md (1)
46-48: 💤 Low valueAdd language identifier to fenced code block.
The fenced code block displaying the SSO redirect URL should specify a language (e.g.,
textorurl) to satisfy markdown linting rules.♻️ Proposed fix
-``` +```text https://sso.stage.redhat.com/auth/realms/redhat-external/protocol/openid-connect/auth... ```As per coding guidelines, the static analysis tool markdownlint-cli2 flags fenced code blocks without language specifiers (MD040).
🤖 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 `@playwright/E2E_SETUP_NOTES.md` around lines 46 - 48, The fenced code block containing the SSO redirect URL currently lacks a language identifier (causing MD040); update the block in E2E_SETUP_NOTES.md by adding a language specifier such as "text" or "url" immediately after the opening backticks (i.e., change the triple backticks before the URL to ```text or ```url) so the markdown linter recognizes the code block language.
🤖 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.
Duplicate comments:
In `@playwright/help-panel-search-tab.spec.ts`:
- Around line 254-276: The test case "favorites a service from search results"
currently has no assertions (see the test declared as test('favorites a service
from search results', ...)), so update it to be skipped until assertions are
implemented by replacing it with a skipped test (e.g., use test.skip with the
same title) to match the bookmark test approach; keep the existing steps
(searchInput, searchResults, favoriteButton) intact so it's easy to re-enable
later and add proper assertions validating the favorite state change.
- Around line 203-229: The test "toggles recommended content scope" currently
can vacuously pass when recommendedToggle (locator variable) is not visible;
change it to explicitly skip instead of silently passing: after obtaining
recommendedToggle and computing isVisible, call test.skip(!isVisible,
'recommended toggle not present on this page') so the test is marked skipped
when the toggle is absent, then proceed with the existing awaits/assertions
(allToggle, bundleToggle, recommendedItems) only when not skipped.
- Around line 231-252: The test "bookmarks a learning resource from search
results" currently performs actions but has no assertions; either add a real
assertion verifying the bookmark state (e.g., after clicking the locator
bookmarkButton check a state change via an accessible attribute like
aria-pressed or an icon/class change, or confirm the backend API result),
referencing the existing locator variable bookmarkButton and the searchInput
flow, or temporarily disable the test by changing test('bookmarks a learning
resource from search results', ...) to test.skip(...) until you implement the
proper assertion logic; ensure you wait for the API/result update before
asserting (use page.waitForResponse or explicit attribute/state checks rather
than fixed timeouts).
In `@playwright/help-panel-support-tab.spec.ts`:
- Around line 186-192: The test currently uses a fixed sleep after clicking
closeButton which is brittle; replace the waitForTimeout(500) with a state-based
wait on the closeButton locator (closeButton) so the test waits for the panel to
actually close (e.g., wait until closeButton is hidden/removed) before
proceeding to reopen via page.getByLabel('Toggle help panel').click(); ensure
you remove the hardcoded timeout and use the locator's wait-for-state API to
detect closure.
---
Nitpick comments:
In `@playwright/E2E_SETUP_NOTES.md`:
- Around line 46-48: The fenced code block containing the SSO redirect URL
currently lacks a language identifier (causing MD040); update the block in
E2E_SETUP_NOTES.md by adding a language specifier such as "text" or "url"
immediately after the opening backticks (i.e., change the triple backticks
before the URL to ```text or ```url) so the markdown linter recognizes the code
block language.
In `@playwright/help-panel-search-tab.spec.ts`:
- Line 36: Replace hardcoded await page.waitForTimeout(...) usages in
help-panel-search-tab.spec.ts with event-driven waits: after
searchInput.fill(...) wait for the network response (waitForResponse matching
'/api/search' and status 200) or a short debounce buffer using
SEARCH_DEBOUNCE_MS followed by expect(searchResults).toBeVisible(...); after
interactions like allToggle.click() or other UI clicks wait for element state
changes with expect(recommendedItems.first()).toBeVisible(...) or
page.waitForSelector(...) and for navigation use
page.waitForLoadState('networkidle') or waitForResponse as appropriate; update
every occurrence of page.waitForTimeout (lines referencing searchInput,
allToggle, recommendedItems, searchResults, SEARCH_DEBOUNCE_MS) to use these
event-driven waits for reliability and speed.
- Around line 300-302: Remove the console.log line that prints '✓ Found result
type indicators (labels/badges)' from the test and replace it with a proper
assertion using the existing labelCount variable (for example assert labelCount
> 0 using the test framework's expect/should). Locate the statement in
help-panel-search-tab.spec.ts where labelCount is computed and remove the
console.log(...) and add an assertion (e.g.,
expect(labelCount).toBeGreaterThan(0) or similar) to ensure the type indicator
validation is enforced in the test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 7ab31c7a-1684-472d-a024-d09baf59b704
📒 Files selected for processing (12)
playwright.config.tsplaywright/E2E_SETUP_NOTES.mdplaywright/HELP_PANEL_E2E_TESTS.mdplaywright/README.mdplaywright/all-learning-resources.spec.tsplaywright/global-setup-with-proxy.tsplaywright/help-panel-api-tab.spec.tsplaywright/help-panel-feedback-tab.spec.tsplaywright/help-panel-learn-tab.spec.tsplaywright/help-panel-search-tab.spec.tsplaywright/help-panel-support-tab.spec.tsplaywright/help-panel.spec.ts
✅ Files skipped from review due to trivial changes (3)
- playwright/help-panel.spec.ts
- playwright/README.md
- playwright/HELP_PANEL_E2E_TESTS.md
🚧 Files skipped from review as they are similar to previous changes (6)
- playwright/all-learning-resources.spec.ts
- playwright.config.ts
- playwright/help-panel-learn-tab.spec.ts
- playwright/help-panel-feedback-tab.spec.ts
- playwright/help-panel-api-tab.spec.ts
- playwright/global-setup-with-proxy.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
playwright/help-panel-search-tab.spec.ts (1)
205-211:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse auto-waiting visibility check before skip decision.
isVisible()on line 206 is an immediate check without auto-waiting. This can cause false skips if the toggle is still rendering. Replace withwaitFor({ state: 'visible', timeout: 3000 })to wait for the element to appear.Safer skip gate
const recommendedToggle = page.locator('[data-ouia-component-id="help-panel-recommended-scope-toggle"]'); - const isVisible = await recommendedToggle.isVisible(); + const isVisible = await recommendedToggle + .waitFor({ state: 'visible', timeout: 3000 }) + .then(() => true) + .catch(() => false); if (!isVisible) { test.skip(true, 'recommended scope toggle not present on this page'); return; }🤖 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 `@playwright/help-panel-search-tab.spec.ts` around lines 205 - 211, The current skip gate uses recommendedToggle.isVisible() which performs an immediate check and can falsely skip if the toggle is still rendering; replace that with an auto-waiting check by awaiting recommendedToggle.waitFor({ state: 'visible', timeout: 3000 }) (or catching a timeout to then call test.skip) so the test only skips after a short wait; update the logic around recommendedToggle, removing the isVisible call and using waitFor on the same locator to decide whether to skip.
🧹 Nitpick comments (1)
playwright/help-panel-search-tab.spec.ts (1)
36-37: ⚡ Quick winReplace fixed sleeps with condition-based waits.
The repeated
page.waitForTimeout()calls throughout the suite (Lines 36, 57, 72, 101, 128, 138, 156, 162, 174, 182, 188, 214, 219, 228, 239, 251, 263, 276, 287) introduce unnecessary delays and flakiness. Instead, wait explicitly for the UI condition that indicates readiness—either through auto-retrying assertions or locator-driven waits.Suggested refactor pattern
- await page.waitForTimeout(SEARCH_DEBOUNCE_MS + 500); - // Wait for results const searchResults = page.getByRole('list', { name: /search results/i }); await expect(searchResults).toBeVisible({ timeout: 15000 });Playwright's auto-retrying assertions and auto-waiting mechanisms (per official documentation) eliminate the flakiness that fixed sleeps often mask.
🤖 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 `@playwright/help-panel-search-tab.spec.ts` around lines 36 - 37, The test file uses many fixed sleeps via page.waitForTimeout(); replace each await page.waitForTimeout(...) with an explicit condition-based wait: wait for a specific locator/state (e.g., await page.locator('...').waitFor({ state: 'visible' }) or await expect(page.locator('...')).toHaveText(...)/toBeVisible()), or wait for a network signal (await page.waitForResponse(...) or await Promise.all([page.waitForResponse(...), page.click(...)])) that corresponds to the UI being ready. Find every occurrence of page.waitForTimeout in help-panel-search-tab.spec.ts and replace it with the appropriate locator-driven wait or auto-retrying expect that matches the UI action in that test (use locator.waitFor/expect(locator).toBeVisible/toHaveCount/toHaveText or page.waitForResponse/page.waitForRequest as applicable).
🤖 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.
Duplicate comments:
In `@playwright/help-panel-search-tab.spec.ts`:
- Around line 205-211: The current skip gate uses recommendedToggle.isVisible()
which performs an immediate check and can falsely skip if the toggle is still
rendering; replace that with an auto-waiting check by awaiting
recommendedToggle.waitFor({ state: 'visible', timeout: 3000 }) (or catching a
timeout to then call test.skip) so the test only skips after a short wait;
update the logic around recommendedToggle, removing the isVisible call and using
waitFor on the same locator to decide whether to skip.
---
Nitpick comments:
In `@playwright/help-panel-search-tab.spec.ts`:
- Around line 36-37: The test file uses many fixed sleeps via
page.waitForTimeout(); replace each await page.waitForTimeout(...) with an
explicit condition-based wait: wait for a specific locator/state (e.g., await
page.locator('...').waitFor({ state: 'visible' }) or await
expect(page.locator('...')).toHaveText(...)/toBeVisible()), or wait for a
network signal (await page.waitForResponse(...) or await
Promise.all([page.waitForResponse(...), page.click(...)])) that corresponds to
the UI being ready. Find every occurrence of page.waitForTimeout in
help-panel-search-tab.spec.ts and replace it with the appropriate locator-driven
wait or auto-retrying expect that matches the UI action in that test (use
locator.waitFor/expect(locator).toBeVisible/toHaveCount/toHaveText or
page.waitForResponse/page.waitForRequest as applicable).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 95ef7f94-f22c-4efe-83e7-6387df7d239f
📒 Files selected for processing (7)
playwright/E2E_SETUP_NOTES.mdplaywright/README.mdplaywright/global-setup-with-proxy.tsplaywright/help-panel-feedback-tab.spec.tsplaywright/help-panel-learn-tab.spec.tsplaywright/help-panel-search-tab.spec.tsplaywright/help-panel-support-tab.spec.ts
✅ Files skipped from review due to trivial changes (2)
- playwright/E2E_SETUP_NOTES.md
- playwright/README.md
🚧 Files skipped from review as they are similar to previous changes (4)
- playwright/help-panel-learn-tab.spec.ts
- playwright/global-setup-with-proxy.ts
- playwright/help-panel-support-tab.spec.ts
- playwright/help-panel-feedback-tab.spec.ts
Add Playwright E2E tests for all Help Panel tabs: - Learn tab: API integration, filtering, bookmarking (7 tests) - API tab: Bundle filtering, external links (5 tests) - Search tab: Search API, filters, recent queries (11 tests) - Support tab: Support cases API, empty states (8 tests) - Feedback tab: Forms, validation, bug reporting (12 tests) Total: 43 E2E tests with real stage API integration Test infrastructure: - Custom global setup with proxy support for local testing - Comprehensive documentation in playwright/README.md - Troubleshooting guide in playwright/E2E_SETUP_NOTES.md Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
jjaquish
force-pushed
the
jjaquish/RHCLOUD-45255
branch
from
ad30e0d to
9a16d61
Compare
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.
Description
RHCLOUD-45255
This is under the learn tab testing Jira, but covers all help panel testing tickets currently assigned to me.
This is to add extensive E2E testing in playwright for all features (where feasible) of the help panel and its various tabs.
As a note for review, I would suggest cloning this PR locally and using
npx playwright test --uito view them in the Playwright UI. The names are descriptive so coverage intention can be seen there, and the tests can be run if desired. This does not cover fixing any currently standing Playwright tests, only adding the new help panel tests.All running tests are currently passing, and some were added and skipped with notes to be addressed later.
Screenshots
Anything reviewers should know?
Checklist
AI disclosure