Fixes #39492 - Replace enzyme test in SettingsTable, SettingsCell, SettingsName with RTL by kfamilonidis · Pull Request #11084 · theforeman/foreman

kfamilonidis · 2026-07-02T13:17:22Z

Fixes #39492: Replace enzyme test in SettingsTable, SettingsCell, SettingsName with RTL

Lukshio · 2026-07-08T09:35:26Z

@kfamilonidis please add more testing cases

kfamilonidis · 2026-07-08T11:23:32Z

Added 4 more tests that check the settings table values (instead of row headers that was before)

Lukshio · 2026-07-14T10:42:57Z

Summary

Good migration across SettingsTable, SettingCell, and SettingName. The author already responded to the earlier "please add more testing cases" feedback by adding 4 behavior-driven tests (cell text, cancel-exits-edit, Escape-exits-edit, select-submit). Solid overall.

Critical (must fix before merge)

None

Suggestions (should fix)

SettingsTable.test.js still builds its store with configureMockStore/mockStore (redux-mock-store). The project's testing guidelines call this out as deprecated for new tests in favor of a real store with initial-state fixtures. Since this PR already reworks the surrounding tests, consider swapping it out here (or note it as a deliberate, scoped-out follow-up).
Several assertions use raw document.querySelector('button#administrator') / [data-ouia-component-id=...] instead of RTL's screen.getByRole/getByLabelText. This matches the pre-existing style in the file, so not a regression, but worth moving to accessible queries where a role/label exists, for resilience if ids ever change.
Commit message uses Fixes #39492: ... - the documented convention (see develop history, e.g. Fixes #38653 - Fix settings show endpoint) is Fixes #XXXXX - description with a hyphen, not a colon.

Nice to have (optional)

None beyond the above.

Checklist gaps

redux-mock-store usage not addressed (see above).
Commit message hyphen convention.

kfamilonidis · 2026-07-14T14:09:02Z

@Lukshio Replaced mock store with a real redux store.
I changed in the three interaction tests with role based assertions.
Left on data-ouia-* deliberately, with a comment noting why.

andreilakatos · 2026-08-10T07:46:44Z

-)
-
 async function extracted(text = '') {
  const editButton = document.querySelector('button#http_proxy_except_list');


Please use screen.getByRole instead of querySelector. Here and on line 110-120 and 190-191, as per the guideline https://github.com/theforeman/foreman/blob/develop/developer_docs/ui-testing-guidelines.asciidoc

andreilakatos · 2026-08-10T07:47:58Z

-describe('SettingCell', () =>
-  testComponentSnapshotsWithFixtures(SettingValue, fixtures));
+// Render the tooltip content inline instead of through the Popper, so the
+// computed tooltipText can be asserted without hover/async teardown issues.


Please unmock with an afterEach here, as per https://github.com/theforeman/foreman/blob/develop/developer_docs/ui-testing-guidelines.asciidoc

andreilakatos · 2026-08-10T07:51:07Z

@@ -177,4 +166,75 @@ describe('SettingsTable', () => {
    );


On lines 100, 130, 141, 151, and 161 we have toHaveBeenCalledTimes which is incremented after every test. This approach is a bit fragile, imo. A better approach would be to use jest.clearAllMocks in between tests.

@kfamilonidis this wasn't handled. Please make this change as well

added the after each hook

Yes, but the comment was about the use of toHaveBeenCalledTimes instead of toHaveBeenCalled and adding jest.clearAllMocks. Please use toHaveBeenCalled on those.

andreilakatos · 2026-08-11T09:32:28Z

@@ -177,4 +166,75 @@ describe('SettingsTable', () => {
    );


@kfamilonidis this wasn't handled. Please make this change as well

andreilakatos · 2026-08-14T10:57:46Z

@@ -177,4 +166,75 @@ describe('SettingsTable', () => {
    );


Yes, but the comment was about the use of toHaveBeenCalledTimes instead of toHaveBeenCalled and adding jest.clearAllMocks. Please use toHaveBeenCalled on those.

…ttingsName with RTL

andreilakatos · 2026-08-17T08:27:31Z

@Lukshio Good to go

andreilakatos · 2026-08-28T08:43:58Z

@Lukshio
@kfamilonidis
This PR should be closed as well, created 3 PR's for the same thing, 2 of them were merged:
#11211
#11209
#11212

pr-processor Bot added the Not yet reviewed label Jul 2, 2026

github-actions Bot added the UI label Jul 2, 2026

kfamilonidis force-pushed the pf5-setttings-table branch from e1f49e5 to 8f88452 Compare July 2, 2026 13:53

Lukshio self-assigned this Jul 8, 2026

kfamilonidis force-pushed the pf5-setttings-table branch from 8f88452 to c75b1ae Compare July 8, 2026 11:15

kfamilonidis force-pushed the pf5-setttings-table branch 3 times, most recently from 5ff80b6 to aabfb44 Compare July 8, 2026 13:42

Lukshio mentioned this pull request Jul 14, 2026

Fixes #39477 - Replace enzyme test in ImpersonateIcon with RTL #11062

Closed

kfamilonidis force-pushed the pf5-setttings-table branch 2 times, most recently from 3b36353 to a38df2e Compare July 14, 2026 12:28

kfamilonidis force-pushed the pf5-setttings-table branch from a38df2e to a0b5b6f Compare July 14, 2026 14:09

kfamilonidis changed the title ~~Fixes #39492: Replace enzyme test in SettingsTable, SettingsCell, SettingsName with RTL~~ Fixes #39492 - Replace enzyme test in SettingsTable, SettingsCell, SettingsName with RTL Jul 15, 2026

andreilakatos suggested changes Aug 10, 2026

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Not yet reviewed labels Aug 10, 2026

kfamilonidis force-pushed the pf5-setttings-table branch from a0b5b6f to 496fe8e Compare August 11, 2026 08:38

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Aug 11, 2026

andreilakatos suggested changes Aug 11, 2026

View reviewed changes

pr-processor Bot added Waiting on contributor and removed Needs re-review labels Aug 11, 2026

kfamilonidis force-pushed the pf5-setttings-table branch from 496fe8e to b04b819 Compare August 14, 2026 09:57

pr-processor Bot added Needs re-review and removed Waiting on contributor Needs re-review labels Aug 14, 2026

andreilakatos suggested changes Aug 14, 2026

View reviewed changes

pr-processor Bot added the Waiting on contributor label Aug 14, 2026

Fixes #39492 - Replace enzyme test in SettingsTable, SettingsCell, Se…

b33407d

…ttingsName with RTL

kfamilonidis force-pushed the pf5-setttings-table branch from b04b819 to b33407d Compare August 17, 2026 08:20

pr-processor Bot added Needs re-review and removed Waiting on contributor labels Aug 17, 2026

andreilakatos approved these changes Aug 17, 2026

View reviewed changes

pr-processor Bot removed the Needs re-review label Aug 17, 2026

kfamilonidis closed this Aug 29, 2026

Uh oh!

Conversation

kfamilonidis commented Jul 2, 2026

Uh oh!

Lukshio commented Jul 8, 2026

Uh oh!

kfamilonidis commented Jul 8, 2026

Uh oh!

Lukshio commented Jul 14, 2026

Summary

Critical (must fix before merge)

Suggestions (should fix)

Nice to have (optional)

Checklist gaps

Uh oh!

kfamilonidis commented Jul 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andreilakatos commented Aug 17, 2026

Uh oh!

andreilakatos commented Aug 28, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants