Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unit Test Case for useDraggableTable #2540

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

uidoyen
Copy link
Contributor

@uidoyen uidoyen commented Feb 27, 2024

Closes: RHOAIENG-2120

Description

added unit test for frontend/src/utilities/useDraggableTable.ts

How Has This Been Tested?

npm run test

Test Impact

Screenshot 2024-04-16 at 5 16 52 PM

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

@uidoyen
Copy link
Contributor Author

uidoyen commented Feb 27, 2024

I was able to get 87.87% of branch coverage.

@christianvogt
Copy link
Contributor

There doesn't seem to be any stability checks being made for the hook return value.
It seems to me like useDraggableTable doesn't care to be stable. That's a problem. Functions like onDragEnd that have no dependencies should be wrapped in useCallback to be made stable. When writing tests, it's important to also evaluate the code being tested for improvements. Please add stability checks when testing hooks.

@uidoyen uidoyen force-pushed the RHOAIENG-2120 branch 2 times, most recently from 979a7d5 to dde3236 Compare April 16, 2024 11:45
@uidoyen
Copy link
Contributor Author

uidoyen commented Apr 16, 2024

@christianvogt updated the test case accordingly. It's up for review. Thanks!

Copy link
Contributor

@christianvogt christianvogt left a comment

Choose a reason for hiding this comment

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

You're testing drag functions individually however when used in pratice, they are called sequence. The only way to establish an internal state is to call the functions in the correct order. For example, onDragEnd will set isDragging to false. Unless you've started your test with a call to onDragStart, there's no way you can assert the class name has changed because of a change to isDragging.

I suggest you add at least one end to end test case that runs through the happy path.

frontend/src/utilities/useDraggableTable.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
frontend/src/utilities/__tests__/useDraggableTable.spec.ts Outdated Show resolved Hide resolved
@uidoyen
Copy link
Contributor Author

uidoyen commented Apr 18, 2024

@christianvogt. Thank you for the feedback and suggestion, Based on your comment, I have used a different approach and made the necessary updates.

@uidoyen
Copy link
Contributor Author

uidoyen commented Apr 18, 2024

/retest

@uidoyen uidoyen force-pushed the RHOAIENG-2120 branch 2 times, most recently from 760e7b7 to e76836d Compare April 19, 2024 14:48
@christianvogt
Copy link
Contributor

/lgtm
/approve

Copy link
Contributor

openshift-ci bot commented Apr 19, 2024

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

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

@openshift-merge-bot openshift-merge-bot bot merged commit 04f38d8 into opendatahub-io:main Apr 19, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants