Skip to content

SAT-48347 - add test_setup that imports foremanJSTestSetup and remove overlapping mocks - #1241

Open
andreilakatos wants to merge 1 commit into
theforeman:developfrom
andreilakatos:48347-add-test-setup-and-remove-overlapping-mocks
Open

SAT-48347 - add test_setup that imports foremanJSTestSetup and remove overlapping mocks#1241
andreilakatos wants to merge 1 commit into
theforeman:developfrom
andreilakatos:48347-add-test-setup-and-remove-overlapping-mocks

Conversation

@andreilakatos

Copy link
Copy Markdown
Contributor

What are the changes introduced in this pull request?

Add test_setup that imports foremanJSTestSetup and remove overlapping mocks

Considerations taken when implementing this change?

Make sure that tests are still passing

What are the testing steps for this pull request?

Make sure all CI is passing and the core setup is there

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - I've found 2 issues, and left some high level feedback:

  • In SyncButton integration tests, the explicit jest.mock('foremanReact/redux/API', () => ({ get, post })) fully replaces the module and drops any other exports, which can cause subtle breakage if the test (or future changes) rely on additional API helpers; consider basing this mock on jest.requireActual and overriding only post (and get if really needed).
  • The new inline mock for foremanReact/Root/Context/ForemanContext in InsightsVulnerabilityActionsBar now hardcodes only useForemanOrganization and may diverge from the default context behavior provided by foremanJSTestSetup; it would be safer either to extend the shared mock or to only override the specific hook via jest.spyOn so future context shape changes stay aligned.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `SyncButton` integration tests, the explicit `jest.mock('foremanReact/redux/API', () => ({ get, post }))` fully replaces the module and drops any other exports, which can cause subtle breakage if the test (or future changes) rely on additional API helpers; consider basing this mock on `jest.requireActual` and overriding only `post` (and `get` if really needed).
- The new inline mock for `foremanReact/Root/Context/ForemanContext` in `InsightsVulnerabilityActionsBar` now hardcodes only `useForemanOrganization` and may diverge from the default context behavior provided by `foremanJSTestSetup`; it would be safer either to extend the shared mock or to only override the specific hook via `jest.spyOn` so future context shape changes stay aligned.

## Individual Comments

### Comment 1
<location path="webpack/InsightsCloudSync/__tests__/InsightsCloudSyncActions.test.js" line_range="20-23" />
<code_context>
-    expect(dispatched.url).toBe('/insights_cloud/tasks');
-    expect(typeof dispatched.handleSuccess).toBe('function');
-    expect(typeof dispatched.errorToast).toBe('function');
+    expect(dispatched.payload.key).toBe(INSIGHTS_CLOUD_SYNC);
+    expect(dispatched.payload.url).toBe('/insights_cloud/tasks');
+    expect(typeof dispatched.payload.handleSuccess).toBe('function');
+    expect(typeof dispatched.payload.errorToast).toBe('function');
   });

</code_context>
<issue_to_address>
**suggestion (testing):** Add an assertion for the dispatched action type to fully validate the wrapped payload structure.

Since the action is now wrapped in `payload`, these assertions cover the key/url and callbacks. To fully validate the API middleware contract, also assert the `dispatched.type` (or whatever type the middleware expects) so the test guards both the wrapper and the payload shape and catches regressions in the action envelope.

Suggested implementation:

```javascript
    expect(dispatch).toHaveBeenCalledTimes(1);
    const dispatched = dispatch.mock.calls[0][0];

    // Validate the action envelope type used by the API middleware
    expect(dispatched.type).toBe(CALL_API);

    expect(dispatch).toHaveBeenCalledTimes(1);
    const dispatched = dispatch.mock.calls[0][0];
    expect(dispatched.payload.key).toBe(INSIGHTS_CLOUD_SYNC);
    expect(dispatched.payload.url).toBe('/insights_cloud/tasks');
    expect(typeof dispatched.payload.handleSuccess).toBe('function');
    expect(typeof dispatched.payload.errorToast).toBe('function');

```

1. Ensure the test imports the correct middleware type constant (e.g. `CALL_API`) at the top of `InsightsCloudSyncActions.test.js`, matching whatever the real action creator uses:
   - For example: `import { CALL_API } from 'redux-api-middleware';` or your local middleware module.
2. If your middleware uses a different type name (e.g. `API`, `API_REQUEST`, or a symbol), update `CALL_API` in the new assertion to the appropriate constant so the test correctly validates the dispatched action envelope.
3. If the duplicated `expect(dispatch)... const dispatched...` block in this snippet is an artifact and only appears once in the real file, apply the new `expect(dispatched.type)...` right after the single `const dispatched = ...` line instead.
</issue_to_address>

### Comment 2
<location path="webpack/ForemanInventoryUpload/Components/PageHeader/components/SyncButton/__tests__/integrations.test.js" line_range="16-18" />
<code_context>
 import { INVENTORY_SYNC } from '../SyncButtonConstants';

-jest.spyOn(API, 'post');
+jest.mock('foremanReact/redux/API', () => ({
+  get: jest.fn(payload => ({ type: 'API_GET', payload })),
+  post: jest.fn(),
+}));

 const mockStore = configureMockStore([thunk]);
</code_context>
<issue_to_address>
**suggestion (testing):** Reset the API mocks between tests to keep isolation as the suite grows.

To prevent tests from leaking mock state when more cases are added, add cleanup such as `afterEach(() => jest.clearAllMocks())` or at least `post.mockReset()` in this suite so each test starts from a clean mock configuration.

```suggestion
const mockStore = configureMockStore([thunk]);

afterEach(() => {
  jest.clearAllMocks();
});

describe('SyncButton integration test', () => {
```
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@andreilakatos
andreilakatos force-pushed the 48347-add-test-setup-and-remove-overlapping-mocks branch from 0f12156 to 63effce Compare August 3, 2026 12:35
@andreilakatos
andreilakatos force-pushed the 48347-add-test-setup-and-remove-overlapping-mocks branch from 63effce to 3fbe7e3 Compare August 3, 2026 12:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant