CONSOLE-4809: Extend RTL Test Coverage for Components Migrated from Enzyme#16249
CONSOLE-4809: Extend RTL Test Coverage for Components Migrated from Enzyme#16249cajieh wants to merge 1 commit intoopenshift:mainfrom
Conversation
|
@cajieh: This pull request references CONSOLE-4809 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 "4.22.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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
bot
added
the
do-not-merge/work-in-progress
openshift-ci
bot
added
component/core
cajieh
force-pushed
the
extend-rtl-ests-overage-omponents-igrated-enzyme
branch
4 times, most recently
from
e926790 to
e84ae1c
Compare
| className={css('co-label tag-item-content', this.props.labelClassName)} | ||
| key={key} | ||
| onClose={() => onRemove(key)} | ||
| isTruncated |
There was a problem hiding this comment.
Appears to be redundant props, which is causing console warnings. If the intent was to truncate the label, let's address that in a follow-up bug using textMaxWidth instead.
cajieh
force-pushed
the
extend-rtl-ests-overage-omponents-igrated-enzyme
branch
3 times, most recently
from
1db0f6b to
f1beac9
Compare
cajieh
changed the title
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
cajieh
force-pushed
the
extend-rtl-ests-overage-omponents-igrated-enzyme
branch
from
f1beac9 to
86b8c52
Compare
openshift-ci
bot
added
component/dev-console
|
@cajieh: This pull request references CONSOLE-4809 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 "4.22.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. |
📝 WalkthroughWalkthroughThis pull request modernizes frontend test suites across multiple shared components and dev-console packages by adopting Testing Library best practices ( ✨ 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: 1
🧹 Nitpick comments (7)
frontend/packages/console-shared/src/components/loading/__tests__/loading.spec.tsx (1)
69-73: Test name could be slightly clearer on default behavior.The test validates that
blameprop content isn't rendered by default (i.e., without an explicit query param enabling it). The logic is correct, but the test name "does not render blame info when query param disabled" might be misread as actively disabling something. Consider renaming to'does not render blame info by default'for clarity.💡 Optional: Clearer test name
- it('does not render blame info when query param disabled', () => { + it('does not render blame info by default', () => {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/loading/__tests__/loading.spec.tsx` around lines 69 - 73, The test name is ambiguous about default behavior; rename the test in loading.spec.tsx to clarify it checks the default (no query param) case by changing the spec for the LoadingBox component from "does not render blame info when query param disabled" to something like "does not render blame info by default" and keep the body unchanged (render(<LoadingBox blame={label} />); expect(screen.queryByText(label)).not.toBeInTheDocument();).frontend/public/components/utils/__tests__/selector-input.spec.tsx (1)
94-103: Tighten tag-removal assertion to verify emitted valueLine 102 only checks that
onChangefired. Please also assert the callback payload reflects removal ofapp=frontend; otherwise this test can pass on incorrect behavior.As per coding guidelines, "-Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/public/components/utils/__tests__/selector-input.spec.tsx` around lines 94 - 103, The test "calls onChange when close button is clicked" only asserts the callback fired; update it to also assert the emitted payload removes "app=frontend". In the SelectorInput test, after clicking the close button, replace or extend the expectation on the mock onChange to verify it was called with the updated tags (e.g., ['env=prod'])—or with the exact payload shape SelectorInput emits (use toHaveBeenCalledWith and toHaveBeenCalledTimes(1) as appropriate) so the assertion verifies the removed tag rather than just the call.frontend/packages/console-shared/src/components/formik-fields/key-value-file-input-field/__tests__/KeyValueFileInputField.spec.tsx (1)
76-78: TightenwaitForusage to avoid false-positive timing behaviorAt Line 91,
waitFor(() => expect(queryByRole...).not...)can pass too early. The same pattern at Line 76 and Line 106 adds retry loops where direct orfindBy*assertions are clearer and less flaky.Suggested cleanup
- await waitFor(() => { - expect(screen.getAllByRole('button', { name: 'Remove key/value' })).toHaveLength(2); - }); + expect(screen.getAllByRole('button', { name: 'Remove key/value' })).toHaveLength(2); - await waitFor(() => { - expect(screen.queryByRole('button', { name: 'Remove key/value' })).not.toBeInTheDocument(); - }); + expect(screen.queryByRole('button', { name: 'Remove key/value' })).not.toBeInTheDocument(); - await waitFor(() => { - expect(screen.getByRole('button', { name: 'Remove key/value' })).toBeVisible(); - }); + expect(screen.getByRole('button', { name: 'Remove key/value' })).toBeVisible(); - await waitFor(() => { - expect(screen.getAllByRole('textbox', { name: 'Key' })).toHaveLength(2); - }); + expect(await screen.findAllByRole('textbox', { name: 'Key' })).toHaveLength(2);As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
Also applies to: 91-93, 106-108, 125-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/formik-fields/key-value-file-input-field/__tests__/KeyValueFileInputField.spec.tsx` around lines 76 - 78, Tests using waitFor around assertions like screen.getAllByRole('button', { name: 'Remove key/value' }) and queryByRole(...) are adding unnecessary retry loops and can be flaky; replace those with explicit async queries (e.g., await screen.findAllByRole('button', { name: 'Remove key/value' }) and await screen.findByRole / findByText where appropriate) or direct synchronous assertions when the element is already present, and remove the surrounding waitFor blocks in the KeyValueFileInputField.spec.tsx tests (referencing the uses of screen.getAllByRole, queryByRole and similar calls) so assertions rely on findBy*/findAllBy* for waiting and on queryBy* for non-existence checks without waitFor.frontend/packages/console-shared/src/components/getting-started/__tests__/GettingStartedCard.spec.tsx (1)
15-18: Consider using a partial mock for@patternfly/react-iconsto make tests resilient to future icon additionsThe current full-module mock works for GettingStartedCard as currently written—it imports only
ArrowRightIconandExternalLinkAltIcon. However, this approach is brittle: if the component or its render subtree later adopts another PatternFly icon, the test will fail due to the incomplete mock shape rather than revealing a real component issue.Refactoring suggestion
-jest.mock('@patternfly/react-icons', () => ({ - ArrowRightIcon: () => 'ArrowRightIcon', - ExternalLinkAltIcon: () => 'ExternalLinkAltIcon', -})); +jest.mock('@patternfly/react-icons', () => { + const actual = jest.requireActual('@patternfly/react-icons'); + return { + ...actual, + ArrowRightIcon: () => 'ArrowRightIcon', + ExternalLinkAltIcon: () => 'ExternalLinkAltIcon', + }; +});🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/console-shared/src/components/getting-started/__tests__/GettingStartedCard.spec.tsx` around lines 15 - 18, Replace the full-module mock for '@patternfly/react-icons' in GettingStartedCard.spec.tsx with a partial mock that delegates to the real module and only overrides ArrowRightIcon and ExternalLinkAltIcon; specifically, change the jest.mock call so it calls jest.requireActual('@patternfly/react-icons') (or imports the actual module) and returns an object that spreads the actual exports and then replaces ArrowRightIcon and ExternalLinkAltIcon with lightweight stubs—this ensures other icons added later remain available and prevents brittle failures while keeping tests stable.frontend/packages/dev-console/src/components/import/advanced/ResourceLimitSection.tsx (1)
7-7: Consider using the barrel export path for consistency.The import changed from
@console/sharedto the direct file path. While functional, using barrel exports (@console/shared) is typically preferred for maintainability—it decouples consumers from internal file structure changes.💡 Suggested change
-import ResourceLimitField from '@console/shared/src/components/formik-fields/ResourceLimitField'; +import { ResourceLimitField } from '@console/shared';This requires ensuring
ResourceLimitFieldis exported from the@console/sharedbarrel. If it's intentionally not in the barrel, the direct path is acceptable.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/dev-console/src/components/import/advanced/ResourceLimitSection.tsx` at line 7, The import in ResourceLimitSection uses the direct path to ResourceLimitField; switch it to the barrel export by importing ResourceLimitField from "@console/shared" for consistency (or, if ResourceLimitField is intentionally not exported from the barrel, leave the direct path but document/confirm the reason). Ensure the barrel index in `@console/shared` re-exports ResourceLimitField (add an export { default as ResourceLimitField } from '<internal-path>' if missing) so consumers can import from "@console/shared" without coupling to internal file layout.frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx (1)
28-132: Scope these mocks to theOperandDetailscoverage.These file-level
jest.mockcalls now affect every describe block in the barrel spec, not just the newOperandDetailscases. That makesOperandStatus,ProvidedAPIPage, etc. less representative and can hide regressions behind mocked PF/layout/descriptor implementations. Moving this setup into theOperandDetailsblock or a dedicated spec file would preserve isolation.As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx` around lines 28 - 132, The file-level jest.mock calls are global and leak into other describe blocks (affecting tests like OperandStatus/ProvidedAPIPage); scope these mocks to only the OperandDetails tests by moving them into the OperandDetails describe block (or into a new dedicated spec file) and registering/unregistering them there (e.g., inside beforeAll/afterAll with jest.resetModules if needed). Specifically, relocate mocks for symbols like useK8sWatchResources, useK8sModels, useK8sModel, useParams/useLocation, Grid/GridItem/DescriptionList/PaneBody, ResourceSummary/SectionHeading, DescriptorDetailsItem/DescriptorConditions and DetailsPage so they are created only for the OperandDetails test scope and do not affect other test suites.frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx (1)
206-217: Derive the loaded-state expectations fromtestPackageManifest.
alpha/testappare copied from the fixture, andnew RegExp(testPackageManifest.metadata.name)is unescaped. Pull the expected channel/CSV from the fixture and escape the dynamic regex so unrelated fixture edits don't make this assertion brittle or overly permissive.♻️ Proposed tweak
renderWithProviders(<CreateSubscriptionYAML />); + const defaultChannel = testPackageManifest.status.channels.find( + ({ name }) => name === testPackageManifest.status.defaultChannel, + )!; - expect(screen.getByText(new RegExp(testPackageManifest.metadata.name))).toBeVisible(); - expect(screen.getByText(/channel:\s*alpha/)).toBeVisible(); + expect( + screen.getByText(new RegExp(_.escapeRegExp(testPackageManifest.metadata.name))), + ).toBeVisible(); + expect( + screen.getByText(new RegExp(`channel:\\s*${_.escapeRegExp(defaultChannel.name)}`)), + ).toBeVisible(); expect(screen.getByText(/source:\s*ocs/)).toBeVisible(); - expect(screen.getByText(/startingCSV:\s*testapp/)).toBeVisible(); + expect( + screen.getByText( + new RegExp(`startingCSV:\\s*${_.escapeRegExp(defaultChannel.currentCSV)}`), + ), + ).toBeVisible();As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx` around lines 206 - 217, The assertions in the test should derive expected values from the testPackageManifest fixture and use an escaped regex to avoid brittle/unintended matches: update the three expectations that currently hardcode "alpha", "ocs", and "testapp" (and the unescaped package name regex) to read channel, source and startingCSV (and metadata.name) from testPackageManifest, escape them with an escapeRegExp helper, and pass the escaped strings into RegExp for screen.getByText; adjust the test around CreateSubscriptionYAML and mockUseK8sWatchResources to use those derived, escaped values so the assertions remain correct when the fixture changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@frontend/packages/console-app/src/components/modals/resource-limits/__tests__/ResourceLimitsModal.spec.tsx`:
- Around line 186-193: The test asserts validation UI immediately after keyboard
input but Formik validation runs in a useEffect, so update the assertions in
ResourceLimitsModal.spec.tsx to wait for asynchronous state changes: after
interacting with cpuRequest (the spinbutton) and entering '300', replace the
immediate expect(save).toBeDisabled() with an awaited assertion inside waitFor()
or use findByText/findByRole to await the disabled state, and replace the direct
screen.getByText('CPU request must be less than or equal to limit.') check with
await screen.findByText(...) (or assert inside waitFor) so the test waits for
Formik's validateForm to finish; apply the same pattern for other similar
assertions around lines where save and validation messages are checked.
---
Nitpick comments:
In
`@frontend/packages/console-shared/src/components/formik-fields/key-value-file-input-field/__tests__/KeyValueFileInputField.spec.tsx`:
- Around line 76-78: Tests using waitFor around assertions like
screen.getAllByRole('button', { name: 'Remove key/value' }) and queryByRole(...)
are adding unnecessary retry loops and can be flaky; replace those with explicit
async queries (e.g., await screen.findAllByRole('button', { name: 'Remove
key/value' }) and await screen.findByRole / findByText where appropriate) or
direct synchronous assertions when the element is already present, and remove
the surrounding waitFor blocks in the KeyValueFileInputField.spec.tsx tests
(referencing the uses of screen.getAllByRole, queryByRole and similar calls) so
assertions rely on findBy*/findAllBy* for waiting and on queryBy* for
non-existence checks without waitFor.
In
`@frontend/packages/console-shared/src/components/getting-started/__tests__/GettingStartedCard.spec.tsx`:
- Around line 15-18: Replace the full-module mock for '@patternfly/react-icons'
in GettingStartedCard.spec.tsx with a partial mock that delegates to the real
module and only overrides ArrowRightIcon and ExternalLinkAltIcon; specifically,
change the jest.mock call so it calls
jest.requireActual('@patternfly/react-icons') (or imports the actual module) and
returns an object that spreads the actual exports and then replaces
ArrowRightIcon and ExternalLinkAltIcon with lightweight stubs—this ensures other
icons added later remain available and prevents brittle failures while keeping
tests stable.
In
`@frontend/packages/console-shared/src/components/loading/__tests__/loading.spec.tsx`:
- Around line 69-73: The test name is ambiguous about default behavior; rename
the test in loading.spec.tsx to clarify it checks the default (no query param)
case by changing the spec for the LoadingBox component from "does not render
blame info when query param disabled" to something like "does not render blame
info by default" and keep the body unchanged (render(<LoadingBox blame={label}
/>); expect(screen.queryByText(label)).not.toBeInTheDocument();).
In
`@frontend/packages/dev-console/src/components/import/advanced/ResourceLimitSection.tsx`:
- Line 7: The import in ResourceLimitSection uses the direct path to
ResourceLimitField; switch it to the barrel export by importing
ResourceLimitField from "@console/shared" for consistency (or, if
ResourceLimitField is intentionally not exported from the barrel, leave the
direct path but document/confirm the reason). Ensure the barrel index in
`@console/shared` re-exports ResourceLimitField (add an export { default as
ResourceLimitField } from '<internal-path>' if missing) so consumers can import
from "@console/shared" without coupling to internal file layout.
In
`@frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx`:
- Around line 206-217: The assertions in the test should derive expected values
from the testPackageManifest fixture and use an escaped regex to avoid
brittle/unintended matches: update the three expectations that currently
hardcode "alpha", "ocs", and "testapp" (and the unescaped package name regex) to
read channel, source and startingCSV (and metadata.name) from
testPackageManifest, escape them with an escapeRegExp helper, and pass the
escaped strings into RegExp for screen.getByText; adjust the test around
CreateSubscriptionYAML and mockUseK8sWatchResources to use those derived,
escaped values so the assertions remain correct when the fixture changes.
In
`@frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx`:
- Around line 28-132: The file-level jest.mock calls are global and leak into
other describe blocks (affecting tests like OperandStatus/ProvidedAPIPage);
scope these mocks to only the OperandDetails tests by moving them into the
OperandDetails describe block (or into a new dedicated spec file) and
registering/unregistering them there (e.g., inside beforeAll/afterAll with
jest.resetModules if needed). Specifically, relocate mocks for symbols like
useK8sWatchResources, useK8sModels, useK8sModel, useParams/useLocation,
Grid/GridItem/DescriptionList/PaneBody, ResourceSummary/SectionHeading,
DescriptorDetailsItem/DescriptorConditions and DetailsPage so they are created
only for the OperandDetails test scope and do not affect other test suites.
In `@frontend/public/components/utils/__tests__/selector-input.spec.tsx`:
- Around line 94-103: The test "calls onChange when close button is clicked"
only asserts the callback fired; update it to also assert the emitted payload
removes "app=frontend". In the SelectorInput test, after clicking the close
button, replace or extend the expectation on the mock onChange to verify it was
called with the updated tags (e.g., ['env=prod'])—or with the exact payload
shape SelectorInput emits (use toHaveBeenCalledWith and toHaveBeenCalledTimes(1)
as appropriate) so the assertion verifies the removed tag rather than just the
call.
🪄 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), Organization UI (inherited)
Review profile: CHILL
Plan: Pro
Run ID: a6f1c433-0927-4332-a00b-b95417f1f640
📒 Files selected for processing (18)
frontend/packages/console-app/src/components/modals/resource-limits/__tests__/ResourceLimitsModal.spec.tsxfrontend/packages/console-shared/src/components/empty-state/__tests__/empty-state.spec.tsxfrontend/packages/console-shared/src/components/formik-fields/NumberSpinnerField.tsxfrontend/packages/console-shared/src/components/formik-fields/ResourceLimitField.tsxfrontend/packages/console-shared/src/components/formik-fields/__tests__/NumberSpinnerField.spec.tsxfrontend/packages/console-shared/src/components/formik-fields/field-types.tsfrontend/packages/console-shared/src/components/formik-fields/key-value-file-input-field/__tests__/KeyValueFileInputField.spec.tsxfrontend/packages/console-shared/src/components/getting-started/__tests__/GettingStartedCard.spec.tsxfrontend/packages/console-shared/src/components/loading/__tests__/loading.spec.tsxfrontend/packages/console-shared/src/components/status/__tests__/status-box.spec.tsxfrontend/packages/dev-console/locales/en/devconsole.jsonfrontend/packages/dev-console/src/components/import/advanced/ResourceLimitSection.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsxfrontend/public/components/utils/__tests__/selector-input.spec.tsxfrontend/public/components/utils/number-spinner.tsxfrontend/public/components/utils/request-size-input.tsxfrontend/public/components/utils/selector-input.jsx
💤 Files with no reviewable changes (1)
- frontend/public/components/utils/selector-input.jsx
📜 Review details
🧰 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/dev-console/locales/en/devconsole.jsonfrontend/packages/console-shared/src/components/formik-fields/field-types.tsfrontend/packages/console-shared/src/components/formik-fields/ResourceLimitField.tsxfrontend/packages/console-shared/src/components/formik-fields/NumberSpinnerField.tsxfrontend/public/components/utils/request-size-input.tsxfrontend/packages/console-shared/src/components/status/__tests__/status-box.spec.tsxfrontend/public/components/utils/number-spinner.tsxfrontend/packages/dev-console/src/components/import/advanced/ResourceLimitSection.tsxfrontend/packages/console-shared/src/components/loading/__tests__/loading.spec.tsxfrontend/packages/console-shared/src/components/formik-fields/__tests__/NumberSpinnerField.spec.tsxfrontend/public/components/utils/__tests__/selector-input.spec.tsxfrontend/packages/console-shared/src/components/empty-state/__tests__/empty-state.spec.tsxfrontend/packages/console-app/src/components/modals/resource-limits/__tests__/ResourceLimitsModal.spec.tsxfrontend/packages/console-shared/src/components/formik-fields/key-value-file-input-field/__tests__/KeyValueFileInputField.spec.tsxfrontend/packages/console-shared/src/components/getting-started/__tests__/GettingStartedCard.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsxfrontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx
🔇 Additional comments (24)
frontend/packages/console-shared/src/components/loading/__tests__/loading.spec.tsx (3)
1-2: Good import structure aligning with RTL best practices.Using
screenqueries andrenderWithProvidersfor components requiring context (Redux, Router) is the correct approach. The distinction betweenrenderWithProvidersforLoadErrorand plainrenderfor stateless indicators is appropriate given the provider dependencies shown in the relevant snippets.
25-35: Solid role-based assertions for accessibility.Using
getByRole('button', { name: 'Try again' })andqueryByRoleis excellent — this ensures the retry button is actually accessible and labeled correctly, which is critical for a11y compliance in PatternFly-based UIs. Much better than relying on text content alone.
46-53: Clean synchronous assertions for inline loader.The switch from async
await getByTestIdto synchronousscreen.getByTestIdwithtoBeVisible()is appropriate here since the component renders synchronously. Verifying theco-m-loader--inlineclass ensures the correct inline styling variant is applied.frontend/packages/console-shared/src/components/status/__tests__/status-box.spec.tsx (1)
22-28: [Rewritten review comment]
[Classification tag]frontend/public/components/utils/__tests__/selector-input.spec.tsx (2)
16-51: Strong coverage for selector conversion helpersGood set of cases for
arrayify,objectify, and requirement/object conversion paths (including null/empty handling).
53-90: Validation and rendering tests are well-structuredThese assertions cover key UX states (placeholder behavior, invalid class toggling, and validation callback transitions) with appropriate async handling.
frontend/packages/console-shared/src/components/formik-fields/key-value-file-input-field/__tests__/KeyValueFileInputField.spec.tsx (1)
47-62: Good move to user-driven interactions and accessible queriesThe updated flows (click/tab + role/name queries) make these tests closer to real behavior and less coupled to implementation details.
Also applies to: 111-127
frontend/packages/console-shared/src/components/getting-started/__tests__/GettingStartedCard.spec.tsx (1)
32-113: Strong RTL coverage expansion hereThe interaction and rendering matrix is materially improved (link variants, click paths, loading/empty states, and style assertion), and this aligns well with the migration goals.
frontend/packages/console-shared/src/components/empty-state/__tests__/empty-state.spec.tsx (1)
1-83: Solid RTL test modernization with proper query patterns.Good adoption of
renderWithProviders,screenqueries, andtoBeVisible()assertions. The role-based queries (lines 48-49) follow accessibility testing best practices. Test coverage for conditional rendering paths (body/footer presence, error details alert) is thorough.frontend/packages/dev-console/locales/en/devconsole.json (1)
359-366: Appropriate i18n keys for accessibility labeling.New translation keys for resource limit aria-labels follow the established pattern. These enable proper localization of the accessibility labels used by screen readers.
frontend/packages/console-shared/src/components/formik-fields/field-types.ts (1)
107-114: Backward-compatible API extension for accessibility.Adding
inputAriaLabel?: stringas optional maintains backward compatibility while enabling proper ARIA labeling. Clean type definition.frontend/packages/console-shared/src/components/formik-fields/ResourceLimitField.tsx (1)
29-42: Proper accessibility wiring with bothariaLabelanddescribedBy.Good pattern passing both
ariaLabelfor the accessible name anddescribedByfor helper text association. TheinputID={fieldId}ensures the input is properly linked to theFormGrouplabel viafieldId.frontend/public/components/utils/request-size-input.tsx (1)
56-68: Clean ARIA label forwarding to NumberSpinner.Proper pass-through of
aria-labelalongside existingaria-describedby. The optional typing maintains backward compatibility.frontend/packages/console-shared/src/components/formik-fields/NumberSpinnerField.tsx (1)
12-18: Interface properly extended for min/max bounds.Adding optional
minandmaxprops with pass-through via spread maintains backward compatibility. These will correctly flow to the underlyingNumberSpinnercomponent.frontend/packages/dev-console/src/components/import/advanced/ResourceLimitSection.tsx (1)
36-75: Proper accessibility labeling for all resource limit fields.Each
ResourceLimitFieldnow has a distinct, localizedinputAriaLabelthat clearly describes the field purpose for screen reader users. This follows a11y best practices for form inputs.frontend/public/components/utils/number-spinner.tsx (1)
16-34: PatternFly NumberInput props correctly applied for accessibility and validation.The
inputAriaLabelprop properly labels the inner input element per PatternFly's API, and themaxprop enables upper-bound enforcement. Combined withminusBtnAriaLabelandplusBtnAriaLabel, this provides complete accessibility coverage for the spinner control.frontend/packages/console-shared/src/components/formik-fields/__tests__/NumberSpinnerField.spec.tsx (1)
10-110: Strong RTL coverage upgrade for NumberSpinnerFieldGood modernization here: role-based queries,
userEvent, keyboard behavior, and min/max boundary checks all improve long-term test reliability and accessibility confidence.frontend/packages/console-app/src/components/modals/resource-limits/__tests__/ResourceLimitsModal.spec.tsx (1)
87-113: Nice test harness consolidation
renderResourceLimitsModalis a solid helper: less duplication, realistic Formik wiring, and easier future extension for validation scenarios.frontend/packages/operator-lifecycle-manager/src/components/operand/__tests__/index.spec.tsx (1)
216-299: Status precedence coverage is solid.These cases line up with
getOperandStatus's phase → status → state → conditions order and cover both array/objectconditionsplus the no-true fallback.frontend/packages/operator-lifecycle-manager/src/components/__tests__/catalog-source.spec.tsx (5)
43-64: Good simplification of the suite-local test doubles.Rendering these helpers as deterministic text wrappers keeps the assertions focused on
CatalogSourcebehavior instead of PatternFly/internal markup.
111-138: Nice coverage for the details branches.This now exercises the
DEFAULT_SOURCE_NAMESPACEmapping fromfrontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx:103-106and thespec.imageendpoint branch.
152-171: GoodDetailsPagewiring check.Verifying
namespace,name,kind, and the three tab keys keeps this wrapper test focused on the contract it owns.
196-204: Good loading-path coverage.This makes the not-yet-loaded branch explicit instead of only asserting the rendered YAML case.
220-237: Nice fallback coverage for missingdefaultChannel.This directly hits the
pkg.status.defaultChannel ? ... : pkg.status.channels[0]branch infrontend/packages/operator-lifecycle-manager/src/components/catalog-source.tsx:245-247.
...ges/console-app/src/components/modals/resource-limits/__tests__/ResourceLimitsModal.spec.tsx
Show resolved
Hide resolved
cajieh
force-pushed
the
extend-rtl-ests-overage-omponents-igrated-enzyme
branch
from
86b8c52 to
f852b6d
Compare
|
/retest |
|
/test e2e-gcp-console |
1 similar comment
|
/test e2e-gcp-console |
|
/label docs-approved /assign @yapei |
|
@cajieh: GitHub didn't allow me to assign the following users: yapei. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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 kubernetes-sigs/prow repository. |
openshift-ci
bot
added
docs-approved
|
/assign @yanpzhan |
|
@cajieh: GitHub didn't allow me to assign the following users: yanpzhan. Note that only openshift members with read permissions, repo collaborators and people who have commented on this issue/PR can be assigned. Additionally, issues/PRs can only have 10 assignees at the same time. 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 kubernetes-sigs/prow repository. |
|
nit: if possible, I'd convert all the |
|
@cajieh: The following test failed, say
Full PR test history. Your PR dashboard. DetailsInstructions 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. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cajieh, stefanonardo 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 |
@stefanonardo Thank you for review. |
|
/test e2e-gcp-console |
3 participants
Summary by CodeRabbit
New Features
Bug Fixes
Accessibility
Localization