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

Enhancement: Improve Test cases Maintainability and Remove Magic Numbers on test/exclude_times_test.test.js, test/filter_times_test.test.js and test/include_times_test.test.js #4334

Closed
balajis-qb opened this issue Oct 21, 2023 · 3 comments · Fixed by qburst/react-datepicker-3#6, #4335 or jones58/personal-knowledge-wiki#29 · May be fixed by rowidanagah/EduMentorMate#9
Assignees

Comments

@balajis-qb
Copy link

Description
This issue is to propose an enhancement to the project by improving the maintainability of certain test cases and removing magic numbers from the test cases which will be helpful for the developers to easily understand the codebase.

Motivation and Benefits

  • Enhancing test case maintainability will lead to more readable and understandable code.
  • Removing magic numbers will make the code more robust and easier to maintain

Related Files/Code

  • test/exclude_times_test.test.js
  • test/filter_times_test.test.js
  • test/include_times_test.test.js

image

image

@martijnrusschen
Copy link
Member

Sounds good

@balajis-qb
Copy link
Author

Hi @martijnrusschen ,

I created this issue based on the review comments I got on one of my previous PRs (#4319 (comment)). Can you assign this issue to me? I'll update the test cases for the mentioned files

balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this issue Oct 21, 2023
In this commit, I've mad the following enhancements to the code:
1. Split the test-cases and move the test-case for checking the aria-disabled attribute to a separate it block.
2. Changed the use of the magic number 4 to the length of the input array excludeTimes for better flexibility and maintainability.

Closes Hacker0x01#4334
balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this issue Oct 21, 2023
In this commit, I've made the following enhancements to the code:
1. Replaced the magic value 17 with meaning constant for clarity.
2. Instead of directly checking the disabledTimeItems.length to be 2, make sure whether it's really the value we assigned (17 or 5pm).
3. Split the test-cases and move the test-case for checking the aria-disabled attribute to a separate it block.

Closes Hacker0x01#4334
balajis-qb pushed a commit to qburst/react-datepicker-3 that referenced this issue Oct 21, 2023
In this commit, I've made the following enhancements to the code:
1. Change the test case of finding the disabled time units count to the finding the enabled time units as the module involves on the existence of the includeTimes
2. Moved the test case to find the enabled time units through aria-disabled attribute to the separate it block
3. Replaced the magic value 45 with the actual computation which resolves to the magic value, to make it more maintainable and readable

Closes Hacker0x01#4334
@balajis-qb
Copy link
Author

@martijnrusschen, Thank you for assigning this refactor ticket to me. I created a PR for this - #4335. Kindly review it and let me know if any changes required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment