Unit Test Case for useDeepCompareMemoize #2373

uidoyen · 2024-01-22T20:38:47Z

Closes: RHOAIENG-2091

Description

Unit Test Case for useDeepCompareMemoize

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

christianvogt · 2024-01-22T20:55:32Z

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

+    expect(result.current).toEqual(initialData);
+
+    // Re-render with new data, expect result to change
+    rerender({ data: newData });
+    expect(result.current).toEqual(newData);
+
+    // Re-render with identical data, expect result not to change
+    rerender({ data: identicalData });
+    expect(result.current).toEqual(initialData);


Please update the tests to use toBe since identity equality is the most important aspect of this hook.

christianvogt · 2024-01-22T20:56:18Z

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

+    const { result, rerender } = renderHook(({ data }) => useDeepCompareMemoize(data), {
+      initialProps: { data: initialData },
+    });


Note that you can still use testHook to simplify the tests somewhat since you aren't needing the extra functionality that comes with renderHook.

uidoyen · 2024-01-23T09:55:09Z

@christianvogt Updated accordingly.

christianvogt · 2024-01-23T15:15:25Z

/retest

christianvogt · 2024-01-23T15:21:35Z

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

+    // Re-render with new data, expect result to change
+    expect(testHook(useDeepCompareMemoize)(newData).result.current).toBe(newData);


What you've done is not a re-render but a brand new render.
testHook returns a rerender function that should be used here.

Please see hooks.spec.ts for examples on rerender.

christianvogt · 2024-01-23T15:26:27Z

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

+
+    // Re-render with an updated value, expect the result to change
+    rerender(updatedValue);
+    expect(result.current).toBe(updatedValue);


Make use of the hook functions (see hooks.spec.ts for examples). While what you've done is correct, it's good to see at least the hookToBeStable() function being used to assert stability when we expect a stable value after a rerender.

Suggested change

expect(result.current).toBe(updatedValue);

expect(result).hookToBe(updatedValue);

expect(result).hookToBeStable();

uidoyen · 2024-01-23T17:06:55Z

@christianvogt thanks for pointing this out. Updated it accordingly.

christianvogt

Please make sure you test stability of all hook results when testing hook functions.

christianvogt · 2024-01-23T18:46:04Z

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

+    // Re-render with new data, expect result to change
+    renderResult.rerender(newData);
+    expect(renderResult).hookToBe(newData);
+    expect(renderResult).hookToHaveUpdateCount(2);
+
+    // Re-render with identical data, expect result not to change
+    renderResult.rerender(identicalData);
+    expect(renderResult).hookToStrictEqual(initialData);
+    expect(renderResult).hookToHaveUpdateCount(3);


Suggested change

// Re-render with new data, expect result to change

renderResult.rerender(newData);

expect(renderResult).hookToBe(newData);

expect(renderResult).hookToHaveUpdateCount(2);

// Re-render with identical data, expect result not to change

renderResult.rerender(identicalData);

expect(renderResult).hookToStrictEqual(initialData);

expect(renderResult).hookToHaveUpdateCount(3);

// Re-render with identical data, expect result not to change

renderResult.rerender(identicalData);

expect(renderResult).hookToBe(initialData);

expect(renderResult).hookToBeStable();

expect(renderResult).hookToHaveUpdateCount(2);

// Re-render with new data, expect result to change

renderResult.rerender(newData);

expect(renderResult).hookToBe(newData);

expect(renderResult).hookToHaveUpdateCount(3);

christianvogt · 2024-01-23T18:46:19Z

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

+    // Re-render with new data, expect result to change
+    renderResult.rerender(newNumber);
+    expect(renderResult).hookToBe(newNumber);
+    expect(renderResult).hookToHaveUpdateCount(2);
+
+    // Re-render with identical data, expect result not to change
+    renderResult.rerender(identicalNumber);
+    expect(renderResult).hookToStrictEqual(initialNumber);
+    expect(renderResult).hookToHaveUpdateCount(3);


Suggested change

// Re-render with new data, expect result to change

renderResult.rerender(newNumber);

expect(renderResult).hookToBe(newNumber);

expect(renderResult).hookToHaveUpdateCount(2);

// Re-render with identical data, expect result not to change

renderResult.rerender(identicalNumber);

expect(renderResult).hookToStrictEqual(initialNumber);

expect(renderResult).hookToHaveUpdateCount(3);

// Re-render with identical data, expect result not to change

renderResult.rerender(identicalNumber);

expect(renderResult).hookToBe(identicalNumber);

expect(renderResult).hookToBeStable();

expect(renderResult).hookToHaveUpdateCount(2);

// Re-render with new data, expect result to change

renderResult.rerender(newNumber);

expect(renderResult).hookToBe(newNumber);

expect(renderResult).hookToHaveUpdateCount(3);

christianvogt · 2024-01-23T18:46:36Z

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

+    // Re-render with new data, expect result to change
+    renderResult.rerender(newArray);
+    expect(renderResult).hookToBe(newArray);
+    expect(renderResult).hookToHaveUpdateCount(2);
+
+    // Re-render with identical data, expect result not to change
+    renderResult.rerender(identicalArray);
+    expect(renderResult).hookToStrictEqual(initialArray);
+    expect(renderResult).hookToHaveUpdateCount(3);


Suggested change

// Re-render with new data, expect result to change

renderResult.rerender(newArray);

expect(renderResult).hookToBe(newArray);

expect(renderResult).hookToHaveUpdateCount(2);

// Re-render with identical data, expect result not to change

renderResult.rerender(identicalArray);

expect(renderResult).hookToStrictEqual(initialArray);

expect(renderResult).hookToHaveUpdateCount(3);

// Re-render with identical data, expect result not to change

renderResult.rerender(identicalArray);

expect(renderResult).hookToBe(initialArray);

expect(renderResult).hookToBeStable();

expect(renderResult).hookToHaveUpdateCount(2);

// Re-render with new data, expect result to change

renderResult.rerender(newArray);

expect(renderResult).hookToBe(newArray);

expect(renderResult).hookToHaveUpdateCount(3);

christianvogt · 2024-01-23T20:47:38Z

/lgtm
/approve

openshift-ci · 2024-01-23T20:47:47Z

[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

uidoyen changed the title ~~Rhoaieng 2091~~ Unit Test Case for useDeepCompareMemoize Jan 22, 2024

uidoyen requested a review from christianvogt January 22, 2024 20:44

christianvogt reviewed Jan 22, 2024

View reviewed changes

uidoyen force-pushed the RHOAIENG-2091 branch 2 times, most recently from c7e17e6 to 8ea514c Compare January 23, 2024 09:21

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

christianvogt reviewed Jan 23, 2024

View reviewed changes

uidoyen force-pushed the RHOAIENG-2091 branch from 8ea514c to 8d1241e Compare January 23, 2024 17:05

uidoyen requested a review from christianvogt January 23, 2024 17:07

christianvogt reviewed Jan 23, 2024

View reviewed changes

uidoyen force-pushed the RHOAIENG-2091 branch from 6830aee to 252da3b Compare January 23, 2024 20:06

Unit Test Case for useDeepCompareMemoize

3307a58

uidoyen force-pushed the RHOAIENG-2091 branch from 252da3b to 3307a58 Compare January 23, 2024 20:07

uidoyen requested a review from christianvogt January 23, 2024 20: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 d8fbf39 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 useDeepCompareMemoize #2373

Unit Test Case for useDeepCompareMemoize #2373

uidoyen commented Jan 22, 2024 •

edited

christianvogt Jan 22, 2024

christianvogt Jan 22, 2024

uidoyen commented Jan 23, 2024

christianvogt commented Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

uidoyen commented Jan 23, 2024

christianvogt left a comment

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt Jan 23, 2024

christianvogt commented Jan 23, 2024

openshift-ci bot commented Jan 23, 2024

		// Re-render with new data, expect result to change
		expect(testHook(useDeepCompareMemoize)(newData).result.current).toBe(newData);

	expect(result.current).toBe(updatedValue);
	expect(result).hookToBe(updatedValue);
	expect(result).hookToBeStable();

Unit Test Case for useDeepCompareMemoize #2373

Unit Test Case for useDeepCompareMemoize #2373

Conversation

uidoyen commented Jan 22, 2024 • edited

Description

How Has This Been Tested?

Test Impact

Request review criteria:

christianvogt Jan 22, 2024

Choose a reason for hiding this comment

christianvogt Jan 22, 2024

Choose a reason for hiding this comment

uidoyen commented Jan 23, 2024

christianvogt commented Jan 23, 2024

christianvogt Jan 23, 2024

Choose a reason for hiding this comment

christianvogt Jan 23, 2024

Choose a reason for hiding this comment

uidoyen commented Jan 23, 2024

christianvogt left a comment

Choose a reason for hiding this comment

christianvogt Jan 23, 2024

Choose a reason for hiding this comment

christianvogt Jan 23, 2024

Choose a reason for hiding this comment

christianvogt Jan 23, 2024

Choose a reason for hiding this comment

christianvogt commented Jan 23, 2024

openshift-ci bot commented Jan 23, 2024

uidoyen commented Jan 22, 2024 •

edited