EDM-3455: Add data-testid attributes for Cypress device E2E tests#551
Open
talhil-rh wants to merge 2 commits intoflightctl:mainfrom
Open
EDM-3455: Add data-testid attributes for Cypress device E2E tests#551talhil-rh wants to merge 2 commits intoflightctl:mainfrom
talhil-rh wants to merge 2 commits intoflightctl:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdded Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
talhil-rh
force-pushed
the
add-device-cypress-data-testids
branch
2 times, most recently
from
2ed33a1 to
3988646
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
libs/ui-components/src/components/common/ResourceLink.tsx (1)
43-49:data-testidis ignored whenrouteLinkis missing.If callers pass
data-testidandrouteLinkis falsy, no test id is rendered. Consider applying it to the fallback text element so the prop contract is consistent across both branches.Proposed diff
- const textEl = <span className="fctl-resource-link__text">{variant === 'full' ? nameOrId : displayText}</span>; + const textEl = ( + <span className="fctl-resource-link__text" data-testid={!routeLink ? dataTestId : undefined}> + {variant === 'full' ? nameOrId : displayText} + </span> + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/common/ResourceLink.tsx` around lines 43 - 49, The fallback branch in ResourceLink.tsx currently returns <> {textEl} </> which drops the data-testid when routeLink is falsy; update the fallback to render an element that accepts props (e.g., a span or the same component used for link children) and pass dataTestId to it so data-testid is present in both branches—ensure the Link branch still uses data-testid={dataTestId} and the fallback uses the same prop on the wrapper around textEl (reference symbols: ResourceLink.tsx, routeLink, Link, dataTestId, textEl, id).libs/ui-components/src/components/common/FlightCtlWizardFooter.tsx (1)
63-69: Consider adding test IDs to Close and Cancel buttons for completeness.The Close button (read-only mode) and Cancel button don't have
data-testidattributes. If these actions might be tested in future E2E flows, consider adding them now for consistency (e.g.,wizard-close-button,wizard-cancel-button).♻️ Optional: Add test IDs for completeness
primaryBtn = ( - <Button variant="primary" onClick={() => navigate(-1)}> + <Button variant="primary" onClick={() => navigate(-1)} data-testid="wizard-close-button"> {t('Close')} </Button> );- <Button variant="link" onClick={onCancel ?? (() => navigate(-1))} isDisabled={isSubmitting}> + <Button variant="link" onClick={onCancel ?? (() => navigate(-1))} isDisabled={isSubmitting} data-testid="wizard-cancel-button"> {t('Cancel')} </Button>Also applies to: 100-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@libs/ui-components/src/components/common/FlightCtlWizardFooter.tsx` around lines 63 - 69, The Close and Cancel buttons lack test IDs; update the JSX where primaryBtn is set for the read-only "Review" step (when isSubmitStep assigns primaryBtn with <Button variant="primary" onClick={() => navigate(-1)}>...) to include data-testid="wizard-close-button", and likewise add data-testid="wizard-cancel-button" to the Cancel button element (where the cancel/secondary Button is rendered, e.g., the variable/JSX that sets the cancel action). Ensure you only add the data-testid attributes and keep existing onClick/props intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@libs/ui-components/src/components/common/FlightCtlWizardFooter.tsx`:
- Around line 63-69: The Close and Cancel buttons lack test IDs; update the JSX
where primaryBtn is set for the read-only "Review" step (when isSubmitStep
assigns primaryBtn with <Button variant="primary" onClick={() =>
navigate(-1)}>...) to include data-testid="wizard-close-button", and likewise
add data-testid="wizard-cancel-button" to the Cancel button element (where the
cancel/secondary Button is rendered, e.g., the variable/JSX that sets the cancel
action). Ensure you only add the data-testid attributes and keep existing
onClick/props intact.
In `@libs/ui-components/src/components/common/ResourceLink.tsx`:
- Around line 43-49: The fallback branch in ResourceLink.tsx currently returns
<> {textEl} </> which drops the data-testid when routeLink is falsy; update the
fallback to render an element that accepts props (e.g., a span or the same
component used for link children) and pass dataTestId to it so data-testid is
present in both branches—ensure the Link branch still uses
data-testid={dataTestId} and the fallback uses the same prop on the wrapper
around textEl (reference symbols: ResourceLink.tsx, routeLink, Link, dataTestId,
textEl, id).
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
apps/standalone/src/app/components/AppLayout/AppLayout.tsxlibs/ui-components/src/components/DetailsPage/DetailsPage.tsxlibs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsxlibs/ui-components/src/components/Device/DevicesPage/DecommissionedDevicesTable.tsxlibs/ui-components/src/components/Device/DevicesPage/EnrolledDeviceTableRow.tsxlibs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsxlibs/ui-components/src/components/EnrollmentRequest/EnrollmentRequestTableRow.tsxlibs/ui-components/src/components/ListPage/ListPage.tsxlibs/ui-components/src/components/Table/Table.tsxlibs/ui-components/src/components/common/FlightCtlWizardFooter.tsxlibs/ui-components/src/components/common/ResourceLink.tsxlibs/ui-components/src/components/modals/ApproveDeviceModal/ApproveDeviceForm.tsxlibs/ui-components/src/components/modals/massModals/MassDecommissionDeviceModal/MassDecommissionDeviceModal.tsxlibs/ui-components/src/components/modals/massModals/MassDeleteDeviceModal/MassDeleteDeviceModal.tsx
✅ Files skipped from review due to trivial changes (1)
- libs/ui-components/src/components/modals/ApproveDeviceModal/ApproveDeviceForm.tsx
🚧 Files skipped from review as they are similar to previous changes (5)
- libs/ui-components/src/components/Device/DevicesPage/EnrolledDevicesTable.tsx
- libs/ui-components/src/components/modals/massModals/MassDecommissionDeviceModal/MassDecommissionDeviceModal.tsx
- libs/ui-components/src/components/Device/DevicesPage/EnrolledDeviceTableRow.tsx
- libs/ui-components/src/components/Device/DevicesPage/DecommissionedDevicesTable.tsx
- libs/ui-components/src/components/Device/DeviceDetails/DeviceDetailsPage.tsx
talhil-rh
force-pushed
the
add-device-cypress-data-testids
branch
4 times, most recently
from
1c1e5e6 to
88f6939
Compare
- AppLayout: nav-toggle on sidebar toggle - ListPage: list-page-title - Enrollment: enrollment-request-approve-button, approve-device-form-submit - EnrolledDevicesTable: enrolled-devices-table, toolbar-decommission-devices, show-decommissioned-devices-switch - EnrolledDeviceTableRow: enrolled-device-row, device-name-link, device-row-actions - ResourceLink: optional data-testid prop (including fallback when routeLink is missing) - FlightCtlWizardFooter: wizard-next-button, wizard-save-button, wizard-back-button, wizard-close-button, wizard-cancel-button - DetailsPage: optional titleDataTestId (device-details-title from DeviceDetailsPage) - MassDecommissionDeviceModal: modal-decommission-confirm - MassDeleteDeviceModal: modal-delete-devices-confirm - DecommissionedDevicesTable: toolbar-delete-forever, show-decommissioned-devices-switch - Table: data-testid prop support Addresses CodeRabbit nitpicks for ResourceLink fallback and wizard buttons. Made-with: Cursor
talhil-rh
force-pushed
the
add-device-cypress-data-testids
branch
from
88f6939 to
94fba6c
Compare
talhil-rh
changed the title
celdrake
reviewed
Mar 5, 2026
|
|
||
| return ( | ||
| <Tr> | ||
| <Tr data-testid="enrolled-device-row"> |
Collaborator
There was a problem hiding this comment.
I can't remember if it's preferable to have unique data-testid
In this case, all enrolled devices rows would have the same ID and we'd need to target them by position. Is that OK?
| id={deviceName} | ||
| name={deviceAlias || t('Untitled')} | ||
| routeLink={ROUTE.DEVICE_DETAILS} | ||
| data-testid="device-name-link" |
Collaborator
There was a problem hiding this comment.
Same question here, is it OK to share the same ID for multiple row elements?
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Made-with: Cursor
Summary by CodeRabbit