Unit Test Case for clusterUtils #2349

uidoyen · 2024-01-17T14:10:54Z

Closes: RHOAIENG-1897

Description

Unit Test Case for clusterUtils

How Has This Been Tested?

npm run test

Test Impact

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

frontend/src/utilities/__tests__/clusterUtils.spec.ts

uidoyen · 2024-01-23T09:51:46Z

@christianvogt updated accordingly, Can you plz look into this again?

frontend/src/utilities/__tests__/clusterUtils.spec.ts

christianvogt · 2024-01-23T19:21:26Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+afterAll(() => {
+  Object.defineProperty(window, 'location', {
+    writable: true,
+    value: originalLocation,
+  });
+});


Use afterEach instead or simply perform this in each describe block.
Also, can we simplify the assignment as well?

Suggested change

afterAll(() => {

Object.defineProperty(window, 'location', {

writable: true,

value: originalLocation,

});

});

afterEach(() => {

window.location = originalLocation

});

christianvogt · 2024-01-23T19:21:44Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+beforeAll(() => {
+  originalLocation = { ...window.location };
+});


Unnecessary. Capture the original location once at the top.

christianvogt · 2024-01-23T19:22:15Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+jest.mock('~/redux/selectors/clusterInfo', () => ({
+  ...jest.requireActual('~/redux/selectors/clusterInfo'), // Use the actual module for other exports
+  useClusterInfo: jest.fn(),
+}));


Use this method of mocking or spyOn but not both. Prefer the use of this mocking and only using spyOn for special circumstances.

christianvogt · 2024-01-23T19:24:25Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+  beforeEach(() => {
+    setupWindowLocation('localhost', 'https:', '443');
+  });


Remove and call setupWindowLocation per test so that the values used by the test is clear to anyone reading the test.

christianvogt · 2024-01-23T19:24:48Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+  afterEach(() => {
+    // Restore window.location to its original state
+    Object.defineProperty(window, 'location', {
+      writable: true,
+      value: originalLocation,
+    });
+  });


Unnecessary if you do it at the top level. See previous comment related to afterAll.

christianvogt · 2024-01-23T19:30:00Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+    const serverURL = 'https://api.example.com';
+
+    // Manually mock the useClusterInfo function
+    const mockUseClusterInfo = jest.spyOn(clusterInfo, 'useClusterInfo');


Using multiple methods of mocking when one will do. No need to use spyOn here.

christianvogt · 2024-01-23T19:30:53Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+
+    // Assert
+    expect(mockUseClusterInfo).toHaveBeenCalledTimes(1);
+    expect(result.current).toBe('https://console-openshift-console.apps.example.com');


Suggested change

expect(result.current).toBe('https://console-openshift-console.apps.example.com');

expect(result).hookToBe('https://console-openshift-console.apps.example.com');

christianvogt · 2024-01-23T20:45:18Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+const devModeMock = jest.spyOn(jest.requireMock('~/utilities/const'), 'DEV_MODE', 'get');
+
+describe('getOpenShiftConsoleServerURL', () => {
+  setupWindowLocation('localhost', 'https:', '443');


This setup is only valid until the first test case changes the value.
Remove and make the call within the specific test case with values needed for that test case.

christianvogt · 2024-01-23T20:46:07Z

frontend/src/utilities/__tests__/clusterUtils.spec.ts

+});
+
+describe('useOpenShiftURL', () => {
+  setupWindowLocation('localhost', 'https:', '443');


Move into the first test case.

christianvogt · 2024-01-23T21:35:59Z

/lgtm
/approve

openshift-ci · 2024-01-23T21:36:08Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [christianvogt]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot requested review from lucferbux and manaswinidas January 17, 2024 14:11

uidoyen requested a review from christianvogt January 17, 2024 14:14

uidoyen force-pushed the RHOAIENG-1897 branch from 56e6417 to 69434d4 Compare January 17, 2024 14:15

christianvogt reviewed Jan 17, 2024

View reviewed changes

frontend/src/utilities/__tests__/clusterUtils.spec.ts Outdated Show resolved Hide resolved

uidoyen force-pushed the RHOAIENG-1897 branch from 69434d4 to fc16e25 Compare January 22, 2024 14:39

uidoyen commented Jan 22, 2024

View reviewed changes

frontend/src/utilities/__tests__/clusterUtils.spec.ts Show resolved Hide resolved

uidoyen force-pushed the RHOAIENG-1897 branch from fc16e25 to 05b1ce4 Compare January 23, 2024 09:50

uidoyen requested a review from christianvogt January 23, 2024 09:51

uidoyen force-pushed the RHOAIENG-1897 branch from 05b1ce4 to 32bef4f Compare January 23, 2024 17:08

christianvogt reviewed Jan 23, 2024

View reviewed changes

uidoyen force-pushed the RHOAIENG-1897 branch from 32bef4f to a670d6b Compare January 23, 2024 20:36

uidoyen requested a review from christianvogt January 23, 2024 20:37

christianvogt reviewed Jan 23, 2024

View reviewed changes

uidoyen requested a review from christianvogt January 23, 2024 20:51

uidoyen force-pushed the RHOAIENG-1897 branch 2 times, most recently from 8be0330 to 64d2066 Compare January 23, 2024 20:54

Unit Test Case for clusterUtils

9287948

uidoyen force-pushed the RHOAIENG-1897 branch from 4a7b06a to 9287948 Compare January 23, 2024 21:09

openshift-ci bot assigned christianvogt Jan 23, 2024

openshift-ci bot added the lgtm label Jan 23, 2024

openshift-ci bot added the approved label Jan 23, 2024

openshift-merge-bot bot merged commit b715cdd into opendatahub-io:main Jan 23, 2024
6 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unit Test Case for clusterUtils #2349

Unit Test Case for clusterUtils #2349

uidoyen commented Jan 17, 2024 •

edited

Loading

uidoyen commented Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt commented Jan 23, 2024

openshift-ci bot commented Jan 23, 2024

	expect(result.current).toBe('https://console-openshift-console.apps.example.com');
	expect(result).hookToBe('https://console-openshift-console.apps.example.com');

Unit Test Case for clusterUtils #2349

Unit Test Case for clusterUtils #2349

Conversation

uidoyen commented Jan 17, 2024 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

uidoyen commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

christianvogt commented Jan 23, 2024

openshift-ci bot commented Jan 23, 2024

uidoyen commented Jan 17, 2024 •

edited

Loading