CONSOLE-5253: Extend Unit Tests: Console-App - Core Navigation & Orchestration#16452
Open
cajieh wants to merge 1 commit into
Open
CONSOLE-5253: Extend Unit Tests: Console-App - Core Navigation & Orchestration#16452cajieh wants to merge 1 commit into
cajieh wants to merge 1 commit into
Conversation
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
openshift-ci-robot
added
the
jira/valid-reference
Contributor
|
@cajieh: This pull request references CONSOLE-5253 which is a valid jira issue. Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "5.0.0" version, but no target version was set. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
Contributor
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
component/shared
Contributor
Author
|
/test e2e-gcp-console |
3 similar comments
Contributor
Author
|
/test e2e-gcp-console |
Contributor
Author
|
/test e2e-gcp-console |
Contributor
Author
|
/test e2e-gcp-console |
cajieh
force-pushed
the
extend-rtl-coverage-console-app
branch
from
21c1d74 to
5c9f27c
Compare
openshift-ci
Bot
added
the
needs-rebase
cajieh
force-pushed
the
extend-rtl-coverage-console-app
branch
from
5c9f27c to
6257c83
Compare
cajieh
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
cajieh
force-pushed
the
extend-rtl-coverage-console-app
branch
from
6257c83 to
d1a7fb5
Compare
Contributor
📝 WalkthroughWalkthroughThis pull request adds extensive test coverage across the OpenShift Console with 50+ new test files and modest accessibility/i18n improvements to cluster configuration components. The changes introduce Jest and React Testing Library specs for navigation (header, links, sections, extensions, hooks), node details (conditions, images, IP lists, roles, uptime), cluster configuration forms, console operator plugin metadata components, dashboard status displays, resource quota charts with percentage calculations, pod disruption budgets, quick start drawers, and selector components (access modes, volume modes). Additionally, access mode, cluster configuration, error boundary, and form layout components now include ARIA attributes (role, aria-busy, aria-label) and translated labels via react-i18next for improved semantic markup and internationalization. Suggested reviewers
🚥 Pre-merge checks | ✅ 12✅ Passed checks (12 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (17)
frontend/packages/console-app/src/components/access-modes/__tests__/access-mode.spec.tsx (1)
1-2: ⚡ Quick winUse
renderWithProvidersinstead of rawrenderin this spec.This suite is component-level and should use the shared test harness for consistency with app providers and future-proofing.
Proposed change
-import { render, screen } from '`@testing-library/react`'; +import { screen } from '`@testing-library/react`'; +import { renderWithProviders } from '`@console/shared/src/test-utils/unit-test-utils`'; ... - render(<AccessModeSelector {...defaultProps} />); + renderWithProviders(<AccessModeSelector {...defaultProps} />); ... - render(<AccessModeSelector {...defaultProps} loaded={false} />); + renderWithProviders(<AccessModeSelector {...defaultProps} loaded={false} />); ... - render(<AccessModeSelector {...defaultProps} />); + renderWithProviders(<AccessModeSelector {...defaultProps} />); ... - render(<AccessModeSelector {...defaultProps} description={description} />); + renderWithProviders(<AccessModeSelector {...defaultProps} description={description} />); ... - render(<AccessModeSelector {...defaultProps} />); + renderWithProviders(<AccessModeSelector {...defaultProps} />); ... - render(<AccessModeSelector {...defaultProps} onChange={onChange} />); + renderWithProviders(<AccessModeSelector {...defaultProps} onChange={onChange} />);As per coding guidelines, "Use reusable helper functions such as
renderWithProvidersandrenderHookWithProvidersfromfrontend/packages/console-shared/src/test-utils."Also applies to: 84-85, 90-91, 97-98, 104-105, 111-112, 122-123
🤖 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 `@frontend/packages/console-app/src/components/access-modes/__tests__/access-mode.spec.tsx` around lines 1 - 2, The tests in access-mode.spec.tsx currently import and call the raw render from '`@testing-library/react`'—replace usages of render with the shared renderWithProviders from 'frontend/packages/console-shared/src/test-utils' (import renderWithProviders alongside or instead of render) and update each test that calls render (occurrences around lines with render/userEvent interactions) to call renderWithProviders(...) so the component-level suite runs with the app providers; keep existing userEvent and screen usage intact and remove the unused raw render import.frontend/packages/console-app/src/components/volume-modes/__tests__/volume-mode.spec.tsx (1)
1-2: ⚡ Quick winStandardize this suite on
renderWithProviders.Using the shared render utility keeps test setup consistent and avoids missing provider context as this component evolves.
As per coding guidelines, "Use reusable helper functions such as
renderWithProvidersandrenderHookWithProvidersfromfrontend/packages/console-shared/src/test-utils."Also applies to: 33-34, 39-40, 48-49, 69-70, 76-77, 82-83, 90-91
🤖 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 `@frontend/packages/console-app/src/components/volume-modes/__tests__/volume-mode.spec.tsx` around lines 1 - 2, Tests are importing and using render from `@testing-library/react`; replace use of render with the shared renderWithProviders helper to ensure consistent provider context: import renderWithProviders from frontend/packages/console-shared/src/test-utils (or the named export) and call renderWithProviders(...) everywhere render(...) is used in this suite (and keep userEvent usage as-is), updating all instances noted (e.g., the render calls around userEvent interactions) so the component tests use renderWithProviders instead of render.frontend/packages/console-app/src/components/quick-starts/__tests__/QuickStartDrawer.spec.tsx (1)
5-12: 💤 Low valueMock returns array instead of JSX element.
The mock returns a raw array
[quickStarts.map(...).join(', '), children]. While React can render arrays, this is an unusual pattern that may not accurately represent the real component's structure. Consider wrapping in a fragment or div for more realistic mocking.♻️ Alternative mock structure
jest.mock('`@patternfly/quickstarts`', () => ({ QuickStartDrawer: jest.fn( ({ quickStarts, children }: { quickStarts: QuickStart[]; children?: React.ReactNode }) => ( - [ + <> {quickStarts.map((qs) => qs.spec.displayName).join(', ')} {children} - ], + </> ), ), }));🤖 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 `@frontend/packages/console-app/src/components/quick-starts/__tests__/QuickStartDrawer.spec.tsx` around lines 5 - 12, The QuickStartDrawer mock in QuickStartDrawer.spec.tsx currently returns a raw array which is atypical; update the jest.mock for '`@patternfly/quickstarts`' so QuickStartDrawer returns a JSX element (e.g., a React fragment or div) containing the joined display names and children instead of an array; locate the mocked QuickStartDrawer factory in the test file and replace the array return with a fragment/div wrapper to better mirror the real component's structure.frontend/packages/console-app/src/components/quick-starts/__tests__/QuickStartEmptyState.spec.tsx (1)
43-101: ⚡ Quick winConsider role-based queries for improved accessibility testing.
The tests currently use
getByTextandqueryByText. Per guidelines, prefer role-based queries (e.g.,getByRole('button'),getByRole('link')) to better match how screen readers and users interact with the UI. This is especially relevant for testing empty states with actionable links.For example, lines 84-85 and 100-101 could query links by role:
expect(screen.getByRole('link', { name: 'Configure quick starts' })).toBeVisible(); expect(screen.getByRole('link', { name: 'Learn more about quick starts' })).toBeVisible();As per coding guidelines: "Always prefer role-based queries (e.g.,
getByRole) over generic selectors for semantic testing" and "Use accessibility-first queries that match how screen readers and users interact with the UI."🤖 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 `@frontend/packages/console-app/src/components/quick-starts/__tests__/QuickStartEmptyState.spec.tsx` around lines 43 - 101, Tests in QuickStartEmptyState.spec.tsx rely on text queries (screen.getByText / queryByText) for actionable links; replace those with role-based queries to improve accessibility testing: update assertions in the QuickStartEmptyState tests that check the "Configure quick starts" and "Learn more about quick starts" elements (used after rendering <QuickStartEmptyState /> and controlled by mockUseAccessReview) to use screen.getByRole('link', { name: 'Configure quick starts' }) and screen.getByRole('link', { name: 'Learn more about quick starts' }) (and use screen.queryByRole('link', { name: 'Configure quick starts' }) when asserting absence) so the tests validate links by their semantic role instead of plain text.frontend/packages/console-app/src/components/quick-starts/loader/__tests__/QuickStartPermissionChecker.spec.tsx (1)
29-31: 💤 Low valueConsider adding
afterEachcleanup pattern.While
clearAllMocks()inbeforeEachis functionally similar, the coding guidelines recommend usingafterEach(() => { jest.restoreAllMocks(); })for consistency across the codebase. This ensures mocks are cleaned up after each test run.♻️ Add afterEach cleanup
beforeEach(() => { jest.clearAllMocks(); }); + + afterEach(() => { + jest.restoreAllMocks(); + });As per coding guidelines: "Clean up mocks with
afterEach(() => { jest.restoreAllMocks(); })"🤖 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 `@frontend/packages/console-app/src/components/quick-starts/loader/__tests__/QuickStartPermissionChecker.spec.tsx` around lines 29 - 31, The test suite currently calls jest.clearAllMocks() in the existing beforeEach; add a complementary afterEach that calls jest.restoreAllMocks() to ensure mocks are restored after each test run (i.e., add an afterEach invoking jest.restoreAllMocks() alongside the existing beforeEach with jest.clearAllMocks()).frontend/packages/console-app/src/components/dashboards-page/__tests__/OperatorStatus.spec.tsx (1)
70-81: 💤 Low valueThird test is redundant—already covered by first two tests.
The test "should render status popup wrapper" asserts that both 'dns' and 'Available' are visible. This combines the assertions from the first two tests ("should render operator status value" checks 'Available', "should render operator name as resource link" checks the name). Unless this test verifies additional wrapper structure or behavior not covered by the others, it can be removed.
🤖 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 `@frontend/packages/console-app/src/components/dashboards-page/__tests__/OperatorStatus.spec.tsx` around lines 70 - 81, Remove the redundant test block named "should render status popup wrapper" in OperatorStatus.spec.tsx (the test that renders ClusterOperatorStatusRow with createMockOperatorStatus('dns','Available') inside MemoryRouter and asserts 'dns' and 'Available' visibility); either delete this entire it(...) block or replace it with a focused assertion that verifies unique wrapper-specific behavior (e.g., a popup wrapper element) if you intended to test structure beyond the two existing tests ("should render operator status value" and "should render operator name as resource link").frontend/packages/console-app/src/components/dashboards-page/__tests__/ControlPlaneStatus.spec.tsx (2)
59-93: 💤 Low valueConsider consolidating repetitive text checks into a single test.
Tests on lines 59–93 all follow the same pattern: render the component once and assert a single text element is visible. These could be combined into one test that verifies all expected labels in a single render, reducing duplication and speeding up the suite.
Example consolidation
- it('should render components column header', () => { - render(<ControlPlanePopup {...defaultProps} />); - expect(screen.getByText('Components')).toBeVisible(); - }); - - it('should render response rate column header', () => { - render(<ControlPlanePopup {...defaultProps} />); - expect(screen.getByText('Response rate')).toBeVisible(); - }); - - it('should render API Servers status', () => { - render(<ControlPlanePopup {...defaultProps} />); - expect(screen.getByText('API Servers')).toBeVisible(); - }); - - it('should render Controller Managers status', () => { - render(<ControlPlanePopup {...defaultProps} />); - expect(screen.getByText('Controller Managers')).toBeVisible(); - }); - - it('should render Schedulers status', () => { - render(<ControlPlanePopup {...defaultProps} />); - expect(screen.getByText('Schedulers')).toBeVisible(); - }); - - it('should render API Request Success Rate status', () => { - render(<ControlPlanePopup {...defaultProps} />); - expect(screen.getByText('API Request Success Rate')).toBeVisible(); - }); + it('should render all control plane components and headers', () => { + render(<ControlPlanePopup {...defaultProps} />); + + expect(screen.getByText('Components')).toBeVisible(); + expect(screen.getByText('Response rate')).toBeVisible(); + expect(screen.getByText('API Servers')).toBeVisible(); + expect(screen.getByText('Controller Managers')).toBeVisible(); + expect(screen.getByText('Schedulers')).toBeVisible(); + expect(screen.getByText('API Request Success Rate')).toBeVisible(); + });🤖 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 `@frontend/packages/console-app/src/components/dashboards-page/__tests__/ControlPlaneStatus.spec.tsx` around lines 59 - 93, Tests repeatedly render <ControlPlanePopup {...defaultProps}> and assert single visible texts; consolidate by creating one test (e.g., "should render control plane labels") that calls render(<ControlPlanePopup {...defaultProps} />) once and then asserts all expected labels with screen.getByText('Components'), screen.getByText('Response rate'), screen.getByText('API Servers'), screen.getByText('Controller Managers'), screen.getByText('Schedulers'), and screen.getByText('API Request Success Rate') are visible; remove the six separate tests to avoid duplicate renders and speed up the suite.
36-47: 💤 Low valueMock implementation paths are never exercised.
The
beforeEachdefines three code paths forgetControlPlaneComponentHealth(error → NOT_AVAILABLE, no response → LOADING, success → OK), but none of the tests verify these states are reflected in the rendered UI. Consider adding tests that provideerrorornullresponses to verify error handling and loading states, or simplify the mock if those paths aren't relevant to this component.🤖 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 `@frontend/packages/console-app/src/components/dashboards-page/__tests__/ControlPlaneStatus.spec.tsx` around lines 36 - 47, The mock for getControlPlaneComponentHealth in beforeEach defines three branches (error → HealthState.NOT_AVAILABLE, no response → HealthState.LOADING, success → HealthState.OK) but tests never assert those UI states; either add tests that call the component with mockGetControlPlaneComponentHealth returning an error and null to assert the NOT_AVAILABLE and LOADING UI states, or simplify the mock to only the success branch if those states are irrelevant. Locate and update the mock implementation (mockGetControlPlaneComponentHealth) in ControlPlaneStatus.spec.tsx and add two new test cases that set the mock to return { state: HealthState.NOT_AVAILABLE, message: 'Not available' } and { state: HealthState.LOADING } respectively, then render the component and assert the expected UI text/indicators for error and loading.frontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginCSPStatusDetail.spec.tsx (1)
31-31: ⚡ Quick winPrefer accessibility-first assertions (
getByRole) over text-only lookups.These assertions are stable today, but switching to role+name queries will better match user behavior and align with project test conventions.
Suggested update
-jest.mock('../ConsoleOperatorConfig', () => ({ - ConsolePluginCSPStatus: ({ hasViolations }: { hasViolations: boolean }) => ( - <span>{hasViolations ? 'Has violations' : 'No violations'}</span> - ), -})); +jest.mock('../ConsoleOperatorConfig', () => ({ + ConsolePluginCSPStatus: ({ hasViolations }: { hasViolations: boolean }) => ( + <span role="status">{hasViolations ? 'Has violations' : 'No violations'}</span> + ), +})); ... - expect(screen.getByText('No violations')).toBeVisible(); + expect(screen.getByRole('status', { name: 'No violations' })).toBeVisible();As per coding guidelines, "Use accessibility-first queries that match how screen readers and users interact with the UI" and "Always prefer role-based queries (e.g.,
getByRole) over generic selectors for semantic testing".Also applies to: 39-39, 47-47, 55-55, 67-67
🤖 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 `@frontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginCSPStatusDetail.spec.tsx` at line 31, Replace text-only queries like expect(screen.getByText('No violations')).toBeVisible() with accessibility-first role+name queries: use screen.getByRole(<correctRole>, { name: /No violations/i }) (choose the actual semantic role used in the component, e.g., 'status', 'heading', or 'alert') and assert visibility; update the same pattern at the other occurrences mentioned (lines with 'No violations' checks in ConsolePluginCSPStatusDetail.spec.tsx) so all four getByText usages become getByRole calls with a case-insensitive name matcher.frontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginStatusDetail.spec.tsx (1)
31-36: ⚡ Quick winUse single cast
as PluginInfoEntryfor consistency with adjacent test files.The double cast
(({ ... } as unknown) as PluginInfoEntry)is unnecessary and inconsistent with the pattern used inConsolePluginEnabledStatusDetail.spec.tsx,ConsolePluginDescriptionDetail.spec.tsx, andConsolePluginVersionDetail.spec.tsx—all in the same directory. The mock object shape already matches whatPluginInfoEntryrequires (manifest,status,enabled,errorMessage), so a single cast suffices. This also preserves type-checking strength per the coding guidelines on TypeScript type safety.🤖 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 `@frontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginStatusDetail.spec.tsx` around lines 31 - 36, Replace the unnecessary double cast "({ ... } as unknown) as PluginInfoEntry" with a single cast "as PluginInfoEntry" for the mock object that includes manifest (name), status, enabled, and errorMessage in ConsolePluginStatusDetail.spec.tsx; update the expression where the mock is constructed so it reads "... as PluginInfoEntry" to match the pattern used in ConsolePluginEnabledStatusDetail.spec.tsx, ConsolePluginDescriptionDetail.spec.tsx, and ConsolePluginVersionDetail.spec.tsx.frontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsx (2)
34-37: ⚡ Quick winRestore mocks between tests, not just clear calls
Switch to
afterEach(() => { jest.restoreAllMocks(); })(or equivalent) so mock implementations don’t leak between cases.As per coding guidelines, "Clean up mocks with
afterEach(() => { jest.restoreAllMocks(); })".🤖 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 `@frontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsx` around lines 34 - 37, The test setup currently calls jest.clearAllMocks() in the beforeEach which can allow mock implementations to leak between tests; change the cleanup to restore mocks after each test by removing or keeping clearAllMocks in setup but adding an afterEach(() => { jest.restoreAllMocks(); }) (or replacing the beforeEach usage) so that the mocked useActivePerspective (and any other jest.Mock) implementations are restored between tests; update the NavHeader.spec.tsx test file to call jest.restoreAllMocks() in an afterEach block while continuing to mock useActivePerspective in the setup.
85-86: ⚡ Quick winUse option/listbox interaction instead of clicking a heading
This assertion path is tied to internal markup (
heading) instead of user-facing control semantics. Query and click theoptionin the openedlistboxto keep this test resilient and accessibility-first.As per coding guidelines, "Use accessibility-first queries that match how screen readers and users interact with the UI" and "Always prefer role-based queries (e.g.,
getByRole) over generic selectors for semantic testing".🤖 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 `@frontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsx` around lines 85 - 86, The test currently queries a heading (screen.getByRole('heading', { name: 'Developer' })) and clicks it; change the interaction to target the user-facing listbox option instead: open the listbox if needed, locate the listbox via getByRole('listbox') (or getByRole('combobox') depending on implementation) and then query the option by role (getByRole('option', { name: 'Developer' })) and perform user.click on that option; update the devOption reference and click call in NavHeader.spec.tsx accordingly so the test uses role-based option/listbox interactions rather than a heading.frontend/packages/console-app/src/components/nav/__tests__/Navigation.spec.tsx (1)
46-84: ⚡ Quick winAvoid asserting child-component internals and prop plumbing
toHaveBeenCalled()on mocked children and invokingonPerspectiveSelected()inside the mock test internal composition rather than user-observable behavior. Prefer assertions on rendered nav behavior (visible state, aria state, interaction outcomes) without forcing callback execution during render.As per coding guidelines, "Test what users see and interact with - DO NOT test internal component state, private methods, props passed to child components, CSS class names/styles, or component structure".
🤖 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 `@frontend/packages/console-app/src/components/nav/__tests__/Navigation.spec.tsx` around lines 46 - 84, Replace tests that assert mocked child internals (MockNavHeader, MockPerspectiveNav) and directly invoke props with user-facing assertions: stop calling MockNavHeader.mockImplementation or expecting MockNavHeader/MockPerspectiveNav toHaveBeenCalled; instead render Navigation via renderWithProviders and assert observable behavior such as presence of header text or controls, the nav's open/closed ARIA state (e.g., aria-expanded or role="navigation"), and simulate user interactions (click the perspective toggle/control) to verify mockOnPerspectiveSelected is called; update tests referencing Navigation, mockOnPerspectiveSelected, MockNavHeader, and MockPerspectiveNav accordingly.frontend/packages/console-app/src/components/nav/__tests__/PinnedResource.spec.tsx (1)
8-8: 💤 Low valueOptional: Clarify or remove this comment.
The comment states
useConfirmNavUnpinModalis imported "for the type check in assertions," but the hook is actually imported so it can be mocked (line 15) and its call arguments can be asserted (line 121). The comment doesn't accurately describe the import's purpose and may confuse future maintainers.📝 Suggested fix
-// Note: useConfirmNavUnpinModal is imported for the type check in assertions - jest.mock('`@console/shared/src/hooks/useK8sModel`', () => ({🤖 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 `@frontend/packages/console-app/src/components/nav/__tests__/PinnedResource.spec.tsx` at line 8, The existing comment incorrectly says useConfirmNavUnpinModal is imported "for the type check in assertions"; update or remove it so it accurately reflects the actual purpose: the hook is imported to be mocked and to assert its call arguments in tests. Edit the comment near the import of useConfirmNavUnpinModal (around the import block) to state "imported to be mocked and to assert call arguments" or simply delete the comment if redundant, ensuring the test still mocks useConfirmNavUnpinModal and asserts its call at the assertion site where it's used.frontend/packages/console-app/src/components/nav/__tests__/PluginNavItem.spec.tsx (1)
2-15: ⚡ Quick winImport from specific SDK paths to avoid build performance issues.
These imports pull from the
@console/dynamic-plugin-sdkpackage index, which can create circular dependencies and slow builds. Other test files in this PR (e.g.,useNavExtensionsForSection.spec.ts) demonstrate the preferred pattern: import from specific paths like@console/dynamic-plugin-sdk/src/lib-core. Notice line 16 already uses a specific path (/src/types), so adopting this pattern consistently will improve build performance and align with the codebase conventions. As per coding guidelines, never import from package index files in new code.♻️ Suggested refactor
import { screen } from '`@testing-library/react`'; import type { NavSection as NavSectionType, HrefNavItem, ResourceNavItem, Separator, -} from '`@console/dynamic-plugin-sdk`'; +} from '`@console/dynamic-plugin-sdk/src/lib-core`'; import { isNavSection, isSeparator, isHrefNavItem, isResourceNSNavItem, isResourceNavItem, useActivePerspective, -} from '`@console/dynamic-plugin-sdk`'; +} from '`@console/dynamic-plugin-sdk/src/lib-core`'; import type { LoadedExtension } from '`@console/dynamic-plugin-sdk/src/types`';You'll also need to update the mock on line 24:
-jest.mock('`@console/dynamic-plugin-sdk`', () => ({ - ...jest.requireActual('`@console/dynamic-plugin-sdk`'), +jest.mock('`@console/dynamic-plugin-sdk/src/lib-core`', () => ({ + ...jest.requireActual('`@console/dynamic-plugin-sdk/src/lib-core`'), isNavSection: jest.fn(),🤖 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 `@frontend/packages/console-app/src/components/nav/__tests__/PluginNavItem.spec.tsx` around lines 2 - 15, Replace package-index imports from '`@console/dynamic-plugin-sdk`' with specific internal SDK paths used elsewhere (e.g., import types from '`@console/dynamic-plugin-sdk/src/types`' and functions from '`@console/dynamic-plugin-sdk/src/lib-core`') so that symbols like NavSection (NavSectionType), HrefNavItem, ResourceNavItem, Separator and the runtime helpers isNavSection, isSeparator, isHrefNavItem, isResourceNSNavItem, isResourceNavItem, useActivePerspective are imported from their concrete module files rather than the package root; also update the corresponding test mock that stubs these exports (the mock defined for those imports) to reference the new module paths so Jest resolves the mocked functions/types correctly.frontend/packages/console-shared/src/components/cluster-configuration/FormLayout.tsx (1)
17-18: 💤 Low valueConsider whether
role="region"is semantically appropriate for a generic form layout wrapper.The
role="region"landmark indicates a significant, independently meaningful section of the page. SinceFormLayoutis a generic layout component that may wrap various forms throughout the application, applyingrole="region"here could:
- Create excessive landmark regions in the document
- Add screen reader navigation noise when forms are small or numerous
- Dilute the semantic value of regions for actual page sections
Unless forms rendered via
FormLayoutare always substantial, independently navigable sections, consider omitting the landmark role and relying on the form-specific semantics already present in the child content.🤖 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 `@frontend/packages/console-shared/src/components/cluster-configuration/FormLayout.tsx` around lines 17 - 18, The FormLayout component currently applies role="region" and aria-label={t('console-shared~Form layout')} unconditionally, which can introduce excessive landmarks; update FormLayout to stop forcing a region by default—either remove the role/aria-label or make them opt-in via a prop (e.g., add useRegion?: boolean and regionLabel?: string) and only render role="region" and aria-label when useRegion is true (preserve the t(...) usage for default label when provided); update any callers that relied on the implicit region to pass useRegion=true where a true landmark is required.frontend/packages/console-shared/src/components/error/fallbacks/ErrorBoundaryInline.tsx (1)
31-35: 💤 Low valueConsider whether
role="region"is semantically appropriate for inline error boundaries.The
role="region"landmark is intended for significant sections of a page that warrant navigation shortcuts. For small inline error messages that may appear frequently throughout the interface, this could create unnecessary screen reader navigation noise. Consider:
- Using
role="alert"orrole="status"if the error boundary content is meant to announce changes- Omitting the role entirely if the error message is already wrapped in semantic HTML
- Reserving
role="region"only if this boundary consistently wraps substantial, independently meaningful content🤖 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 `@frontend/packages/console-shared/src/components/error/fallbacks/ErrorBoundaryInline.tsx` around lines 31 - 35, The inline error wrapper in ErrorBoundaryInline.tsx currently uses role="region"; update the markup around the ErrorBoundary (the div that wraps <ErrorBoundary {...props} FallbackComponent={fallback}>{children}</ErrorBoundary>) to use a more appropriate ARIA semantics: either remove the role entirely or replace it with role="alert" (or role="status") depending on whether the error should be announced, and ensure any announcement needs are reflected (e.g., aria-live behavior) when choosing alert/status; keep the rest of the component (props, fallback, children) unchanged.
🤖 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
`@frontend/packages/console-app/src/components/cluster-configuration/__tests__/ClusterConfigurationCustomField.spec.tsx`:
- Around line 104-106: The test in ClusterConfigurationCustomField.spec.tsx is
using screen.getByRole('paragraph', { name: 'Extension with custom props' })
which is invalid for a bare <p> element; update the assertion to use an
accessible label query that matches the element's aria-label (e.g. use
screen.getByLabelText('Extension with custom props') to select the paragraph)
and then assert .toHaveTextContent('test-value') so the test uses a valid
accessible query for the element.
In
`@frontend/packages/console-app/src/components/cluster-configuration/ClusterConfigurationCustomField.tsx`:
- Line 23: The aria-label currently uses raw plugin-provided text (item.label);
make it i18n-aware by importing the translation hook (e.g., useTranslation) and
using t(item.label) with a safe fallback to item.label
(aria-label={t(item.label) || item.label}), then apply the same change to the
label props in ClusterConfigurationTextField and
ClusterConfigurationCheckboxField so all field types pass labels through the
translator; also ensure
useClusterConfigurationItems/getClusterConfigurationGroups continue to supply
the same string key contract (or update those to emit i18n keys) so translation
keys line up.
In
`@frontend/packages/console-app/src/components/dashboards-page/__tests__/ClusterUpdateActivity.spec.tsx`:
- Line 37: The test is asserting internal implementation by checking
MockActivityItem calls; remove the two assertions that reference
MockActivityItem (e.g., expect(MockActivityItem).toHaveBeenCalled()) so the spec
only verifies user-visible output like screen.getByText('Updating cluster to
4.15.0').toBeVisible(). Locate the assertions in ClusterUpdateActivity.spec.tsx
that reference MockActivityItem and delete them (both occurrences), leaving the
existing text/visibility assertions intact.
In
`@frontend/packages/console-app/src/components/nav/__tests__/NavSection.spec.tsx`:
- Around line 61-71: The test fixture createResourceExtension currently types
its return as LoadedExtension<ResourceNavItem> which is too broad for an
extension with type 'console.navigation/resource-ns'; change the return type to
LoadedExtension<ResourceNSNavItem> so the fixture matches the discriminated
union and preserves type-safe narrowing with helpers like
isResourceNSNavItem()/isResourceClusterNavItem(), updating the function
signature accordingly.
In
`@frontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/NodeUptime.spec.tsx`:
- Around line 3-8: Replace the barrel import and mock of getNodeUptime with the
concrete selector module: change the import of getNodeUptime to come from the
selector module and update the jest.mock target accordingly so the test imports
getNodeUptime from '`@console/shared/src/selectors/node`' (keep NodeUptime import
as-is) and mock that same concrete module using
jest.mock('`@console/shared/src/selectors/node`', () => ({ getNodeUptime:
jest.fn() })).
---
Nitpick comments:
In
`@frontend/packages/console-app/src/components/access-modes/__tests__/access-mode.spec.tsx`:
- Around line 1-2: The tests in access-mode.spec.tsx currently import and call
the raw render from '`@testing-library/react`'—replace usages of render with the
shared renderWithProviders from
'frontend/packages/console-shared/src/test-utils' (import renderWithProviders
alongside or instead of render) and update each test that calls render
(occurrences around lines with render/userEvent interactions) to call
renderWithProviders(...) so the component-level suite runs with the app
providers; keep existing userEvent and screen usage intact and remove the unused
raw render import.
In
`@frontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginCSPStatusDetail.spec.tsx`:
- Line 31: Replace text-only queries like expect(screen.getByText('No
violations')).toBeVisible() with accessibility-first role+name queries: use
screen.getByRole(<correctRole>, { name: /No violations/i }) (choose the actual
semantic role used in the component, e.g., 'status', 'heading', or 'alert') and
assert visibility; update the same pattern at the other occurrences mentioned
(lines with 'No violations' checks in ConsolePluginCSPStatusDetail.spec.tsx) so
all four getByText usages become getByRole calls with a case-insensitive name
matcher.
In
`@frontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginStatusDetail.spec.tsx`:
- Around line 31-36: Replace the unnecessary double cast "({ ... } as unknown)
as PluginInfoEntry" with a single cast "as PluginInfoEntry" for the mock object
that includes manifest (name), status, enabled, and errorMessage in
ConsolePluginStatusDetail.spec.tsx; update the expression where the mock is
constructed so it reads "... as PluginInfoEntry" to match the pattern used in
ConsolePluginEnabledStatusDetail.spec.tsx,
ConsolePluginDescriptionDetail.spec.tsx, and
ConsolePluginVersionDetail.spec.tsx.
In
`@frontend/packages/console-app/src/components/dashboards-page/__tests__/ControlPlaneStatus.spec.tsx`:
- Around line 59-93: Tests repeatedly render <ControlPlanePopup
{...defaultProps}> and assert single visible texts; consolidate by creating one
test (e.g., "should render control plane labels") that calls
render(<ControlPlanePopup {...defaultProps} />) once and then asserts all
expected labels with screen.getByText('Components'), screen.getByText('Response
rate'), screen.getByText('API Servers'), screen.getByText('Controller
Managers'), screen.getByText('Schedulers'), and screen.getByText('API Request
Success Rate') are visible; remove the six separate tests to avoid duplicate
renders and speed up the suite.
- Around line 36-47: The mock for getControlPlaneComponentHealth in beforeEach
defines three branches (error → HealthState.NOT_AVAILABLE, no response →
HealthState.LOADING, success → HealthState.OK) but tests never assert those UI
states; either add tests that call the component with
mockGetControlPlaneComponentHealth returning an error and null to assert the
NOT_AVAILABLE and LOADING UI states, or simplify the mock to only the success
branch if those states are irrelevant. Locate and update the mock implementation
(mockGetControlPlaneComponentHealth) in ControlPlaneStatus.spec.tsx and add two
new test cases that set the mock to return { state: HealthState.NOT_AVAILABLE,
message: 'Not available' } and { state: HealthState.LOADING } respectively, then
render the component and assert the expected UI text/indicators for error and
loading.
In
`@frontend/packages/console-app/src/components/dashboards-page/__tests__/OperatorStatus.spec.tsx`:
- Around line 70-81: Remove the redundant test block named "should render status
popup wrapper" in OperatorStatus.spec.tsx (the test that renders
ClusterOperatorStatusRow with createMockOperatorStatus('dns','Available') inside
MemoryRouter and asserts 'dns' and 'Available' visibility); either delete this
entire it(...) block or replace it with a focused assertion that verifies unique
wrapper-specific behavior (e.g., a popup wrapper element) if you intended to
test structure beyond the two existing tests ("should render operator status
value" and "should render operator name as resource link").
In
`@frontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsx`:
- Around line 34-37: The test setup currently calls jest.clearAllMocks() in the
beforeEach which can allow mock implementations to leak between tests; change
the cleanup to restore mocks after each test by removing or keeping
clearAllMocks in setup but adding an afterEach(() => { jest.restoreAllMocks();
}) (or replacing the beforeEach usage) so that the mocked useActivePerspective
(and any other jest.Mock) implementations are restored between tests; update the
NavHeader.spec.tsx test file to call jest.restoreAllMocks() in an afterEach
block while continuing to mock useActivePerspective in the setup.
- Around line 85-86: The test currently queries a heading
(screen.getByRole('heading', { name: 'Developer' })) and clicks it; change the
interaction to target the user-facing listbox option instead: open the listbox
if needed, locate the listbox via getByRole('listbox') (or getByRole('combobox')
depending on implementation) and then query the option by role
(getByRole('option', { name: 'Developer' })) and perform user.click on that
option; update the devOption reference and click call in NavHeader.spec.tsx
accordingly so the test uses role-based option/listbox interactions rather than
a heading.
In
`@frontend/packages/console-app/src/components/nav/__tests__/Navigation.spec.tsx`:
- Around line 46-84: Replace tests that assert mocked child internals
(MockNavHeader, MockPerspectiveNav) and directly invoke props with user-facing
assertions: stop calling MockNavHeader.mockImplementation or expecting
MockNavHeader/MockPerspectiveNav toHaveBeenCalled; instead render Navigation via
renderWithProviders and assert observable behavior such as presence of header
text or controls, the nav's open/closed ARIA state (e.g., aria-expanded or
role="navigation"), and simulate user interactions (click the perspective
toggle/control) to verify mockOnPerspectiveSelected is called; update tests
referencing Navigation, mockOnPerspectiveSelected, MockNavHeader, and
MockPerspectiveNav accordingly.
In
`@frontend/packages/console-app/src/components/nav/__tests__/PinnedResource.spec.tsx`:
- Line 8: The existing comment incorrectly says useConfirmNavUnpinModal is
imported "for the type check in assertions"; update or remove it so it
accurately reflects the actual purpose: the hook is imported to be mocked and to
assert its call arguments in tests. Edit the comment near the import of
useConfirmNavUnpinModal (around the import block) to state "imported to be
mocked and to assert call arguments" or simply delete the comment if redundant,
ensuring the test still mocks useConfirmNavUnpinModal and asserts its call at
the assertion site where it's used.
In
`@frontend/packages/console-app/src/components/nav/__tests__/PluginNavItem.spec.tsx`:
- Around line 2-15: Replace package-index imports from
'`@console/dynamic-plugin-sdk`' with specific internal SDK paths used elsewhere
(e.g., import types from '`@console/dynamic-plugin-sdk/src/types`' and functions
from '`@console/dynamic-plugin-sdk/src/lib-core`') so that symbols like NavSection
(NavSectionType), HrefNavItem, ResourceNavItem, Separator and the runtime
helpers isNavSection, isSeparator, isHrefNavItem, isResourceNSNavItem,
isResourceNavItem, useActivePerspective are imported from their concrete module
files rather than the package root; also update the corresponding test mock that
stubs these exports (the mock defined for those imports) to reference the new
module paths so Jest resolves the mocked functions/types correctly.
In
`@frontend/packages/console-app/src/components/quick-starts/__tests__/QuickStartDrawer.spec.tsx`:
- Around line 5-12: The QuickStartDrawer mock in QuickStartDrawer.spec.tsx
currently returns a raw array which is atypical; update the jest.mock for
'`@patternfly/quickstarts`' so QuickStartDrawer returns a JSX element (e.g., a
React fragment or div) containing the joined display names and children instead
of an array; locate the mocked QuickStartDrawer factory in the test file and
replace the array return with a fragment/div wrapper to better mirror the real
component's structure.
In
`@frontend/packages/console-app/src/components/quick-starts/__tests__/QuickStartEmptyState.spec.tsx`:
- Around line 43-101: Tests in QuickStartEmptyState.spec.tsx rely on text
queries (screen.getByText / queryByText) for actionable links; replace those
with role-based queries to improve accessibility testing: update assertions in
the QuickStartEmptyState tests that check the "Configure quick starts" and
"Learn more about quick starts" elements (used after rendering
<QuickStartEmptyState /> and controlled by mockUseAccessReview) to use
screen.getByRole('link', { name: 'Configure quick starts' }) and
screen.getByRole('link', { name: 'Learn more about quick starts' }) (and use
screen.queryByRole('link', { name: 'Configure quick starts' }) when asserting
absence) so the tests validate links by their semantic role instead of plain
text.
In
`@frontend/packages/console-app/src/components/quick-starts/loader/__tests__/QuickStartPermissionChecker.spec.tsx`:
- Around line 29-31: The test suite currently calls jest.clearAllMocks() in the
existing beforeEach; add a complementary afterEach that calls
jest.restoreAllMocks() to ensure mocks are restored after each test run (i.e.,
add an afterEach invoking jest.restoreAllMocks() alongside the existing
beforeEach with jest.clearAllMocks()).
In
`@frontend/packages/console-app/src/components/volume-modes/__tests__/volume-mode.spec.tsx`:
- Around line 1-2: Tests are importing and using render from
`@testing-library/react`; replace use of render with the shared
renderWithProviders helper to ensure consistent provider context: import
renderWithProviders from frontend/packages/console-shared/src/test-utils (or the
named export) and call renderWithProviders(...) everywhere render(...) is used
in this suite (and keep userEvent usage as-is), updating all instances noted
(e.g., the render calls around userEvent interactions) so the component tests
use renderWithProviders instead of render.
In
`@frontend/packages/console-shared/src/components/cluster-configuration/FormLayout.tsx`:
- Around line 17-18: The FormLayout component currently applies role="region"
and aria-label={t('console-shared~Form layout')} unconditionally, which can
introduce excessive landmarks; update FormLayout to stop forcing a region by
default—either remove the role/aria-label or make them opt-in via a prop (e.g.,
add useRegion?: boolean and regionLabel?: string) and only render role="region"
and aria-label when useRegion is true (preserve the t(...) usage for default
label when provided); update any callers that relied on the implicit region to
pass useRegion=true where a true landmark is required.
In
`@frontend/packages/console-shared/src/components/error/fallbacks/ErrorBoundaryInline.tsx`:
- Around line 31-35: The inline error wrapper in ErrorBoundaryInline.tsx
currently uses role="region"; update the markup around the ErrorBoundary (the
div that wraps <ErrorBoundary {...props}
FallbackComponent={fallback}>{children}</ErrorBoundary>) to use a more
appropriate ARIA semantics: either remove the role entirely or replace it with
role="alert" (or role="status") depending on whether the error should be
announced, and ensure any announcement needs are reflected (e.g., aria-live
behavior) when choosing alert/status; keep the rest of the component (props,
fallback, children) unchanged.
🪄 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: Repository YAML (base), Central YAML (inherited)
Review profile: CHILL
Plan: Enterprise
Run ID: 3ed9974e-f600-4920-bb77-042a164ae95c
📒 Files selected for processing (52)
frontend/packages/console-app/src/components/access-modes/__tests__/access-mode.spec.tsxfrontend/packages/console-app/src/components/access-modes/access-mode.tsxfrontend/packages/console-app/src/components/cluster-configuration/ClusterConfigurationCustomField.tsxfrontend/packages/console-app/src/components/cluster-configuration/ClusterConfigurationForm.tsxfrontend/packages/console-app/src/components/cluster-configuration/__tests__/ClusterConfigurationCustomField.spec.tsxfrontend/packages/console-app/src/components/cluster-configuration/__tests__/ClusterConfigurationField.spec.tsxfrontend/packages/console-app/src/components/cluster-configuration/__tests__/ClusterConfigurationForm.spec.tsxfrontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginCSPStatusDetail.spec.tsxfrontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginDescriptionDetail.spec.tsxfrontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginEnabledStatusDetail.spec.tsxfrontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginStatusDetail.spec.tsxfrontend/packages/console-app/src/components/console-operator/__tests__/ConsolePluginVersionDetail.spec.tsxfrontend/packages/console-app/src/components/dashboards-page/__tests__/ClusterUpdateActivity.spec.tsxfrontend/packages/console-app/src/components/dashboards-page/__tests__/ControlPlaneStatus.spec.tsxfrontend/packages/console-app/src/components/dashboards-page/__tests__/OperatorStatus.spec.tsxfrontend/packages/console-app/src/components/dashboards-page/dynamic-plugins-health-resource/__tests__/NotLoadedDynamicPlugins.spec.tsxfrontend/packages/console-app/src/components/favorite/FavoriteNavItem.tsxfrontend/packages/console-app/src/components/favorite/__tests__/FavoriteNavItem.spec.tsxfrontend/packages/console-app/src/components/nav/NavItemHref.tsxfrontend/packages/console-app/src/components/nav/__tests__/NavHeader.spec.tsxfrontend/packages/console-app/src/components/nav/__tests__/NavItemHref.spec.tsxfrontend/packages/console-app/src/components/nav/__tests__/NavLink.spec.tsxfrontend/packages/console-app/src/components/nav/__tests__/NavSection.spec.tsxfrontend/packages/console-app/src/components/nav/__tests__/Navigation.spec.tsxfrontend/packages/console-app/src/components/nav/__tests__/PinnedResource.spec.tsxfrontend/packages/console-app/src/components/nav/__tests__/PluginNavItem.spec.tsxfrontend/packages/console-app/src/components/nav/__tests__/useConfirmNavUnpinModal.spec.tsxfrontend/packages/console-app/src/components/nav/__tests__/useNavExtensionForPerspective.spec.tsfrontend/packages/console-app/src/components/nav/__tests__/useNavExtensionsForSection.spec.tsfrontend/packages/console-app/src/components/nav/__tests__/utils.spec.tsfrontend/packages/console-app/src/components/nav/index.tsxfrontend/packages/console-app/src/components/nodes/__tests__/NodeDetailsConditions.spec.tsxfrontend/packages/console-app/src/components/nodes/__tests__/NodeDetailsImages.spec.tsxfrontend/packages/console-app/src/components/nodes/__tests__/NodeIPList.spec.tsxfrontend/packages/console-app/src/components/nodes/__tests__/NodeRoles.spec.tsxfrontend/packages/console-app/src/components/nodes/modals/__tests__/GroupsEditorModal.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/BareMetalInventoryItems.spec.tsxfrontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/NodeUptime.spec.tsxfrontend/packages/console-app/src/components/nodes/status/__tests__/SchedulableStatus.spec.tsxfrontend/packages/console-app/src/components/pdb/__tests__/AvailabilityDisplay.spec.tsxfrontend/packages/console-app/src/components/pdb/__tests__/AvailabilityRequirement.spec.tsxfrontend/packages/console-app/src/components/pdb/__tests__/DisruptionsAllowed.spec.tsxfrontend/packages/console-app/src/components/quick-starts/__tests__/QuickStartDrawer.spec.tsxfrontend/packages/console-app/src/components/quick-starts/__tests__/QuickStartEmptyState.spec.tsxfrontend/packages/console-app/src/components/quick-starts/loader/__tests__/QuickStartPermissionChecker.spec.tsxfrontend/packages/console-app/src/components/resource-quota/__tests__/ClusterResourceQuotaCharts.spec.tsxfrontend/packages/console-app/src/components/resource-quota/__tests__/ResourceQuotaCharts.spec.tsxfrontend/packages/console-app/src/components/resource-quota/__tests__/utils.spec.tsfrontend/packages/console-app/src/components/volume-modes/__tests__/volume-mode.spec.tsxfrontend/packages/console-shared/locales/en/console-shared.jsonfrontend/packages/console-shared/src/components/cluster-configuration/FormLayout.tsxfrontend/packages/console-shared/src/components/error/fallbacks/ErrorBoundaryInline.tsx
Comment on lines
+104
to
+106
| expect( | ||
| screen.getByRole('paragraph', { name: 'Extension with custom props' }), | ||
| ).toHaveTextContent('test-value'); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the invalid role query exists and needs replacement
rg -n "getByRole\\((\"|')paragraph" frontend/packages/console-app/src/components/cluster-configuration/__tests__/ClusterConfigurationCustomField.spec.tsxRepository: openshift/console
Length of output: 143
🏁 Script executed:
#!/bin/bash
# Read the test file around line 105 to understand the context
head -120 frontend/packages/console-app/src/components/cluster-configuration/__tests__/ClusterConfigurationCustomField.spec.tsx | tail -40Repository: openshift/console
Length of output: 1310
🏁 Script executed:
#!/bin/bash
# Check the component implementation
find . -name "ClusterConfigurationCustomField.tsx" -o -name "ClusterConfigurationCustomField.ts" | head -5Repository: openshift/console
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check ARIA role documentation - verify if 'paragraph' is a valid role
# Let's also check if there are other uses of getByRole in the test file
rg -n "getByRole" frontend/packages/console-app/src/components/cluster-configuration/__tests__/ClusterConfigurationCustomField.spec.tsxRepository: openshift/console
Length of output: 582
🏁 Script executed (no clone):
#!/bin/bash
# Verify ARIA role definitions - paragraph should not be a valid role
curl -s "https://www.w3.org/TR/wai-aria-1.2/" 2>/dev/null | grep -i "paragraph" | head -5 || echo "Web search needed for definitive ARIA spec"
Length of output: 2749
🏁 Script executed:
#!/bin/bash
# Check if there are other uses of getByRole with paragraph in the codebase
rg "getByRole.*paragraph" . --type tsx --type ts 2>/dev/null | head -10Repository: openshift/console
Length of output: 43
🏁 Script executed (no clone):
#!/bin/bash
# Check testing library best practices for paragraph role
# The paragraph role requires explicit role assignment - <p> doesn't implicitly map to it
echo "Checking if <p> implicitly maps to paragraph role in browser APIs..."
cat > /tmp/test_aria.js << 'EOF'
// In modern browsers, the 'paragraph' role is valid in ARIA but <p> elements
// do not have an implicit ARIA role mapping. The role needs to be explicitly set.
// React Testing Library relies on implicit/explicit role mappings.
// A bare <p> element has NO implicit role and getByRole('paragraph') will fail.
console.log("Summary: <p> element has NO implicit ARIA role by default")
console.log("The 'paragraph' role exists in ARIA but must be explicitly assigned")
console.log("getByRole('paragraph') will fail for a bare <p> without role='paragraph'")
EOF
node /tmp/test_aria.js
Length of output: 299
Use a valid accessible query for this assertion.
Line 105 uses getByRole('paragraph', ...), but the rendered <p> element has no implicit ARIA role mapping—bare <p> elements require explicit role="paragraph" to be role-queryable. Since the element has aria-label="Extension with custom props", query by its accessible label instead.
Suggested fix
- expect(
- screen.getByRole('paragraph', { name: 'Extension with custom props' }),
- ).toHaveTextContent('test-value');
+ expect(screen.getByLabelText('Extension with custom props')).toHaveTextContent('test-value');🤖 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
`@frontend/packages/console-app/src/components/cluster-configuration/__tests__/ClusterConfigurationCustomField.spec.tsx`
around lines 104 - 106, The test in ClusterConfigurationCustomField.spec.tsx is
using screen.getByRole('paragraph', { name: 'Extension with custom props' })
which is invalid for a bare <p> element; update the assertion to use an
accessible label query that matches the element's aria-label (e.g. use
screen.getByLabelText('Extension with custom props') to select the paragraph)
and then assert .toHaveTextContent('test-value') so the test uses a valid
accessible query for the element.
Comment on lines
+61
to
+71
| const createResourceExtension = (id: string, name: string): LoadedExtension<ResourceNavItem> => | ||
| ({ | ||
| type: 'console.navigation/resource-ns', | ||
| uid: `uid-${id}`, | ||
| properties: { | ||
| id, | ||
| name, | ||
| model: { group: '', version: 'v1', kind: 'Pod' }, | ||
| section: 'workloads', | ||
| }, | ||
| } as LoadedExtension<ResourceNavItem>); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify extension type strings and corresponding TS types used in nav tests/components.
rg -n -C2 "console.navigation/resource|ResourceNavItem|ResourceNSNavItem|isResourceNavItem|isResourceNSNavItem" frontend/packages/console-app/src/components/navRepository: openshift/console
Length of output: 15671
🏁 Script executed:
#!/bin/bash
# Find and examine ResourceNavItem and ResourceNSNavItem type definitions
find . -name "*.ts" -o -name "*.tsx" | xargs grep -l "ResourceNavItem\|ResourceNSNavItem" | grep -E "(dynamic-plugin|`@console`)" | head -20Repository: openshift/console
Length of output: 217
🏁 Script executed:
#!/bin/bash
# Check if ResourceNavItem is a union/discriminated type
rg -A 20 "type ResourceNavItem|interface ResourceNavItem|export.*ResourceNavItem" --type ts --type tsx | head -100Repository: openshift/console
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Check the actual extension type constants and their corresponding type mappings
rg -n "console.navigation/resource" --type ts --type tsxRepository: openshift/console
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Read the navigation types file directly
cat -n ./frontend/packages/console-dynamic-plugin-sdk/src/extensions/navigation.ts | head -150Repository: openshift/console
Length of output: 5379
🏁 Script executed:
#!/bin/bash
# Search for resource-ns and ResourceNavItem mapping without type filter
rg "console.navigation/resource" -nRepository: openshift/console
Length of output: 6624
Use ResourceNSNavItem instead of ResourceNavItem for the fixture return type
The fixture creates an extension with type: 'console.navigation/resource-ns', which is a discriminated union narrowing to ResourceNSNavItem. Typing it as LoadedExtension<ResourceNavItem> weakens the type contract—when the code uses isResourceNSNavItem() or isResourceClusterNavItem() type guards, the broader union type defeats type-safe narrowing. Change the return type to LoadedExtension<ResourceNSNavItem> to correctly reflect what the fixture produces and maintain type precision for guard testing.
Example fix
const createResourceExtension = (id: string, name: string): LoadedExtension<ResourceNSNavItem> =>
({
type: 'console.navigation/resource-ns',
uid: `uid-${id}`,
properties: {
id,
name,
model: { group: '', version: 'v1', kind: 'Pod' },
section: 'workloads',
},
} as LoadedExtension<ResourceNSNavItem>);🤖 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
`@frontend/packages/console-app/src/components/nav/__tests__/NavSection.spec.tsx`
around lines 61 - 71, The test fixture createResourceExtension currently types
its return as LoadedExtension<ResourceNavItem> which is too broad for an
extension with type 'console.navigation/resource-ns'; change the return type to
LoadedExtension<ResourceNSNavItem> so the fixture matches the discriminated
union and preserves type-safe narrowing with helpers like
isResourceNSNavItem()/isResourceClusterNavItem(), updating the function
signature accordingly.
Comment on lines
+3
to
+8
| import { getNodeUptime } from '@console/shared/src'; | ||
| import NodeUptime from '../NodeUptime'; | ||
|
|
||
| jest.mock('@console/shared/src', () => ({ | ||
| getNodeUptime: jest.fn(), | ||
| })); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the concrete module that defines getNodeUptime
rg -nP --type=ts --type=tsx '\bgetNodeUptime\b' frontend/packages/console-shared/src frontend/packages/console-app/src/components/nodes/node-dashboardRepository: openshift/console
Length of output: 89
🏁 Script executed:
#!/bin/bash
# Search for getNodeUptime definition in console-shared
rg -n 'getNodeUptime' frontend/packages/console-shared/src --type tsRepository: openshift/console
Length of output: 445
🏁 Script executed:
#!/bin/bash
# Show the actual test file imports and mock
head -20 frontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/NodeUptime.spec.tsxRepository: openshift/console
Length of output: 693
Replace barrel import and mock with concrete module path to avoid circular dependencies.
Line 3 and lines 6–8 import and mock getNodeUptime through the @console/shared/src package index. Use the concrete module path instead:
import { getNodeUptime } from '`@console/shared/src/selectors/node`';
import NodeUptime from '../NodeUptime';
jest.mock('`@console/shared/src/selectors/node`', () => ({
getNodeUptime: jest.fn(),
}));
Per coding guidelines, new frontend code must import from specific file paths, not package indices, to prevent circular dependency cycles and improve build performance.
🤖 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
`@frontend/packages/console-app/src/components/nodes/node-dashboard/__tests__/NodeUptime.spec.tsx`
around lines 3 - 8, Replace the barrel import and mock of getNodeUptime with the
concrete selector module: change the import of getNodeUptime to come from the
selector module and update the jest.mock target accordingly so the test imports
getNodeUptime from '`@console/shared/src/selectors/node`' (keep NodeUptime import
as-is) and mock that same concrete module using
jest.mock('`@console/shared/src/selectors/node`', () => ({ getNodeUptime:
jest.fn() })).
d1a7fb5 to
f3ac55b
Compare
openshift-ci
Bot
removed
the
needs-rebase
cajieh
force-pushed
the
extend-rtl-coverage-console-app
branch
6 times, most recently
from
27105cc to
4ac9625
Compare
cajieh
force-pushed
the
extend-rtl-coverage-console-app
branch
from
4ac9625 to
7450c85
Compare
Contributor
Author
|
/test e2e-gcp-console |
2 similar comments
Contributor
Author
|
/test e2e-gcp-console |
Contributor
Author
|
/test e2e-gcp-console |
logonoff
reviewed
May 19, 2026
Member
logonoff
left a comment
logonoff
left a comment
There was a problem hiding this comment.
A few comments which are generally applicable to other parts of the diff
| import ClusterConfigurationCustomField from '../ClusterConfigurationCustomField'; | ||
| import type { ResolvedClusterConfigurationItem } from '../types'; | ||
|
|
||
| const renderWithRouter = (ui: ReactElement) => render(<MemoryRouter>{ui}</MemoryRouter>); |
Member
There was a problem hiding this comment.
Shouldn't this be using renderWithProviders or at least get extracted into a shared-level helper?
| default: jest.fn(({ item }: { item: ResolvedClusterConfigurationItem }) => item.label), | ||
| })); | ||
|
|
||
| const MockClusterConfigurationField = ClusterConfigurationField as jest.Mock; |
Member
There was a problem hiding this comment.
note for better type checking you may opt to use jest.MockedFunction but this is optional
|
|
||
| jest.mock('@console/shared/src/hooks/useConsoleSelector', () => ({ | ||
| useConsoleSelector: jest.fn(), | ||
| })); |
Member
There was a problem hiding this comment.
Note it's an antipattern to mock redux hooks and API (https://redux.js.org/usage/writing-tests) and this should be discouraged in new tests
Instead use renderWithProviders and supply the initial redux state
|
|
||
| jest.mock('@console/plugin-sdk/src/api/usePluginInfo', () => ({ | ||
| usePluginInfo: jest.fn(), | ||
| })); |
Member
There was a problem hiding this comment.
For the same reason as react-redux, please refactor to use renderWithProviders and create a test plugin store object with the required test data instead of mocking the hook
| @@ -0,0 +1,82 @@ | |||
| import { render, screen } from '@testing-library/react'; | |||
| import { MemoryRouter } from 'react-router'; | |||
Member
There was a problem hiding this comment.
We should probably just use renderWithProviders as it supplies the memory router
Comment on lines
+40
to
+43
| const multiplePerspectives = [ | ||
| createMockPerspective('admin', 'Administrator'), | ||
| createMockPerspective('dev', 'Developer'), | ||
| ]; |
Member
There was a problem hiding this comment.
In theory usePerspectives should already be populated with both perspectives, static plugin data is loaded in by default using renderWithProviders. See jest-transform-local-plugins.js for details. Thus this mock should be removed
cajieh
force-pushed
the
extend-rtl-coverage-console-app
branch
3 times, most recently
from
152b17a to
e24f73a
Compare
Contributor
Author
|
/test all |
1 similar comment
Contributor
Author
|
/test all |
Contributor
Author
|
/test e2e-gcp-console |
cajieh
force-pushed
the
extend-rtl-coverage-console-app
branch
3 times, most recently
from
451787b to
eb4f560
Compare
cajieh
force-pushed
the
extend-rtl-coverage-console-app
branch
from
eb4f560 to
ac968df
Compare
Contributor
Author
|
/label docs-approved |
openshift-ci
Bot
added
docs-approved
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
3 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.
CONSOLE-5253: Extend Unit Tests: Console-App - Core Navigation & Orchestration
This PR is the first installment [1/n] of a coordinated effort to extend unit test coverage within the console-app package. To ensure thorough code reviews, safe CI merges, and to avoid giant diffs, the testing infrastructure overhaul for Core Navigation & Orchestration is being broken down into a multi-PRs.
Analysis / Root cause:
Gap in unit tests
Solution description:
Extend unit test coverage.
Screenshots / screen recording:
N/A
Test setup:
Run
yarn testTest cases:
N/A
Browser conformance:
N/A
Additional info:
N/A
Reviewers and assignees:
Summary by CodeRabbit
Tests
Refactor
Chores