CONSOLE-5229: Enable RTL ESLint rules in Knative tests by removing file-level no-container / no-node-access suppressions#16405
Conversation
|
@cajieh: This pull request references CONSOLE-5229 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. |
openshift-ci-robot
added
the
jira/valid-reference
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
|
[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
the
approved
|
/retest |
cajieh
force-pushed
the
enable-rtl-eslint-rules-knative-tests
branch
3 times, most recently
from
3a3f172 to
1e11c38
Compare
|
/test all |
cajieh
force-pushed
the
enable-rtl-eslint-rules-knative-tests
branch
3 times, most recently
from
7d7fd65 to
22ef89d
Compare
|
/test images |
|
/test e2e-gcp-console |
cajieh
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
cajieh
force-pushed
the
enable-rtl-eslint-rules-knative-tests
branch
from
22ef89d to
158e48c
Compare
📝 WalkthroughWalkthroughThis pull request systematically refactors test infrastructure and test files in the Knative plugin to adopt React Testing Library best practices. A new 🚥 Pre-merge checks | ✅ 11 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (11 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 (3)
frontend/packages/knative-plugin/src/components/overview/serving-list/__tests__/ServingListPage.spec.tsx (1)
52-56: ⚡ Quick winRemove redundant duplicate render test.
Line 52–56 repeats the exact same assertions as Line 46–50, so it doesn’t increase confidence and just adds maintenance overhead.
Proposed cleanup
- it('should render the main components without errors', () => { - render(<ServingListPage />); - expect(screen.getByTestId('mock-NamespaceBar')).toBeVisible(); - expect(screen.getByTestId('mock-MultiTabListPage')).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/knative-plugin/src/components/overview/serving-list/__tests__/ServingListPage.spec.tsx` around lines 52 - 56, Remove the duplicated test block that renders ServingListPage and asserts visibility of 'mock-NamespaceBar' and 'mock-MultiTabListPage' — specifically delete the redundant it(...) test that calls render(<ServingListPage />) and the two expect(...) assertions so only the original test remains; look for the duplicated "should render the main components without errors" it block referencing ServingListPage and the test ids 'mock-NamespaceBar' and 'mock-MultiTabListPage' and remove the second occurrence.frontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSourceModal.spec.tsx (1)
23-27: 💤 Low value
Formmock spreads all PF-specific props onto a native<form>element.
PF'sFormaccepts props likeisWidthLimited,isHorizontal, etc. that are not valid HTML attributes. Spreading the entire...propsremainder onto<form>will cause React to emit"Unknown prop"DOM warnings during the test run. These don't fail tests on their own, but they pollute the output and can mask real warnings. Filtering known-non-DOM props, or only forwardingid, would keep the test output clean.🧹 Proposed tidy-up
- Form: jest.fn(({ children, ...props }) => ( - <form data-test={props.id} {...props}> - {children} - </form> - )), + Form: jest.fn(({ children, id, ...rest }: any) => ( + <form data-test={id} id={id}> + {children} + </form> + )),The same concern applies identically to the
TrafficSplittingModal.spec.tsxFormmock (line 26-30).🤖 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/knative-plugin/src/components/sink-source/__tests__/SinkSourceModal.spec.tsx` around lines 23 - 27, The Form mock in SinkSourceModal.spec.tsx currently spreads all PF-specific props onto a native <form>, causing React "Unknown prop" warnings; update the jest.fn mock for Form (and the analogous Form mock in TrafficSplittingModal.spec.tsx) to avoid spreading unknown props — only forward safe DOM attributes like id (e.g., use the id prop and children) or explicitly filter out PF-only props such as isWidthLimited/isHorizontal before passing props to the <form>, so tests no longer emit DOM unknown-prop warnings.frontend/packages/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsub.spec.tsx (1)
75-94: ⚡ Quick winThree of four test cases are functional duplicates — the suite provides no additional coverage.
"should render the component with Formik"(lines 75–78) and"should render properly for Subscription resource type"(lines 80–83) are byte-for-byte identical: sameformProps, same single assertion."should handle different source objects"(lines 91–94) also renders withformProps(the Subscription fixture) without supplying any alternate source, making it identical to tests 1 and 2.The only genuinely distinct case is the
Triggertest (lines 85–89).♻️ Proposed cleanup
- it('should render the component with Formik', () => { - render(<SinkPubsub {...formProps} />); - expect(screen.getByTestId('mock-Formik')).toBeVisible(); - }); - - it('should render properly for Subscription resource type', () => { - render(<SinkPubsub {...formProps} />); - expect(screen.getByTestId('mock-Formik')).toBeVisible(); - }); + it('should render Formik wrapper for Subscription resource type', () => { + render(<SinkPubsub {...formProps} />); + expect(screen.getByTestId('mock-Formik')).toBeVisible(); + }); it('should render properly for Trigger resource type', () => { const triggerProps = { source: EventTriggerObj, resourceType: 'Trigger' }; render(<SinkPubsub {...triggerProps} />); expect(screen.getByTestId('mock-Formik')).toBeVisible(); }); - it('should handle different source objects', () => { - render(<SinkPubsub {...formProps} />); - expect(screen.getByTestId('mock-Formik')).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/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsub.spec.tsx` around lines 75 - 94, Tests contain three identical renders using the same formProps (Subscription) which is redundant; keep one Subscription render test and the distinct Trigger test (using EventTriggerObj and resourceType: 'Trigger'), and either remove the other duplicate tests ("should render the component with Formik" and one of the Subscription tests) or change the "should handle different source objects" test to actually supply a different source (e.g., a different fixture than formProps or a null/edge-case source) and assert expected behavior; update references to SinkPubsub, formProps, EventTriggerObj, and resourceType in the affected test cases accordingly.
🤖 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/knative-plugin/src/components/add/__tests__/EventSink.spec.tsx`:
- Around line 18-29: The Formik mock is leaking real Formik hooks by spreading
...actual, which lets components like EventSinkSection (used inside
EventSinkForm) call useFormikContext() against no provider and fail; fix by
removing the ...actual spread from the jest.mock so the module is fully mocked
(or replace the mock with the existing mockFormikRenderer test utility from
unit-test-utils to render the component inside a real Formik provider), ensuring
Formik is either fully stubbed or the component is wrapped with a proper Formik
context so useFormikContext/Field/useField don't run against undefined.
In
`@frontend/packages/knative-plugin/src/components/overview/__tests__/RevisionsOverviewList.spec.tsx`:
- Around line 93-100: The test for RevisionsOverviewList currently only asserts
visibility of the traffic split button; update the assertion to check the
disabled state as the test name promises by replacing or augmenting the existing
expect(screen.getByTestId('mock-Button')).toBeVisible() with
expect(screen.getByTestId('mock-Button')).toBeDisabled() (or add that
assertion), so the "no revisions" scenario verifies the button is disabled;
target the getByTestId('mock-Button') call in RevisionsOverviewList.spec.tsx.
In
`@frontend/packages/knative-plugin/src/components/overview/__tests__/RoutesOverviewList.spec.tsx`:
- Around line 55-63: The test "should render multiple RoutesOverviewListItem
when multiple routes" only asserts a single visible item; update the test in
RoutesOverviewList.spec.tsx to assert multiplicity by querying all items
produced by <RoutesOverviewList> (use the test id 'mock-RoutesOverviewListItem')
and expecting length to equal the number of routes in
MockKnativeResources.ksroutes.data (or at least >1); ensure you use
screen.getAllByTestId('mock-RoutesOverviewListItem') and assert the resulting
array length matches/mock data count rather than a single getByTestId visibility
check.
In
`@frontend/packages/knative-plugin/src/components/pub-sub/details/__test__/TriggerDetails.spec.tsx`:
- Around line 60-64: The test for TriggerDetails uses getByTestId which throws
if the element is missing, and currently asserts visibility even though the spec
says FilterTable should not render; update the negative-case assertion by using
screen.queryByTestId('mock-FilterTable') on the TriggerDetails render (with
triggerDataClone where spec.filter is omitted) and assert that it is null or not
present (e.g., expect(...).toBeNull() or expect(...).not.toBeInTheDocument()) so
the test correctly verifies FilterTable is not rendered.
---
Nitpick comments:
In
`@frontend/packages/knative-plugin/src/components/overview/serving-list/__tests__/ServingListPage.spec.tsx`:
- Around line 52-56: Remove the duplicated test block that renders
ServingListPage and asserts visibility of 'mock-NamespaceBar' and
'mock-MultiTabListPage' — specifically delete the redundant it(...) test that
calls render(<ServingListPage />) and the two expect(...) assertions so only the
original test remains; look for the duplicated "should render the main
components without errors" it block referencing ServingListPage and the test ids
'mock-NamespaceBar' and 'mock-MultiTabListPage' and remove the second
occurrence.
In
`@frontend/packages/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsub.spec.tsx`:
- Around line 75-94: Tests contain three identical renders using the same
formProps (Subscription) which is redundant; keep one Subscription render test
and the distinct Trigger test (using EventTriggerObj and resourceType:
'Trigger'), and either remove the other duplicate tests ("should render the
component with Formik" and one of the Subscription tests) or change the "should
handle different source objects" test to actually supply a different source
(e.g., a different fixture than formProps or a null/edge-case source) and assert
expected behavior; update references to SinkPubsub, formProps, EventTriggerObj,
and resourceType in the affected test cases accordingly.
In
`@frontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSourceModal.spec.tsx`:
- Around line 23-27: The Form mock in SinkSourceModal.spec.tsx currently spreads
all PF-specific props onto a native <form>, causing React "Unknown prop"
warnings; update the jest.fn mock for Form (and the analogous Form mock in
TrafficSplittingModal.spec.tsx) to avoid spreading unknown props — only forward
safe DOM attributes like id (e.g., use the id prop and children) or explicitly
filter out PF-only props such as isWidthLimited/isHorizontal before passing
props to the <form>, so tests no longer emit DOM unknown-prop warnings.
🪄 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: bd9e1994-1fc0-4861-95d5-72b233cb22a9
📒 Files selected for processing (50)
frontend/__mocks__/react-i18next.tsfrontend/packages/knative-plugin/src/__tests__/rtl-stub-components.tsxfrontend/packages/knative-plugin/src/components/add/__tests__/EventSink.spec.tsxfrontend/packages/knative-plugin/src/components/add/__tests__/EventSinkAlert.spec.tsxfrontend/packages/knative-plugin/src/components/add/__tests__/EventSinkForm.spec.tsxfrontend/packages/knative-plugin/src/components/add/__tests__/EventSinkPage.spec.tsxfrontend/packages/knative-plugin/src/components/add/__tests__/EventSource.spec.tsxfrontend/packages/knative-plugin/src/components/add/__tests__/EventSourceAlert.spec.tsxfrontend/packages/knative-plugin/src/components/add/__tests__/EventSourceForm.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sinks/__tests__/EventSinkSection.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sinks/__tests__/KafkaSinkSection.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sources/__tests__/ApiServerSection.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sources/__tests__/ContainerSourceSection.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sources/__tests__/EventSourceSection.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sources/__tests__/KafkaSourceNetSection.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sources/__tests__/KafkaSourceSection.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sources/__tests__/SinkBindingSection.spec.tsxfrontend/packages/knative-plugin/src/components/add/event-sources/form-fields/__tests__/SinkResources.spec.tsxfrontend/packages/knative-plugin/src/components/eventing/__tests__/EventingListPage.spec.tsxfrontend/packages/knative-plugin/src/components/knatify/__tests__/CreateKnatifyPage.spec.tsxfrontend/packages/knative-plugin/src/components/knatify/__tests__/KnatifyForm.spec.tsxfrontend/packages/knative-plugin/src/components/overview/RevisionsOverviewListItem.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/ConfigurationsOverviewList.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/ConfigurationsOverviewListItem.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/DeploymentOverviewList.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/EventPubSubResources.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/EventSourceOwnedList.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/EventSourceResources.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/KSRouteSplitListItem.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/KSRoutesOverviewListItem.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/RevisionsOverviewList.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/RevisionsOverviewListItem.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/RoutesOverviewList.spec.tsxfrontend/packages/knative-plugin/src/components/overview/__tests__/RoutesOverviewListItem.spec.tsxfrontend/packages/knative-plugin/src/components/overview/serving-list/__tests__/ServingListPage.spec.tsxfrontend/packages/knative-plugin/src/components/pub-sub/details/__test__/DynamicResourceLink.spec.tsxfrontend/packages/knative-plugin/src/components/pub-sub/details/__test__/SubscriptionDetails.spec.tsxfrontend/packages/knative-plugin/src/components/pub-sub/details/__test__/TriggerDetails.spec.tsxfrontend/packages/knative-plugin/src/components/revisions/__tests__/RevisionRow.spec.tsxfrontend/packages/knative-plugin/src/components/routes/__tests__/RouteRow.spec.tsxfrontend/packages/knative-plugin/src/components/services/__tests__/ServiceRow.spec.tsxfrontend/packages/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsub.spec.tsxfrontend/packages/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsubModal.spec.tsxfrontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSource.spec.tsxfrontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSourceModal.spec.tsxfrontend/packages/knative-plugin/src/components/sink-uri/__tests__/SinkUri.spec.tsxfrontend/packages/knative-plugin/src/components/sink-uri/__tests__/SinkUriModal.spec.tsxfrontend/packages/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingFields.spec.tsxfrontend/packages/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingModal.spec.tsxfrontend/packages/knative-plugin/src/topology/sidebar/__tests__/KnativeOverviewSections.spec.tsx
📜 Review details
🔇 Additional comments (40)
frontend/packages/knative-plugin/src/components/overview/serving-list/__tests__/ServingListPage.spec.tsx (1)
1-1: Good RTL migration in this spec.Switching to
screen-based assertions and component mocks with stable test hooks is clean and aligned with the lint-rule enablement goal.Also applies to: 11-16, 47-49
frontend/packages/knative-plugin/src/components/eventing/__tests__/EventingListPage.spec.tsx (2)
1-1: Nice update to RTL-compliant test patterns.Using render +
screenand functional JSX mocks here is a solid improvement and keeps the test intent clear.Also applies to: 12-20, 57-61
63-140: Coverage depth is solid for this page.Good to see the props contract assertion and namespace URL behavior checks retained alongside the RTL query migration.
frontend/packages/knative-plugin/src/components/overview/RevisionsOverviewListItem.tsx (1)
47-47: Good scoped test hook on the deployment branch.This keeps RTL selectors deterministic without changing the existing conditional render behavior.
frontend/packages/knative-plugin/src/components/services/__tests__/ServiceRow.spec.tsx (1)
10-14: RTL migration is clean and consistent.Using DOM-backed mocks plus
screen-based assertions removes container/node access and keeps the tests stable.Also applies to: 45-49, 72-145
frontend/packages/knative-plugin/src/__tests__/rtl-stub-components.tsx (1)
7-24: Nice extraction of reusable RTL stubs.This improves test maintainability and standardizes mocks across knative-plugin suites.
frontend/packages/knative-plugin/src/components/pub-sub/details/__test__/DynamicResourceLink.spec.tsx (1)
1-9: Good modernization of the DynamicResourceLink test.The shared stub +
screenassertions are straightforward and aligned with the lint-rule objective.Also applies to: 25-40
frontend/packages/knative-plugin/src/components/add/event-sources/form-fields/__tests__/SinkResources.spec.tsx (1)
17-20: Solid switch to shared text stubs andscreenqueries.This keeps the tests lint-compliant and easier to reason about.
Also applies to: 50-57
frontend/packages/knative-plugin/src/components/add/event-sinks/__tests__/EventSinkSection.spec.tsx (1)
10-33: Nice cleanup of test doubles and assertions.The deterministic mock text strategy makes the visibility/presence checks much clearer.
Also applies to: 98-118
frontend/packages/knative-plugin/src/components/add/event-sources/__tests__/EventSourceSection.spec.tsx (1)
1-22: Looks good—consistent RTL query migration here as well.Assertions are now aligned with user-visible output rather than DOM internals.
Also applies to: 42-44
frontend/packages/knative-plugin/src/components/revisions/__tests__/RevisionRow.spec.tsx (1)
10-20: Good RTL-first refactor for RevisionRow tests.The shift to shared stubs and
screenassertions is consistent and maintainable.Also applies to: 60-104
frontend/packages/knative-plugin/src/components/knatify/__tests__/CreateKnatifyPage.spec.tsx (1)
71-71: LGTM — clean migration to the centralizedreact-i18nextmock.frontend/packages/knative-plugin/src/components/add/__tests__/EventSourceAlert.spec.tsx (1)
1-35: LGTM — well-structured RTL migration.Correct use of
queryByText(absence assertion) vs.getByText(presence +toBeVisible()), and returning a plain string from theAlertmock is valid React 16+.frontend/packages/knative-plugin/src/components/add/event-sources/__tests__/KafkaSourceNetSection.spec.tsx (1)
36-41: LGTM — count assertion is precise and correctly cross-checked against the formik mock.2 SASL + 3 TLS = 5
SecretKeySelectorinstances. UsingtoHaveLength(5)is more declarative than the> 0pattern seen elsewhere and will catch regressions accurately.frontend/packages/knative-plugin/src/components/add/event-sources/__tests__/SinkBindingSection.spec.tsx (1)
50-63: LGTM — correct use of RTL queries and precise count assertions.frontend/packages/knative-plugin/src/components/knatify/__tests__/KnatifyForm.spec.tsx (1)
38-39: LGTM —FlexForm/FormBodyas transparent pass-throughs is the right call here.Making layout wrappers transparent ensures the stub text from each section actually renders and can be asserted on, without needing to know the internal DOM structure of those layout components.
frontend/packages/knative-plugin/src/components/add/event-sources/__tests__/ApiServerSection.spec.tsx (1)
50-61: LGTM — mixing real translation-key assertions with stub-based component assertions is a valid pattern here.
screen.getByText('Resource')relies on thei18nextmock stripping the namespace prefix (knative~Resource→'Resource'), which is the documented behavior. This is an intentional choice that ties the test to translation key stability — acceptable and more meaningful than purely mock-based assertions for user-visible labels.frontend/packages/knative-plugin/src/components/routes/__tests__/RouteRow.spec.tsx (1)
18-19: rtl-stub-components module is complete and correctly exported.Verification confirms
frontend/packages/knative-plugin/src/__tests__/rtl-stub-components.tsxexports all required symbols:
knativeInternalUtilsStubs.ResourceLink— properly typed and renders expected mockknativeInternalUtilsStubs.SidebarSectionHeading— also presentcreateKnativeTextStub— function implemented correctlyAll usages in test files (e.g., RouteRow.spec.tsx) match the actual module exports. No missing or malformed exports.
frontend/__mocks__/react-i18next.ts (1)
53-54: ⚡ Quick win
Transregex limitation verified, but not a concern for knative-plugin.The regex
/<\d+><\/\d+>/does match only empty adjacent tags (<0></0>), and react-i18next's documented standard is the numbered-tag convention with content:defaults="hello <0>world</0>". However, verification shows knative-plugin uses noTranscomponent withdefaultsat all—only theuseTranslation()hook. The codebase-wide pattern consistently uses the empty-tag form (<0></0>paired withcomponents={[children]}), which the mock correctly handles. The documented scope of the mock ("components-array-only pattern") aligns with actual usage.frontend/packages/knative-plugin/src/components/add/event-sinks/__tests__/KafkaSinkSection.spec.tsx (1)
40-59: Nice RTL migration here.The updated
screen-based assertions are clear and aligned with user-visible behavior for title/field presence checks.frontend/packages/knative-plugin/src/components/add/event-sources/__tests__/ContainerSourceSection.spec.tsx (1)
55-74: Good accessibility-oriented assertions.Using
getByRole('textbox', { name: ... })and visible text checks improves test intent and resilience.frontend/packages/knative-plugin/src/components/add/event-sources/__tests__/KafkaSourceSection.spec.tsx (1)
67-110: Solid conversion to RTL-first assertions.The required-field checks via accessible queries keep these tests closer to real user interaction semantics.
frontend/packages/knative-plugin/src/components/add/__tests__/EventSource.spec.tsx (1)
46-83: Good behavioral assertions for context-source branching.The heading/button/target checks clearly validate the two sink initialization paths.
frontend/packages/knative-plugin/src/components/pub-sub/details/__test__/SubscriptionDetails.spec.tsx (1)
35-49: Looks good.This keeps the detail tests RTL-friendly while preserving both presence and absence scenarios.
frontend/packages/knative-plugin/src/components/overview/__tests__/EventSourceResources.spec.tsx (1)
64-130: Strong scenario coverage retained after RTL cleanup.The sink target matrix remains well-tested while avoiding container/node access patterns.
frontend/packages/knative-plugin/src/topology/sidebar/__tests__/KnativeOverviewSections.spec.tsx (1)
74-75: LGTM on the i18n mock simplification.Using the shared
react-i18nextmock keeps this test aligned with the suite-wide pattern.frontend/packages/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingFields.spec.tsx (1)
19-19: Good simplification of i18n test setup.Using the shared
react-i18nextmock here keeps this spec consistent with the repo-wide RTL cleanup and removes duplicate mock logic.frontend/packages/knative-plugin/src/components/sink-uri/__tests__/SinkUriModal.spec.tsx (1)
36-36: Nice cleanup for translation mocking.Switching to the shared module mock keeps this test lean and aligned with the broader Knative test refactor.
frontend/packages/knative-plugin/src/components/sink-uri/__tests__/SinkUri.spec.tsx (1)
15-15: Looks good.This keeps i18n mocking centralized and avoids test-local mock drift.
frontend/packages/knative-plugin/src/components/sink-pubsub/__tests__/SinkPubsubModal.spec.tsx (1)
26-27: Good balance of shared mock reuse and local override.Reusing the common i18n mock while overriding
Transfor this spec keeps behavior explicit without reintroducing boilerplate.frontend/packages/knative-plugin/src/components/overview/__tests__/ConfigurationsOverviewList.spec.tsx (1)
7-8: Great RTL-style assertion update.Using a renderable stub and
screen-based assertions is cleaner and consistent with the lint-rule enablement goal.Also applies to: 17-19
frontend/packages/knative-plugin/src/components/add/__tests__/EventSinkAlert.spec.tsx (1)
1-1: Strong cleanup of test assertions.The move from container-driven checks to
screenqueries makes the intent clearer and matches the RTL linting direction for this PR.Also applies to: 5-5, 10-12, 15-17, 20-28
frontend/packages/knative-plugin/src/components/overview/__tests__/DeploymentOverviewList.spec.tsx (1)
6-11: Nice reuse of shared stub components.Pulling in
knativeInternalUtilsStubsand asserting viascreenimproves maintainability and keeps this spec aligned with the rest of the suite.Also applies to: 54-56
frontend/packages/knative-plugin/src/components/sink-source/__tests__/SinkSource.spec.tsx (1)
18-18: Good tightening of the Formik contract assertion.Checking
childrenas a function is a stronger, less brittle validation than a generic presence check.Also applies to: 62-65
frontend/packages/knative-plugin/src/components/traffic-splitting/__tests__/TrafficSplittingModal.spec.tsx (2)
26-30: Same non-DOM prop spread concern asSinkSourceModal.spec.tsx— see that comment for the suggested fix.
42-80: RTL migration is well-executed — LGTM.Bare
jest.mock('react-i18next')(no factory) is the cleanest pattern in the PR, and the role-based button queries ({ name: 'Save' }/{ name: 'Cancel' }) are properly user-facing rather than relying on translation keys.frontend/packages/knative-plugin/src/components/overview/__tests__/EventSourceOwnedList.spec.tsx (1)
1-26: Clean RTL migration — LGTM.The switch to
screen.getByTestIdagainst the sharedknativeInternalUtilsStubsis consistent with the pattern used across the PR.frontend/packages/knative-plugin/src/components/add/__tests__/EventSinkForm.spec.tsx (1)
1-52: Clean stub-component migration — LGTM.The
createKnativeTextStubpattern is applied consistently forSyncedEditorField,FormFooter,CodeEditorField, andEventSinkSection, and the barejest.mock('react-i18next')aligns with the project-wide auto-mock.frontend/packages/knative-plugin/src/components/add/__tests__/EventSinkPage.spec.tsx (1)
1-141: Solid RTL migration — LGTM.String-returning mock components (
'mock-LoadingBox','mock-EventSink','mock-EventSinkAlert') pair cleanly withscreen.getByText/screen.queryByText, and the symmetric positive/negative assertions in each test case give good signal.frontend/packages/knative-plugin/src/components/overview/__tests__/RevisionsOverviewListItem.spec.tsx (1)
110-113: ⚡ Quick winThe
toHaveAttribute('kind', 'RevisionModel')assertion is correctly implemented. TheknativeInternalUtilsStubs.ResourceLinkstub forwards all props as DOM attributes via the spread operator ({...(props as HTMLAttributes<HTMLSpanElement>)}), which means thekindprop will be properly set on the rendered element. RTL is also correctly configured infrontend/setup-tests.jsto usedata-testas the testIdAttribute, matching the stub's selector. The test assertion will pass and correctly validate the kind attribute.
| it('should show info if no Revisions present, link for all revisions should not be shown and traffic split button should be disabled', () => { | ||
| const { container } = render( | ||
| render( | ||
| <RevisionsOverviewList revisions={[]} service={MockKnativeResources.revisions.data[0]} />, | ||
| ); | ||
| expect(container.querySelector('Link')).not.toBeInTheDocument(); | ||
| expect(screen.queryByTestId('mock-Link')).not.toBeInTheDocument(); | ||
| expect(screen.getByText(/No Revisions found for this resource/)).toBeInTheDocument(); | ||
| const button = container.querySelector('Button'); | ||
| expect(button).toBeInTheDocument(); | ||
| expect(screen.getByTestId('mock-Button')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Disabled-state behavior is unasserted in the “no revisions” scenario.
The test name says the traffic split button should be disabled, but it currently checks only visibility.
Suggested fix
it('should show info if no Revisions present, link for all revisions should not be shown and traffic split button should be disabled', () => {
render(
<RevisionsOverviewList revisions={[]} service={MockKnativeResources.revisions.data[0]} />,
);
expect(screen.queryByTestId('mock-Link')).not.toBeInTheDocument();
expect(screen.getByText(/No Revisions found for this resource/)).toBeInTheDocument();
- expect(screen.getByTestId('mock-Button')).toBeVisible();
+ expect(screen.getByTestId('mock-Button')).toBeDisabled();
});📝 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('should show info if no Revisions present, link for all revisions should not be shown and traffic split button should be disabled', () => { | |
| const { container } = render( | |
| render( | |
| <RevisionsOverviewList revisions={[]} service={MockKnativeResources.revisions.data[0]} />, | |
| ); | |
| expect(container.querySelector('Link')).not.toBeInTheDocument(); | |
| expect(screen.queryByTestId('mock-Link')).not.toBeInTheDocument(); | |
| expect(screen.getByText(/No Revisions found for this resource/)).toBeInTheDocument(); | |
| const button = container.querySelector('Button'); | |
| expect(button).toBeInTheDocument(); | |
| expect(screen.getByTestId('mock-Button')).toBeVisible(); | |
| }); | |
| it('should show info if no Revisions present, link for all revisions should not be shown and traffic split button should be disabled', () => { | |
| render( | |
| <RevisionsOverviewList revisions={[]} service={MockKnativeResources.revisions.data[0]} />, | |
| ); | |
| expect(screen.queryByTestId('mock-Link')).not.toBeInTheDocument(); | |
| expect(screen.getByText(/No Revisions found for this resource/)).toBeInTheDocument(); | |
| expect(screen.getByTestId('mock-Button')).toBeDisabled(); | |
| }); |
🤖 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/knative-plugin/src/components/overview/__tests__/RevisionsOverviewList.spec.tsx`
around lines 93 - 100, The test for RevisionsOverviewList currently only asserts
visibility of the traffic split button; update the assertion to check the
disabled state as the test name promises by replacing or augmenting the existing
expect(screen.getByTestId('mock-Button')).toBeVisible() with
expect(screen.getByTestId('mock-Button')).toBeDisabled() (or add that
assertion), so the "no revisions" scenario verifies the button is disabled;
target the getByTestId('mock-Button') call in RevisionsOverviewList.spec.tsx.
| it('should render multiple RoutesOverviewListItem when multiple routes', () => { | ||
| const { container } = render( | ||
| render( | ||
| <RoutesOverviewList | ||
| ksroutes={MockKnativeResources.ksroutes.data} | ||
| resource={MockKnativeResources.revisions.data[0]} | ||
| />, | ||
| ); | ||
| expect(container.querySelector('routesoverviewlistitem')).toBeInTheDocument(); | ||
| expect(screen.getByTestId('mock-RoutesOverviewListItem')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
“Multiple routes” case is not actually asserting multiplicity.
This test currently checks only that one item is visible, so it doesn’t verify the behavior in the test name.
Suggested fix
it('should render multiple RoutesOverviewListItem when multiple routes', () => {
render(
<RoutesOverviewList
ksroutes={MockKnativeResources.ksroutes.data}
resource={MockKnativeResources.revisions.data[0]}
/>,
);
- expect(screen.getByTestId('mock-RoutesOverviewListItem')).toBeVisible();
+ expect(screen.getAllByTestId('mock-RoutesOverviewListItem').length).toBeGreaterThan(1);
});📝 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('should render multiple RoutesOverviewListItem when multiple routes', () => { | |
| const { container } = render( | |
| render( | |
| <RoutesOverviewList | |
| ksroutes={MockKnativeResources.ksroutes.data} | |
| resource={MockKnativeResources.revisions.data[0]} | |
| />, | |
| ); | |
| expect(container.querySelector('routesoverviewlistitem')).toBeInTheDocument(); | |
| expect(screen.getByTestId('mock-RoutesOverviewListItem')).toBeVisible(); | |
| }); | |
| it('should render multiple RoutesOverviewListItem when multiple routes', () => { | |
| render( | |
| <RoutesOverviewList | |
| ksroutes={MockKnativeResources.ksroutes.data} | |
| resource={MockKnativeResources.revisions.data[0]} | |
| />, | |
| ); | |
| expect(screen.getAllByTestId('mock-RoutesOverviewListItem').length).toBeGreaterThan(1); | |
| }); |
🤖 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/knative-plugin/src/components/overview/__tests__/RoutesOverviewList.spec.tsx`
around lines 55 - 63, The test "should render multiple RoutesOverviewListItem
when multiple routes" only asserts a single visible item; update the test in
RoutesOverviewList.spec.tsx to assert multiplicity by querying all items
produced by <RoutesOverviewList> (use the test id 'mock-RoutesOverviewListItem')
and expecting length to equal the number of routes in
MockKnativeResources.ksroutes.data (or at least >1); ensure you use
screen.getAllByTestId('mock-RoutesOverviewListItem') and assert the resulting
array length matches/mock data count rather than a single getByTestId visibility
check.
| it('should not render FilterTable if filter is not present', () => { | ||
| const triggerDataClone = _.omit(_.cloneDeep(triggerData), 'spec.filter'); | ||
| const { container } = render(<TriggerDetails obj={triggerDataClone} />); | ||
| expect(container.querySelector('filtertable')).toBeInTheDocument(); | ||
| render(<TriggerDetails obj={triggerDataClone} />); | ||
| expect(screen.getByTestId('mock-FilterTable')).toBeVisible(); | ||
| }); |
There was a problem hiding this comment.
Fix the negative-case assertion for missing filter.
Line 63 currently asserts FilterTable is visible, but the test name says it should not render when spec.filter is absent. This inverts the scenario’s intent.
Suggested fix
it('should not render FilterTable if filter is not present', () => {
const triggerDataClone = _.omit(_.cloneDeep(triggerData), 'spec.filter');
render(<TriggerDetails obj={triggerDataClone} />);
- expect(screen.getByTestId('mock-FilterTable')).toBeVisible();
+ expect(screen.queryByTestId('mock-FilterTable')).not.toBeInTheDocument();
});📝 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('should not render FilterTable if filter is not present', () => { | |
| const triggerDataClone = _.omit(_.cloneDeep(triggerData), 'spec.filter'); | |
| const { container } = render(<TriggerDetails obj={triggerDataClone} />); | |
| expect(container.querySelector('filtertable')).toBeInTheDocument(); | |
| render(<TriggerDetails obj={triggerDataClone} />); | |
| expect(screen.getByTestId('mock-FilterTable')).toBeVisible(); | |
| }); | |
| it('should not render FilterTable if filter is not present', () => { | |
| const triggerDataClone = _.omit(_.cloneDeep(triggerData), 'spec.filter'); | |
| render(<TriggerDetails obj={triggerDataClone} />); | |
| expect(screen.queryByTestId('mock-FilterTable')).not.toBeInTheDocument(); | |
| }); |
🤖 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/knative-plugin/src/components/pub-sub/details/__test__/TriggerDetails.spec.tsx`
around lines 60 - 64, The test for TriggerDetails uses getByTestId which throws
if the element is missing, and currently asserts visibility even though the spec
says FilterTable should not render; update the negative-case assertion by using
screen.queryByTestId('mock-FilterTable') on the TriggerDetails render (with
triggerDataClone where spec.filter is omitted) and assert that it is null or not
present (e.g., expect(...).toBeNull() or expect(...).not.toBeInTheDocument()) so
the test correctly verifies FilterTable is not rendered.
158e48c to
ee39c88
Compare
cajieh
force-pushed
the
enable-rtl-eslint-rules-knative-tests
branch
from
ee39c88 to
1ab93a6
Compare
|
/test all |
|
/test images |
|
/test e2e-gcp-console |
|
@cajieh: The following tests 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. |
2 participants
CONSOLE-5229: Enable RTL ESLint rules in Knative tests by removing file-level no-container / no-node-access suppressions
Analysis / Root cause:
Tech-Debt
Solution description:
Cleanup
Screenshots / screen recording:
N/A
Test setup:
Run
yarn test <Knative tests>Test cases:
N/A
Browser conformance:
Additional info:
Reviewers and assignees:
/assign @vikram-raj
Summary by CodeRabbit
Tests
Chores