Skip to content

Fixes #39467 - Replace enzyme test in ConfigReports with RTL#11050

Open
kfamilonidis wants to merge 1 commit into
theforeman:developfrom
kfamilonidis:pf5-config-reports
Open

Fixes #39467 - Replace enzyme test in ConfigReports with RTL#11050
kfamilonidis wants to merge 1 commit into
theforeman:developfrom
kfamilonidis:pf5-config-reports

Conversation

@kfamilonidis

Copy link
Copy Markdown
Contributor

Fixes #39467: Replace enzyme test in ConfigReports with RTL

@github-actions github-actions Bot added the UI label Jun 29, 2026
@kfamilonidis
kfamilonidis force-pushed the pf5-config-reports branch 4 times, most recently from 8c0c716 to d03ed6b Compare June 30, 2026 11:09
@Lukshio

Lukshio commented Jul 14, 2026

Copy link
Copy Markdown
Contributor

Summary

Solid migration from an enzyme shallow snapshot to RTL assertions against real rendered output (including charts). The new test actually exercises rendering (previously only shallow, so ChartBox internals and SVGs were never rendered at all) — that's a meaningful increase in coverage, not just a like-for-like swap.

Suggestions (should fix)

  • Chart data values are no longer verified: the old snapshot asserted the exact data passed into each ChartBox ([["runtime", 5]] for metrics, [["applied", 2]] for status). The new test only checks that an svg exists inside each chart box, not that the right values/labels were rendered. If the chart legend/labels render as text (e.g. via title/aria-label on the SVG or accessible chart legend), consider asserting on that text so a regression in data-wiring (e.g. wrong field mapped to the chart) wouldn't slip through silently.
  • Heavy reliance on implementation CSS classes: assertions like .chart-box.report-chart, .bar-chart-medium-width, table.report-chart couple the test tightly to current markup/class names rather than user-facing behavior. This is somewhat unavoidable for chart-only components, but note that any CSS/PF5 class rename later on (e.g. if ChartBox itself gets migrated) will break this test even though the feature still works — worth a comment in the test noting why the class-based checks exist, or scoping them down to only what's needed to disambiguate the two chart boxes.

Nice to have (optional)

  • screen.getAllByText('10') asserting length 2 is a little implicit about why there are two matches (table cell + footer total) — the inline comment already explains this, which is good; consider also using within(table) to make the assertion more explicit about which 10 belongs to which row/footer if this ever needs a third occurrence added.

Checklist gaps

  • Commit message uses Fixes #39467: Replace enzyme test... — Foreman convention is a hyphen, not a colon: Fixes #39467 - Replace enzyme test in ConfigReports with RTL.

@kfamilonidis
kfamilonidis force-pushed the pf5-config-reports branch 2 times, most recently from bb23810 to 572fa3c Compare July 14, 2026 13:48
@kfamilonidis

kfamilonidis commented Jul 14, 2026

Copy link
Copy Markdown
Contributor Author

@Lukshio I added a NOTE comment above the class-based assertions explaining why they're coupled to CSS classes. Added, scoped to each chart's own card:

  • metricsChart has runtime and not applied
  • statusChart has applied and not runtime.

Made each 10 explicit about which row/footer it belongs to and won't break if a third 10 is added elsewhere.

@kfamilonidis kfamilonidis changed the title Fixes #39467: Replace enzyme test in ConfigReports with RTL Fixes #39467 - Replace enzyme test in ConfigReports with RTL Jul 15, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants