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

UIIN-1671 create JEST/RTL for ItemsListRow.js #2090

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

ssandupatla
Copy link
Contributor

REFS: UIIN-1671 create JEST/RTL for ItemsListRow.js
URL: https://issues.folio.org/browse/UIIN-1671

@github-actions
Copy link

github-actions bot commented Mar 29, 2023

Jest Unit Test Statistics

    1 files  ±0  167 suites  +1   4m 32s ⏱️ - 3m 4s
508 tests +2  508 ✔️ +2  0 💤 ±0  0 ±0 
512 runs  +2  512 ✔️ +2  0 💤 ±0  0 ±0 

Results for commit abd5689. ± Comparison against base commit a12a91c.

♻️ This comment has been updated with latest results.

@github-actions
Copy link

github-actions bot commented Mar 29, 2023

BigTest Unit Test Statistics

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ±0 

Results for commit abd5689. ± Comparison against base commit a12a91c.

♻️ This comment has been updated with latest results.

Copy link
Member

@zburke zburke left a comment

Choose a reason for hiding this comment

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

The two test descriptions suggest testing opposite conditions (isDragging should be true, then isDragging should be false) and use the two different mock implementations of react-beautiful-dnd. OK, that part makes sense. But then you should be checking the same condition in both places, i.e. assert that something is true in the first test, then false in the second test. Alternatively, if you can't leverage the same element for both tests, then include both expect clauses in both tests and check that their conditions flip, e.g.

it('test 1', () => {
  expect(first condition).toBeInTheDocument();
  expect(second condition).not.toBeInTheDocument();
});

it('test 2', () => {
  expect(first condition).not.toBeInTheDocument();
  expect(second condition).toBeInTheDocument();
});



describe('ItemsListRow', () => {
it('new mo should render isDragging of snapshot is true', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what "new mo" means; is this a typo? I don't understand how the description "isDragging ... is true" relates to the expect clause

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have cleared all typos. Changes made as per your suggestions. Updated test description.

Comment on lines 75 to 77
it('child should render isDragging of snapshot is false', () => {
renderItemsListRow(defaultProps);
expect(screen.getByRole('row', { name: 'cell 1cell 2cell 3' })).toBeInTheDocument();
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand how the description "isDragging ... is false" relates to the expect clause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We have cleared all typos. Changes made as per your suggestions.

@sonarcloud
Copy link

sonarcloud bot commented Apr 14, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@ssandupatla ssandupatla requested a review from zburke April 21, 2023 08:59
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.

3 participants