Added tests for MLMD hooks #2860

Gkrumbach07 · 2024-05-29T18:30:32Z

closes: https://issues.redhat.com/browse/RHOAIENG-5510

Description

adds jest tests to all mlmd related hooks

How Has This Been Tested?

CI passes and make sure all mlmd hooks are covered

Test Impact

Adding coverage to mlmd hooks

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 or cypress tests 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

codecov · 2024-05-29T18:59:58Z

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.73%. Comparing base (2dc72ad) to head (bfb4845).
Report is 50 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2860      +/-   ##
==========================================
+ Coverage   77.49%   78.73%   +1.23%     
==========================================
  Files        1109     1101       -8     
  Lines       23394    23440      +46     
  Branches     5890     5907      +17     
==========================================
+ Hits        18129    18455     +326     
+ Misses       5265     4985     -280

Files	Coverage Δ
...src/concepts/pipelines/context/MlmdListContext.tsx	`52.63% <ø> (ø)`
...pelines/apiHooks/mlmd/useGetEventsByExecutionId.ts	`93.75% <75.00%> (+22.32%)`	⬆️
...xperiments/executions/details/ExecutionDetails.tsx	`0.00% <0.00%> (ø)`

... and 102 files with indirect coverage changes

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2dc72ad...bfb4845. Read the comment docs.

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useArtifactsFromMlmdContext.spec.ts

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetExecutionById.spec.ts

christianvogt · 2024-06-03T16:53:42Z

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetArtifactsList.spec.ts

+    mockUseMlmdListContext.mockReturnValue({
+      pageToken: '',
+      maxResultSize: 100,
+      filterQuery: '',
+    } as MlmdListContextProps);
+  });
+
+  it('should fetch and return the artifacts list', async () => {
+    mockGetArtifacts.mockResolvedValue({
+      getArtifactsList: () => mockArtifacts,
+      getNextPageToken: () => 'next-page-token',
+    } as GetArtifactsResponse);


You should assert that mockGetArtifacts gets called with the correct parameters as defined in your list context.

christianvogt · 2024-06-03T16:56:08Z

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetEventsByExecutionId.spec.ts

+    const error = new Error('Cannot fetch events');
+    mockGetEventsByExecutionIDs.mockRejectedValue(error);
+
+    const renderResult = testHook(useGetEventsByExecutionId)('1');


Odd that the param is of type string but for useGetEventsByExecutionIds it's of type number. Why the difference?

the function expects a string. i just fixed the function

Looks like there's inconsistencies in the API types. That's something you should look into as part of another PR as you start to use the APIs more.

christianvogt · 2024-06-03T16:59:19Z

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetEventsByExecutionId.spec.ts

+    expect(renderResult.result.current).toStrictEqual(
+      standardUseFetchState(mockEventsResponse, true),
+    );
+    expect(renderResult).hookToHaveUpdateCount(3);


This should be a warning that your hook is not receiving stable values. The update count jumps from 1 to 3.

The reason is because useGetEventsByExecutionId creates a new array of numbers every time it is called. This new array is used a dependency within useGetEventsByExecutionIds.

You need to use useMemo on the new array.

christianvogt · 2024-06-03T17:04:00Z

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetExecutionsList.spec.ts

+    mockUseMlmdListContext.mockReturnValue({
+      pageToken: '',
+      maxResultSize: 100,
+      filterQuery: '',
+    } as MlmdListContextProps);
+  });
+
+  it('should fetch and return the executions list', async () => {
+    mockGetExecutions.mockResolvedValue({
+      getExecutionsList: () => mockExecutions,
+      getNextPageToken: () => 'next-page-token',
+    } as GetExecutionsResponse);


Same comment as earlier. You should assert mockGetExecutions gets called with the values coming from the mlmd list context.

christianvogt · 2024-06-03T17:06:48Z

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetLinkedArtifactsByEvents.spec.ts

+  it('should return an empty array if no events have artifact IDs', async () => {
+    const renderResult = testHook(useGetLinkedArtifactsByEvents)([]);
+
+    expect(renderResult.result.current).toStrictEqual(standardUseFetchState([]));
+    expect(renderResult).hookToHaveUpdateCount(1);
+  });


Need to wait for an update before you can assert the final value when using useFetchState

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useGetMlmdContextType.spec.ts

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useMlmdContext.spec.ts

christianvogt · 2024-06-03T17:14:57Z

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/useMlmdContext.spec.ts

+  });
+
+  it('should handle errors from getMlmdContext', async () => {
+    const error = new Error('Cannot find specified context');


You should use a separate error message here just to not confuse with the same error that's defined in useMlmdContext. Which also doesn't have a test case.

christianvogt · 2024-06-03T17:20:09Z

frontend/src/concepts/pipelines/apiHooks/mlmd/__tests__/usePipelineRunMlmdContext.spec.ts

+  useMlmdContext: jest.fn(),
+}));
+
+describe('usePipelineRunMlmdContext', () => {


Considering usePipelineRunMlmdContext is just a wrapper on top of useMlmdContext with the type set to MlmdContextTypes.RUN, these test cases aren't very useful. In fact they stood out to me as being odd because they don't require waiting for an update since you've mocked the one call made to useMlmdContext. Essentially all the tests do is test your mock.

i will remove it

Refactor MlmdListContextProps to be exported fixes

Gkrumbach07 · 2024-06-04T19:42:25Z

@christianvogt addressed all comments

christianvogt · 2024-06-05T19:24:23Z

/lgtm
/approve

openshift-ci · 2024-06-05T19:24:34Z

[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:

~~frontend/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 christianvogt and pnaik1 May 29, 2024 18:32

christianvogt reviewed May 29, 2024

View reviewed changes

Gkrumbach07 force-pushed the task/RHOAIENG-5510-add-test-coverage-for-pipeline-mlmd-hooks branch from 5b8bd4d to 3954ecd Compare May 31, 2024 18:31

Gkrumbach07 requested a review from christianvogt May 31, 2024 18:31

christianvogt reviewed Jun 3, 2024

View reviewed changes

Added tests for MLMD hooks

bfb4845

Refactor MlmdListContextProps to be exported fixes

Gkrumbach07 force-pushed the task/RHOAIENG-5510-add-test-coverage-for-pipeline-mlmd-hooks branch from 3954ecd to bfb4845 Compare June 4, 2024 19:41

openshift-ci bot assigned christianvogt Jun 5, 2024

openshift-ci bot added the lgtm label Jun 5, 2024

openshift-ci bot added the approved label Jun 5, 2024

openshift-merge-bot bot merged commit ac0fc61 into opendatahub-io:main Jun 5, 2024
8 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added tests for MLMD hooks #2860

Added tests for MLMD hooks #2860

Gkrumbach07 commented May 29, 2024

codecov bot commented May 29, 2024 •

edited

Loading

christianvogt Jun 3, 2024

christianvogt Jun 3, 2024

Gkrumbach07 Jun 4, 2024

christianvogt Jun 5, 2024

christianvogt Jun 3, 2024

christianvogt Jun 3, 2024

christianvogt Jun 3, 2024

christianvogt Jun 3, 2024

christianvogt Jun 3, 2024

Gkrumbach07 Jun 4, 2024

Gkrumbach07 commented Jun 4, 2024

christianvogt commented Jun 5, 2024

openshift-ci bot commented Jun 5, 2024

Added tests for MLMD hooks #2860

Added tests for MLMD hooks #2860

Conversation

Gkrumbach07 commented May 29, 2024

Description

How Has This Been Tested?

Test Impact

Request review criteria:

codecov bot commented May 29, 2024 • edited Loading

Codecov Report

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

Choose a reason for hiding this comment

Gkrumbach07 commented Jun 4, 2024

christianvogt commented Jun 5, 2024

openshift-ci bot commented Jun 5, 2024

codecov bot commented May 29, 2024 •

edited

Loading