CONSOLE-4605: Migrate OLM core "packages/operator-lifecycle-manager" unit tests to React Testing Library #15704
Conversation
openshift-ci-robot
added
the
jira/valid-reference
|
@sg00dwin: This pull request references CONSOLE-4605 which is a valid jira issue. In 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. |
|
QA Approver: |
|
/test okd-scos-e2e-aws-ovn |
|
went through the changes and all tests are passing in CI |
openshift-ci-robot
added
the
verified
|
@yapei: This PR has been marked as verified by In 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. |
There was a problem hiding this comment.
@sg00dwin It looks good, except for a few comments I've added.
The test files should be moved to the tests subdirectory of the target directory. This will ensure consistency with other migrated tests and aligns with Console best practices.
Also, it appears that the following three tests are missing from the migration:
- packages/operator-lifecycle-manager/src/components/install-plan.spec.tsx
- packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.spec.tsx
- packages/operator-lifecycle-manager/src/components/operand/index.spec.tsx
We can either include them in this PR or scope them into a new, small story for 'missing tests.' What do you think?"
| jest.clearAllMocks(); | ||
|
|
||
| installPlanApprovalModalProps = { | ||
| obj: _.cloneDeep(testSubscription), |
There was a problem hiding this comment.
nit: Could be simplified to a shallow copy using object spread: obj = { ...testCatalogSource }. Although not new.
|
|
||
| expect(createYAML.find(ErrorBoundary).exists()).toBe(true); | ||
| expect(createYAML.find(ErrorBoundary).childAt(0).dive().find(CreateYAML).exists()).toBe(true); | ||
| expect(screen.getByText(new RegExp(testPackageManifest.metadata.name))).toBeInTheDocument(); |
There was a problem hiding this comment.
| expect(screen.getByText(new RegExp(testPackageManifest.metadata.name))).toBeInTheDocument(); | |
| expect(screen.getByText(new RegExp(testPackageManifest.metadata.name))).toBeVisible(); |
|
|
||
| expect(wrapper.find(DetailsPage).props().resources).toEqual([ | ||
| expect(detailsPageProps.pages).toHaveLength(3); |
There was a problem hiding this comment.
This tests implementation details (how DetailsPage is called), not user behavior. I would make it a follow-up in the story to extend RTL tests.
There was a problem hiding this comment.
Opened follow-on story
https://issues.redhat.com/browse/ODC-7820
| wrapper = shallow(<CatalogSourceDetailsPage />); | ||
| mockDetailsPage.mockClear(); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Missing afterEach(jest.restoreAllMocks()) after using jest.spyOn()
| afterEach(() => { | |
| jest.restoreAllMocks(); | |
| }); |
|
/label px-approved These are transparent test migrations with no px or docs impact. |
openshift-ci
bot
added
px-approved
sg00dwin
force-pushed
the
CONSOLE-4605-migration-to-RTL
branch
from
4c7cbda to
09e107d
Compare
openshift-ci-robot
removed
the
verified
sg00dwin
force-pushed
the
CONSOLE-4605-migration-to-RTL
branch
from
09e107d to
e40cc2e
Compare
WalkthroughAdded many new test suites and refactored tests in operator-lifecycle-manager; simultaneously removed several legacy spec files. Changes are test-only (additions, deletions, and RTL migration of OperatorGroup tests); no production code or public API signatures were modified. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (7)
frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/uninstall-operator-modal.spec.tsx (1)
66-113: Test structure for k8sKill calls is clear but could verify call ordering.Tests 2 and 3 both render the component, submit the form, wait for exactly 2 calls to
k8sKill, and then validate details of one call each. This is a reasonable pattern for testing independent aspects of behavior.However, if call ordering matters (subscription deleted before CSV), consider consolidating into a single test that validates the sequence, or explicitly verify call order using
toHaveNthReturnedWith/mock.callsinspection.frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/subscription-channel-modal.spec.tsx (1)
83-106: Async update & close behavior well covered; minor duplication onlyThe pattern of selecting the
nightlyradio, submitting the form, and usingwaitForaroundk8sUpdateandcloseexpectations properly exercises the async flow and payload shape. The expectations againstSubscriptionModelandspec.channel: 'nightly'are precise and valuable.There is a bit of repeated “select nightly + submit form” boilerplate across the two tests; if this pattern grows elsewhere, consider extracting a tiny helper (e.g.,
selectChannelAndSubmit('nightly')) to keep things DRY, but it’s not required for this PR.Also applies to: 108-122
frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/installplan-approval-modal.spec.tsx (1)
93-96: Consider a more robust form selection pattern.Using
.closest('form')combined with an optional check can mask test failures. If the form isn't found, the test silently skips the submission and may produce a false positive.Consider one of these alternatives:
- Add a
data-testidto the form element and query it directly:- const form = screen.getByRole('radio', { name: /manual/i }).closest('form'); - if (form) { - fireEvent.submit(form); - } + const form = screen.getByTestId('install-plan-approval-form'); + fireEvent.submit(form);
- Assert the form exists before submission:
const form = screen.getByRole('radio', { name: /manual/i }).closest('form'); - if (form) { - fireEvent.submit(form); - } + expect(form).toBeInTheDocument(); + fireEvent.submit(form!);Also applies to: 123-126, 145-148
frontend/packages/operator-lifecycle-manager/src/components/__tests__/subscription.spec.tsx (2)
156-353: Consider adding consistent cleanup to all describe blocks.Some describe blocks have
beforeEach/afterEachcleanup (lines 70-76, 261-263, 308-315), while others don't (SubscriptionStatus, SubscriptionsList, SubscriptionsPage). For test isolation and to prevent mock state pollution between tests, consider adding consistent cleanup:describe('SubscriptionStatus', () => { + beforeEach(() => { + jest.clearAllMocks(); + }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + it('renders "Upgrade available" when update is available', () => {Apply similar cleanup to
SubscriptionsListandSubscriptionsPagedescribe blocks.
205-205: Consider avoiding.WrappedComponentif possible.Accessing
.WrappedComponentexposes internal implementation details and makes the test fragile to changes in how the component is wrapped (e.g., HOC changes). If the component can be tested without accessing the wrapped version, that would be more maintainable. However, this is acceptable for test migrations where compatibility with existing patterns is needed.frontend/packages/operator-lifecycle-manager/src/components/__tests__/package-manifest.spec.tsx (1)
55-158: Tests are comprehensive and working correctly.The test suite provides good coverage of the PackageManifestTableRow component with proper mock lifecycle management. All assertions are meaningful and the tests are passing in CI.
Optional: Consider reducing repetition.
The table wrapper structure is repeated across all four tests (lines 68-80, 96-108, 117-129, 140-152). Extracting this to a helper function could improve maintainability:
const renderPackageManifestRow = (obj, customData, columns = []) => { return renderWithProviders( <table> <tbody> <tr> <PackageManifestTableRow obj={obj} customData={customData} columns={columns} /> </tr> </tbody> </table>, ); };Optional: Clarify test description.
The test at line 136 could have a clearer description. It's testing fallback behavior when
catalogSourceis not provided incustomData, not when the catalog source doesn't exist. Consider:"renders column for catalog source using package manifest name when catalogSource is not in customData".frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx (1)
1-73: Well-structured mock setup for RTL migration.The mocking strategy is comprehensive and appropriate for testing these operand components. The typed mock reference for
DetailsPage(line 72) is particularly good practice for inspecting mock calls later.Consider adding an
afterEachblock to restore mocks for consistency with the create-operand.spec.tsx file:+afterEach(() => { + jest.restoreAllMocks(); +});
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (21)
frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/install-plan.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/operator-group.spec.tsx(8 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/package-manifest.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/subscription.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/catalog-source.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/install-plan.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/installplan-approval-modal.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/subscription-channel-modal.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/uninstall-operator-modal.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/modals/subscription-channel-modal.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/create-operand.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/operand/create-operand.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/operand/index.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/package-manifest.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/subscription.spec.tsx(0 hunks)
💤 Files with no reviewable changes (10)
- frontend/packages/operator-lifecycle-manager/src/components/subscription.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/modals/uninstall-operator-modal.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/catalog-source.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/install-plan.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/operand/create-operand.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/modals/installplan-approval-modal.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/package-manifest.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/modals/subscription-channel-modal.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/operand/index.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.spec.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/installplan-approval-modal.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/subscription-channel-modal.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/uninstall-operator-modal.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/subscription.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/install-plan.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/package-manifest.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/operator-group.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/create-operand.spec.tsx
🔇 Additional comments (30)
frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/uninstall-operator-modal.spec.tsx (2)
1-34: Well-structured RTL test suite with sound mock strategy.The test migration follows React Testing Library best practices effectively: proper use of
renderWithProviders, semantic queries (getByRole,getByText), and correct async handling withwaitFor. Mock configuration is isolated inbeforeEachand resets between tests.One minor maintainability note: the
k8sGetResourcemock (line 32–34) appears unused in the test suite. Consider removing it if the component doesn't invoke this utility, or verify it's being called indirectly through the component under test.
128-137: Modal close callback test is clean; verify no edge-case handling needed.The test correctly verifies that the close callback is invoked exactly once after form submission. Ensure the component has no error-handling flow where close should not be called (e.g., validation errors or k8sKill failures). If error paths exist, consider a companion test for those scenarios.
frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/subscription-channel-modal.spec.tsx (4)
20-65: Solid, isolated test setupCloning the subscription and package fixtures plus resetting mocks in
beforeEachgives each test a clean, independent state and avoids cross‑test mutation. This setup looks good and maintainable.
67-72: Good basic render smoke testChecking the modal title and primary Save button via accessible queries is a good, low‑cost smoke test that will catch obvious regressions in the modal wiring.
74-81: Accessible channel options coverageVerifying that a radio exists per channel and asserting by accessible name (
stable,nightly) exercises both data‑driven rendering and a11y labels, which is exactly what you want here.
124-141: Submit button state tests are effectiveThe tests that ensure Save is initially disabled and becomes enabled only after changing the channel give good confidence that users can’t submit a no‑op change and that the UI reacts correctly to selection changes.
frontend/packages/operator-lifecycle-manager/src/components/modals/__tests__/installplan-approval-modal.spec.tsx (1)
1-154: Well-structured RTL migration with comprehensive coverage.The test suite successfully migrates from Enzyme to React Testing Library with good practices:
- Proper use of
renderWithProvidersand RTL queries- Comprehensive coverage of modal rendering, radio selection, and form submission
- Correct mocking strategy for dependencies
- Tests both Subscription and InstallPlan update scenarios
- Proper cleanup with beforeEach/afterEach
frontend/packages/operator-lifecycle-manager/src/components/__tests__/subscription.spec.tsx (1)
1-353: Test migration looks good overall.The migration from Enzyme to React Testing Library is well-executed. The tests appropriately verify component behavior by checking both mock interactions and rendered output. The comprehensive mocking strategy ensures tests remain fast and focused on the subscription components.
frontend/packages/operator-lifecycle-manager/src/components/__tests__/package-manifest.spec.tsx (2)
1-29: LGTM! Clean mock setup and imports.The mocking strategy is well-structured, with proper isolation of component dependencies. The ResourceLink mock correctly preserves the actual implementation while allowing override, and typed mock references improve type safety.
31-53: LGTM! Header tests are clear and focused.The tests effectively verify the table header configuration with appropriate assertions.
frontend/packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsx (3)
172-205: Well-structured logo component tests.These tests effectively verify the image rendering logic, fallback behavior, and text content display with strong, specific assertions.
237-316: Well-structured subscription tests with comprehensive scenario coverage.The tests effectively cover the empty state, subscription details rendering, and complex PackageManifest matching logic with strong assertions.
207-209: Good practice: documenting test removal rationale.The comments explaining why certain tests were removed (complexity, AsyncComponent mocking requirements, integration test coverage) demonstrate thoughtful decision-making about appropriate test boundaries.
Also applies to: 228-230, 232-236, 318-321
frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx (4)
74-103: LGTM! Clean RTL assertions for table row rendering.The tests properly verify that the OperandTableRow component renders the expected operand metadata, and the table wrapper structure is appropriate for testing a table row component.
105-195: Excellent coverage of OperandStatus display logic.The test suite thoroughly covers the status field priority hierarchy (status → phase → state → condition → dash) and verifies both labels and values. The use of
getByTestIdfor status text assertions is appropriate.
197-231: LGTM! Effective verification of DetailsPage configuration.The tests properly mock routing dependencies and verify the DetailsPage receives the correct tab configuration. The pattern of inspecting mock calls to verify component props is appropriate for this scenario.
233-282: LGTM! Clean tests for CRD-based UI rendering.The tests appropriately verify that the UI adapts based on the number of CRDs in the CSV. Good use of
cloneDeep(line 248) to avoid mutating the shared test fixture.frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/create-operand.spec.tsx (3)
1-43: LGTM! Comprehensive mock setup for operand creation tests.The mock configuration properly isolates the components under test. The type assertions on lines 41-42 using
as unknown as jest.Mockare acceptable for test mocks where the actual implementation types don't match the mock interface.
44-150: Excellent test coverage of CreateOperand configuration.The test suite thoroughly verifies that SyncedEditor receives the correct configuration based on different inputs. The test for
alm-examplesannotation parsing (lines 90-131) is particularly comprehensive, ensuring sample data is correctly extracted and passed to the editor.
152-216: LGTM! Thorough verification of OperandYAML prop handling.The test suite comprehensively covers the OperandYAML component's behavior, including conditional prop passing (resourceObjPath based on the
nextprop) and default values. The tests effectively verify that the component correctly forwards props to CreateYAML.frontend/packages/operator-lifecycle-manager/src/components/__tests__/operator-group.spec.tsx (2)
19-50: Excellent RTL migration!The HOC tests properly verify component behavior using RTL queries and assertions. Good use of both positive (
getByText,toBeVisible) and negative (queryByText,not.toBeInTheDocument) checks.
52-264: LGTM!The utility function tests remain focused on logic verification with appropriate test coverage for edge cases and various operator group configurations.
frontend/packages/operator-lifecycle-manager/src/components/__tests__/install-plan.spec.tsx (5)
42-136: Well-structured table row tests.Good coverage of rendering scenarios with proper table markup wrapping and RTL queries to verify visible content.
138-220: Tests verify component configuration.These tests confirm the correct setup of list and page components. While they test implementation details, this approach is reasonable for migration work.
265-342: Solid approval flow testing.Good coverage of the approval workflow including empty states, button visibility, and API interaction verification with proper async handling.
354-403: Appropriate conditional rendering tests.Tests correctly verify that the preview link appears only when manual approval is required.
405-427: Page configuration verified.The test confirms the details page is set up with the expected navigation tabs.
frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx (3)
92-107: Clean rendering test.The test verifies that key catalog source information is visible to users.
109-157: Details page tests with proper cleanup.The
afterEachblock correctly restores mocks. The TODO comment at line 126 appropriately acknowledges the implementation-detail testing approach for potential future refactoring.
159-219: Excellent YAML creation tests.Good coverage of both loading and loaded states, with verification of YAML content including package name, channel, source, and CSV information.
.../packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsx
Show resolved
Hide resolved
| it('renders managed namespace', () => { | ||
| renderWithProviders(<ClusterServiceVersionTableRow {...clusterServiceVersionTableRowProps} />); | ||
|
|
||
| // The component should render without errors | ||
| expect(screen.getByText(testClusterServiceVersion.spec.displayName)).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Strengthen test assertion to actually verify namespace rendering.
The test claims to verify namespace rendering but only asserts that the display name is visible. This doesn't confirm the namespace is actually rendered.
Consider asserting the presence of the namespace text:
it('renders managed namespace', () => {
renderWithProviders(<ClusterServiceVersionTableRow {...clusterServiceVersionTableRowProps} />);
- // The component should render without errors
- expect(screen.getByText(testClusterServiceVersion.spec.displayName)).toBeVisible();
+ expect(screen.getByText(testClusterServiceVersion.metadata.namespace)).toBeVisible();
});📝 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.
| it('renders managed namespace', () => { | |
| renderWithProviders(<ClusterServiceVersionTableRow {...clusterServiceVersionTableRowProps} />); | |
| // The component should render without errors | |
| expect(screen.getByText(testClusterServiceVersion.spec.displayName)).toBeVisible(); | |
| }); | |
| it('renders managed namespace', () => { | |
| renderWithProviders(<ClusterServiceVersionTableRow {...clusterServiceVersionTableRowProps} />); | |
| expect(screen.getByText(testClusterServiceVersion.metadata.namespace)).toBeVisible(); | |
| }); |
🤖 Prompt for AI Agents
In
frontend/packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsx
around lines 129 to 134, the test labeled "renders managed namespace" only
asserts the displayName is visible and doesn't verify the namespace; update the
test to assert that the expected namespace string from the test data is rendered
(e.g., use screen.getByText or getByRole with the namespace value) so the test
fails if the namespace is missing, ensuring the component actually renders the
namespace.
.../packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsx
Show resolved
Hide resolved
frontend/packages/operator-lifecycle-manager/src/components/__tests__/install-plan.spec.tsx
Show resolved
Hide resolved
@cajieh I have added these 3 test files to the commit. And addressed your latest feedback changes. |
@sg00dwin Added a few more comments. |
| }); | ||
|
|
||
| // ClusterServiceVersionList tests removed - complex component requiring extensive mocking | ||
| // Original Enzyme tests verified table header configuration which are implementation details |
There was a problem hiding this comment.
Let's remove the comment since it is capture in the Extend RTL test story ticket.
| // ClusterServiceVersionDetails tests removed - hybrid component uses AsyncComponent | ||
| // for MarkdownView and multiple complex child components requiring extensive mocking. | ||
| // Original Enzyme tests heavily tested implementation details (finding specific DOM nodes, | ||
| // checking props passed to children). User-facing behavior is tested via integration tests. |
| // ClusterServiceVersionDetailsPage tests removed - container component with complex | ||
| // hook dependencies (useClusterServiceVersion, useK8sWatchResource, useAccessReview). | ||
| // Original Enzyme tests verified DetailsPage props configuration which are implementation | ||
| // details. Page-level behavior is tested via Cypress integration tests. |
| const radios = screen.getAllByRole('radio'); | ||
| expect(radios).toHaveLength(2); | ||
| expect(screen.getByRole('radio', { name: /automatic \(default\)/i })).toBeVisible(); | ||
| expect(screen.getByRole('radio', { name: /manual/i })).toBeVisible(); |
There was a problem hiding this comment.
Better to use exact text matches, which makes the tests more precise and performant
| expect(screen.getByRole('radio', { name: /manual/i })).toBeVisible(); | |
| expect(screen.getByRole('radio', { name: 'Automatic (default)' })).toBeVisible(); | |
| expect(screen.getByRole('radio', { name: 'Manual' })).toBeVisible(); |
| const automaticRadio = screen.getByRole('radio', { name: /automatic \(default\)/i }); | ||
| const manualRadio = screen.getByRole('radio', { name: /manual/i }); | ||
|
|
||
| expect(automaticRadio).toBeChecked(); | ||
| expect(manualRadio).not.toBeChecked(); |
There was a problem hiding this comment.
| const automaticRadio = screen.getByRole('radio', { name: /automatic \(default\)/i }); | |
| const manualRadio = screen.getByRole('radio', { name: /manual/i }); | |
| expect(automaticRadio).toBeChecked(); | |
| expect(manualRadio).not.toBeChecked(); | |
| expect(screen.getByRole('radio', { name: 'Automatic (default)' })).toBeChecked(); | |
| expect(screen.getByRole('radio', { name: 'Manual' })).toBeChecked(); |
| const form = screen.getByRole('form'); | ||
| fireEvent.submit(form); |
There was a problem hiding this comment.
| const form = screen.getByRole('form'); | |
| fireEvent.submit(form); | |
| fireEvent.submit(screen.getByRole('form')); |
| const form = screen.getByRole('form'); | ||
| fireEvent.submit(form); |
There was a problem hiding this comment.
| const form = screen.getByRole('form'); | |
| fireEvent.submit(form); | |
| fireEvent.submit(screen.getByRole('form')); |
| const form = screen.getByRole('form'); | ||
| fireEvent.submit(form); |
There was a problem hiding this comment.
| const form = screen.getByRole('form'); | |
| fireEvent.submit(form); | |
| fireEvent.submit(screen.getByRole('form')); |
| it('renders create button with correct text for single CRD', () => { | ||
| renderWithProviders(<ProvidedAPIPage kind="TestResource" csv={testClusterServiceVersion} />); | ||
|
|
||
| expect(screen.getByText('Create Test Resource')).toBeInTheDocument(); |
There was a problem hiding this comment.
| expect(screen.getByText('Create Test Resource')).toBeInTheDocument(); | |
| expect(screen.getByText('Create Test Resource')).toBeVisible() |
| expect(screen.getByText('Uninstall Operator?')).toBeInTheDocument(); | ||
| expect(screen.getByRole('button', { name: 'Uninstall' })).toBeInTheDocument(); |
There was a problem hiding this comment.
Prefer .toBeVisible() for any element a user is expected to see or interact with, and the .toBeInTheDocument() to verify structural elements or to check that a component has rendered or been removed conditionally.
| expect(screen.getByText('Uninstall Operator?')).toBeInTheDocument(); | |
| expect(screen.getByRole('button', { name: 'Uninstall' })).toBeInTheDocument(); | |
| expect(screen.getByText('Uninstall Operator?')).toBeVisible() | |
| expect(screen.getByRole('button', { name: 'Uninstall' })).toBeVisible() |
The change is required across several files.
|
|
||
| renderWithProviders(<InstallPlanPreview obj={manualPlan} />); | ||
|
|
||
| expect(screen.getByRole('button', { name: /approve/i })).toBeVisible(); |
There was a problem hiding this comment.
| expect(screen.getByRole('button', { name: /approve/i })).toBeVisible(); | |
| expect(screen.getByRole('button', { name: 'Approve' })).toBeVisible(); |
|
|
||
| renderWithProviders(<InstallPlanPreview obj={manualPlan} />); | ||
|
|
||
| expect(screen.getByRole('button', { name: /deny/i })).toBeVisible(); |
There was a problem hiding this comment.
| expect(screen.getByRole('button', { name: /deny/i })).toBeVisible(); | |
| expect(screen.getByRole('button', { name: 'Deny' })).toBeVisible(); |
| renderWithProviders(<InstallPlanPreview obj={installPlan} />); | ||
|
|
||
| // Check that preview functionality is available for components with status 'Unknown' | ||
| const previewButtons = screen.queryAllByRole('button', { name: /preview/i }); |
There was a problem hiding this comment.
| const previewButtons = screen.queryAllByRole('button', { name: /preview/i }); | |
| const previewButtons = screen.queryAllByRole('button', { name: 'Preview' }); |
|
|
||
| renderWithProviders(<InstallPlanDetails obj={automaticPlan} />); | ||
|
|
||
| expect(screen.queryByRole('button', { name: /preview installplan/i })).not.toBeInTheDocument(); |
There was a problem hiding this comment.
| expect(screen.queryByRole('button', { name: /preview installplan/i })).not.toBeInTheDocument(); | |
| expect(screen.queryByRole('button', { name: 'Preview Installplan' })).not.toBeInTheDocument(); |
sg00dwin
force-pushed
the
CONSOLE-4605-migration-to-RTL
branch
from
e40cc2e to
1987dc6
Compare
openshift-merge-robot
added
the
needs-rebase
Convert 8 test files from Enzyme to RTL with improved accessibility testing and behavioral assertions. Add new tests to replace previous disabled tests Assisted-by Claude
sg00dwin
force-pushed
the
CONSOLE-4605-migration-to-RTL
branch
from
1987dc6 to
e19a479
Compare
openshift-merge-robot
removed
the
needs-rebase
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx (1)
126-126: Remove TODO comment as requested in previous review.Per cajieh's previous feedback, this TODO comment should be removed since the work is tracked in the follow-up story (ODC-7820).
Apply this diff:
- // TODO: Refactor to test user behavior instead of implementation details
🧹 Nitpick comments (3)
frontend/packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsx (2)
101-224: Consider extracting the repetitive table wrapper into a render helper.All eight tests in this suite repeat the same table structure wrapper. Extracting this to a helper function would improve maintainability and reduce duplication.
Consider adding a helper function before the tests:
const renderTableRow = (props: ClusterServiceVersionTableRowProps) => { return renderWithProviders( <table> <tbody> <tr> <ClusterServiceVersionTableRow {...props} /> </tr> </tbody> </table>, ); };Then simplify each test:
it('renders component wrapped in ErrorBoundary', () => { - renderWithProviders( - <table> - <tbody> - <tr> - <ClusterServiceVersionTableRow {...clusterServiceVersionTableRowProps} /> - </tr> - </tbody> - </table>, - ); + renderTableRow(clusterServiceVersionTableRowProps); expect(screen.getByText(testClusterServiceVersion.spec.displayName)).toBeVisible(); });
274-278: Consider adding a test for the positive case (canCreate=true).The test suite only verifies that the create link is absent when
canCreateis false, but doesn't verify it appears whencanCreateis true. Adding a positive test case would improve coverage.Example test to add:
it('renders create link when canCreate is true', () => { renderWithProviders(<CRDCard {...crdCardProps} canCreate={true} />); expect(screen.getByRole('link', { name: /Create/i })).toBeInTheDocument(); });frontend/packages/operator-lifecycle-manager/src/components/__tests__/install-plan.spec.tsx (1)
67-72: Consider using more direct query patterns.The current approach of using
getAllByTextfollowed by array filtering works but is indirect. Consider usingwithin()scoping or more specific role-based queries for better test clarity.Example refactor:
- const installPlanLinks = screen.getAllByText(installPlan.metadata.name); - const installPlanLink = installPlanLinks.find((link) => - link.getAttribute('href')?.includes('InstallPlan'), - ); - expect(installPlanLink).toBeVisible(); + const installPlanLink = screen.getByRole('link', { name: installPlan.metadata.name }); + expect(installPlanLink).toHaveAttribute('href', expect.stringContaining('InstallPlan'));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (8)
frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/install-plan.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/operator-group.spec.tsx(8 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/package-manifest.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/__tests__/subscription.spec.tsx(1 hunks)frontend/packages/operator-lifecycle-manager/src/components/catalog-source.spec.tsx(0 hunks)frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.spec.tsx(0 hunks)
💤 Files with no reviewable changes (2)
- frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/catalog-source.spec.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- frontend/packages/operator-lifecycle-manager/src/components/tests/operator-group.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/tests/subscription.spec.tsx
- frontend/packages/operator-lifecycle-manager/src/components/tests/package-manifest.spec.tsx
🧰 Additional context used
📓 Path-based instructions (1)
**
⚙️ CodeRabbit configuration file
-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity.
Files:
frontend/packages/operator-lifecycle-manager/src/components/__tests__/clusterserviceversion.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/install-plan.spec.tsx
🔇 Additional comments (10)
frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx (3)
1-90: LGTM on imports and mock setup.The imports and extensive mock configuration are appropriate for RTL-based testing. The mocks properly isolate the components under test.
92-107: Good RTL pattern testing user-visible behavior.This test suite properly focuses on what the user sees (display name and publisher) rather than implementation details.
159-219: LGTM on CreateSubscriptionYAML tests.These tests appropriately verify user-visible behavior including loading states and YAML content display. The mock override pattern is functional for this migration.
frontend/packages/operator-lifecycle-manager/src/components/__tests__/install-plan.spec.tsx (7)
1-42: LGTM: Test setup is well-structured.The imports and mock configuration are appropriate for RTL-based testing with proper mocking of external dependencies.
139-176: LGTM: Table configuration tests are appropriate.Using mock inspection to verify Table props is a reasonable approach for this component structure.
178-221: LGTM: Page configuration tests are comprehensive.The tests appropriately verify MultiListPage setup and resource fetching configuration.
289-318: LGTM: Integration test for approval flow is well-structured.The test properly verifies the k8sPatch call with correct parameters when the Approve button is clicked.
353-402: LGTM: Conditional rendering tests are well-covered.The tests appropriately verify that the preview link appears only for manual approval plans and use proper negative assertions.
404-426: LGTM: Navigation structure test is appropriate.The test verifies all three expected navigation tabs are configured correctly.
345-350: The current test implementation is correct and should not be changed.The button label in the
InstallPlanPreviewcomponent is{step.resource.name}, not "Preview". The test correctly verifies the button's visibility by matching against the actual resource name. The original review comment's suggestion to use{ name: 'Preview' }contradicts the actual UI implementation and would cause the test to fail.Likely an incorrect or invalid review comment.
|
/lgtm |
openshift-ci
bot
added
lgtm
|
/verified later @yapei |
openshift-ci-robot
added
verified-later
verified
|
@cajieh: This PR has been marked to be verified later by In 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. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, sg00dwin The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@sg00dwin: all tests passed! Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here. |
6 participants
No description provided.