Skip to content

Fixes #39473 - Replace enzyme test in HostStatuses/Status with RTL - #11058

Closed
kfamilonidis wants to merge 1 commit into
theforeman:developfrom
kfamilonidis:pf5-status
Closed

Fixes #39473 - Replace enzyme test in HostStatuses/Status with RTL#11058
kfamilonidis wants to merge 1 commit into
theforeman:developfrom
kfamilonidis:pf5-status

Conversation

@kfamilonidis

Copy link
Copy Markdown
Contributor

Fixes #39473: Replace enzyme test in HostStatuses/Status with RTL

@github-actions github-actions Bot added the UI label Jun 30, 2026
@kfamilonidis
kfamilonidis force-pushed the pf5-status branch 3 times, most recently from 8c24272 to 6c5587a Compare July 1, 2026 11:33
@Lukshio

Lukshio commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Good migration from a brittle full-render snapshot to explicit RTL assertions on rendered text and link hrefs — this is a real improvement in signal over the old snapshot (which would break on any unrelated markup/prop change).

Suggestions (should fix)

  • Missing "Owned" link assertions: the old snapshot covered LinkOrLabel for both Total: and Owned: counts (each with its own href, e.g. owner = current_user AND (status = warn). The new test only asserts href for the Total: links via getByRole('link', ...). Since Owned: counts are also rendered as links (per the snapshot), please add the same toHaveAttribute('href', ...) assertions for the Owned: links so that search-query parity isn't silently lost in a future change.
  • No coverage of the expand/collapse behavior: Status renders inside a PF Card/CardHeader with isExpanded/onExpand and a CardExpandableContent containing the Details breakdown (visible in the removed snapshot). The new RTL test only checks the collapsed-state text/links and drops coverage of expanding the card and verifying the Details table shows the OK/Warning/Error breakdown. Consider adding a userEvent.click on the expand toggle (aria-label: 'Details' per the old snapshot) and asserting the expanded content appears — this was actual behavior under test before and is now untested.

Nice to have (optional)

  • Consider using within(screen.getByRole('link', { name: 'Total: 3' })) style or screen.getAllByRole('link') with array assertions to reduce repetition across the three statuses, but the current explicit form is also fine and readable.

Checklist gaps

  • Commit message uses Fixes #39473: Replace enzyme test... — Foreman convention is a hyphen, not a colon: Fixes #39473 - Replace enzyme test in HostStatuses/Status with RTL.
  • No interaction test for the expandable Details section that existed implicitly in the snapshot.

@kfamilonidis

Copy link
Copy Markdown
Contributor Author

@Lukshio I added toHaveAttribute('href', …) assertions for all three Owned: links.
I added an expands to reveal the status breakdown table test that exercises the interaction the old snapshot only captured structurally.

@kfamilonidis kfamilonidis changed the title Fixes #39473: Replace enzyme test in HostStatuses/Status with RTL Fixes #39473 - Replace enzyme test in HostStatuses/Status with RTL Jul 15, 2026

describe('Status', () => {
testComponentSnapshotsWithFixtures(Status, fixtures);
it('renders Status', () => {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as the other PR, a split into multiple it statements would be clearer

import { store } from '../HostStatuses.fixtures.js'
import { testComponentSnapshotsWithFixtures } from '../../../common/testHelpers';

jest.mock('react-redux', () => ({

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, I know you didn't make this change, but the test contains some mocking issue:

Image

This needs to be:

selectHostStatusOwnedPaths: jest.fn().mockReturnValue({
  okOwnedPath: get(status, 'ok_owned_path'),
  warnOwnedPath: get(status, 'warn_owned_path'),
  errorOwnedPath: get(status, 'error_owned_path'),
}),

And then you need to fix the tests in the function

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed.

@andreilakatos

Copy link
Copy Markdown
Contributor

@Lukshio Still using getByText, but other than that this should be good to go

@andreilakatos

Copy link
Copy Markdown
Contributor

@Lukshio @kfamilonidis
Should be closed, created this PR that was merged for it #11216

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants