WIP - SAT-38755 - Convert RH Cloud Host Details snapshots to RTL #1125
Conversation
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
Reviewer's GuideThis PR standardizes tests by replacing snapshot-driven patterns with explicit React Testing Library and Jest assertions across helpers, actions, reducers, selectors, and components, and removes obsolete snapshot files. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- You can move the
import '@testing-library/jest-dom'into your Jest setup file to remove the boilerplate in every test. - Consider standardizing your describe/it naming conventions (e.g. module vs. helper names) across all test files for better consistency.
- In your async action error test, you might also assert that the failure action (INSIGHTS_HITS_FAILURE) is not dispatched, to more fully cover the error path.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- You can move the `import '@testing-library/jest-dom'` into your Jest setup file to remove the boilerplate in every test.
- Consider standardizing your describe/it naming conventions (e.g. module vs. helper names) across all test files for better consistency.
- In your async action error test, you might also assert that the failure action (INSIGHTS_HITS_FAILURE) is *not* dispatched, to more fully cover the error path.
## Individual Comments
### Comment 1
<location> `webpack/InsightsHostDetailsTab/__tests__/InsightsTabReducer.test.js:33-35` </location>
<code_context>
+ expect(newState.hits).toEqual([]);
+ });
+
+ it('should handle INSIGHTS_HITS_SUCCESS', () => {
+ const initialState = Immutable({ hits: [] });
+ const action = {
type: INSIGHTS_HITS_SUCCESS,
payload: { hits },
- },
- },
-};
-
-describe('AccountList reducer', () =>
- testReducerSnapshotWithFixtures(reducer, fixtures));
+ };
+ const newState = reducer(initialState, action);
+ expect(newState.hits).toEqual(hits);
+ });
+});
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for handling unexpected payloads or missing hits in INSIGHTS_HITS_SUCCESS.
Please add a test where INSIGHTS_HITS_SUCCESS is dispatched with a payload missing the 'hits' property or with 'hits' as undefined/null to verify the reducer's behavior in these scenarios.
```suggestion
expect(newState.hits).toEqual(hits);
});
it('should handle INSIGHTS_HITS_SUCCESS with missing hits property', () => {
const initialState = Immutable({ hits: [] });
const action = {
type: INSIGHTS_HITS_SUCCESS,
payload: {},
};
const newState = reducer(initialState, action);
expect(newState).toHaveProperty('hits');
expect(newState.hits).toEqual([]);
});
it('should handle INSIGHTS_HITS_SUCCESS with hits as undefined', () => {
const initialState = Immutable({ hits: [] });
const action = {
type: INSIGHTS_HITS_SUCCESS,
payload: { hits: undefined },
};
const newState = reducer(initialState, action);
expect(newState).toHaveProperty('hits');
expect(newState.hits).toEqual([]);
});
it('should handle INSIGHTS_HITS_SUCCESS with hits as null', () => {
const initialState = Immutable({ hits: [] });
const action = {
type: INSIGHTS_HITS_SUCCESS,
payload: { hits: null },
};
const newState = reducer(initialState, action);
expect(newState).toHaveProperty('hits');
expect(newState.hits).toEqual([]);
});
});
```
</issue_to_address>
### Comment 2
<location> `webpack/common/Switcher/__tests__/SwitcherPF4.test.js:7-16` </location>
<code_context>
- describe('rendering', () =>
- testComponentSnapshotsWithFixtures(InsightsTab, fixtures));
+ describe('rendering', () => {
+ it('should render with props', () => {
+ render(<InsightsTab {...props} />);
+ expect(screen.getByText('Recommendations')).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for the onChange callback to verify user interaction.
Please add a test that simulates user interaction and verifies the onChange callback is triggered.
</issue_to_address>
### Comment 3
<location> `webpack/common/Switcher/__tests__/HelpLabel.test.js:7-16` </location>
<code_context>
- describe('rendering', () =>
- testComponentSnapshotsWithFixtures(InsightsTab, fixtures));
+ describe('rendering', () => {
+ it('should render with props', () => {
+ render(<InsightsTab {...props} />);
+ expect(screen.getByText('Recommendations')).toBeInTheDocument();
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for accessibility attributes and user interaction.
Please add a test to check that the HelpLabel button includes accessibility attributes and handles user interactions like clicks.
</issue_to_address>
### Comment 4
<location> `webpack/InsightsHostDetailsTab/__tests__/InsightsTab.test.js:10-12` </location>
<code_context>
- describe('rendering', () =>
- testComponentSnapshotsWithFixtures(InsightsTab, fixtures));
+ describe('rendering', () => {
+ it('should render with props', () => {
+ render(<InsightsTab {...props} />);
+ expect(screen.getByText('Recommendations')).toBeInTheDocument();
+ });
+ });
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for edge cases such as missing or partial props.
Please add tests for scenarios where props are missing or set to undefined/null to verify the component handles these cases without errors.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
parthaa
changed the title
ShimShtein
reviewed
Nov 10, 2025
| import { noop } from 'foremanReact/common/helpers'; | ||
| import InsightsCloudSync from './InsightsCloudSync'; | ||
|
|
||
| // Mock webpack share scopes for module federation |
Member
There was a problem hiding this comment.
Probably, why was it needed for the test?
ShimShtein
reviewed
Nov 10, 2025
| vulnerabilityDisabled({ | ||
| describe('ForemanRhCloud helpers', () => { | ||
| it('should return foreman Url', () => { | ||
| expect(foremanUrl('/test_path')).toBe('MY_TEST_URL_PREFIX.example.com/test_path'); |
Member
There was a problem hiding this comment.
Do we have a core function instead of creating our own one?
| selectForemanInventoryUpload(state), | ||
| 'should return InsightsCloudSync': () => selectInsightsCloudSync(state), | ||
| }; | ||
| describe('ForemanRhCloud selectors', () => { |
Member
There was a problem hiding this comment.
I would argue that if the selector is trivial, we may skip this test entirely and make sure we're testing the components that use those selectors
| const fixtures = { | ||
| 'should return rhCloud wrapper': () => | ||
| rhCloudStateWrapper({ inventoryChild: {} }, { insightsChild: {} }), | ||
| describe('ForemanRhCloud helpers', () => { |
Member
There was a problem hiding this comment.
This file feels a bit meta-testing. We're testing the test helpers 🤔
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
This relates to converting the following tests to RTL using AI :)
Summary by Sourcery
Convert RH Cloud Host Details and related test suites from snapshot-based testing to explicit Jest and React Testing Library tests, removing legacy snapshot files
Enhancements:
Tests:
Chores: