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

Invalid Test Case Results from Enzyme's.find() - Returns inconsistent Results for <DatePicker /> component in exclude_times_test.test.js #4317

Closed
balajis-qb opened this issue Oct 14, 2023 · 4 comments · Fixed by qburst/react-datepicker-3#4 or #4318
Assignees

Comments

@balajis-qb
Copy link

Problem
In the exclude_times_test.test.js file, we have encountered an issue when using Enzyme's .find() method in conjunction with mount() to render a <DatePicker /> component and querying elements by class. The .find() method is returning an empty wrapper even when the specified class does not exist, which is leading to inconsistencies in our test results and making it challenging to determine the validity of our test cases.

image

In the above screenshot, the test case should fail, as the <DatePicker /> is not opened or inline, it won't render any date or time, hence the test case would fail obviously. But as we're expecting expect(datePicker.find('xxx')).not.toBeNull() the test case is getting ignored.

So, in this file, I found 2 issues

  1. <DatePicker /> is not opened, hence the test case fails
  2. Using .not.toBeNull instead of checking specific assertion like length check

I also found one another important bug, that is related to the use of setTime helper while setting excludeTimes prop
image

If you notice the above screenshot, we're passing invalid argument to the setTime helper. For example, instead of hour, we are passing hours and instead of minute we're passing minutes.

Proposed Solution

  • Fix the setTime call
  • Open the <DatePicker /> component to test it, by passing the open prop
  • Use .toHaveLength(4) instead of using .not.toBeNull
@balajis-qb
Copy link
Author

@martijnrusschen , can you please assign this issue to me. I'll work on it.

@martijnrusschen
Copy link
Member

Done

@martijnrusschen
Copy link
Member

We'd like to remove enzyme from the test suite so if you're rewriting tests let's try avoiding more enzyme usage.

balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this issue Oct 14, 2023
In the exclude_times_test.js file, the following modifications were made to improve the test case:

1. Added the 'open' prop to the <DatePicker /> component to ensure that it is open and its children can be tested.

2. Corrected the call to 'setTime' by providing the correct parameter format, { hour: x, minute: x }, instead of { hours: x, minutes: x }.

3. Replaced the assertion using '.not.toBeNull()' with '.toHaveLength(4)' when using '.find()'. This change makes the assertion more specific and reliable, ensuring that the test case passes only when it should.

These updates enhance the accuracy and reliability of the test case in the exclude_times_test.js file.

Closes Hacker0x01#4317
balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this issue Oct 14, 2023
This commit updates the test case in exclude_times_test.js from using Enzyme to using @testing-library/react as requested by @martijnrusschen on [the issue Hacker0x01#4317](Hacker0x01#4317 (comment))
@balajis-qb
Copy link
Author

balajis-qb commented Oct 14, 2023

Thank you for assigning the issue to me @martijnrusschen. As per your request, I fixed the test cases and also migrated the file to use react-testing-library instead of enzyme. Kindly review my PR and let me know if any changes required.

#4318

martijnrusschen added a commit that referenced this issue Oct 14, 2023
…iour

Fix #4317: Fix Enzyme Test file in exclude_times_test.js and migrate it to use @testing-library/react
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment