chore: Updating tests to the masking to ensure date formats are covered #3530
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3530 +/- ##
==========================================
+ Coverage 96.62% 96.65% +0.03%
==========================================
Files 817 818 +1
Lines 23794 23811 +17
Branches 8343 8278 -65
==========================================
+ Hits 22991 23015 +24
+ Misses 796 742 -54
- Partials 7 54 +47 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| }); | ||
| }); | ||
| describe('getValidValue', () => { | ||
| mask.segments.forEach((segment, segmentIndex) => { |
There was a problem hiding this comment.
This is not the easiest code to read, but I couldn't think of a better way that encompasses a test case for each segment. It looks ugly here, but in the test breakdown it all makes sense
There was a problem hiding this comment.
I think overall the test logic here is too complex to be valuable and maintainable in the long term: we almost need tests for the tests... Instead I would suggest trying to create some real-world examples and using those, as that will be much easier to understand and maintain over time.
There was a problem hiding this comment.
(if I was adding new functionality, I would have no idea where to start within this test logic to add new tests)
There was a problem hiding this comment.
Thanks for the confirmation. Yea we might even need tests for the tests for the tests... I will do more like I've done below.
There was a problem hiding this comment.
I kept all tests but made setup declarative, removing all iterators. @gethinwebster please take another look
dpitcock
force-pushed
the
denpitco-masking-tests
branch
from
5c7ab39 to
e497786
Compare
| const minuteMask = { min: 0, max: 59, length: 2 }; | ||
| const secondMask = { min: 0, max: 59, length: 2 }; | ||
| function createTimeMask(separator: ':' | '-', segmentsCount?: 2 | 3) { | ||
| segmentsCount = segmentsCount ?? (Math.random() > 0.5 ? 2 : 3); |
There was a problem hiding this comment.
why is there randomness here?
There was a problem hiding this comment.
That is because for tests that assert the first 1-2 segments, it should not matter whether the mask supports 2 or 3 segments.
| @@ -60,33 +62,33 @@ class MaskFormat { | |||
| return inputSegments.every((segmentValue, i) => { | |||
| const segment = this.segments[i]; | |||
|
|
|||
| // disallow empty segments | |||
| // Disallow empty segments | |||
There was a problem hiding this comment.
suggest removing these writing changes, I'm not sure they serve any real purpose and just end up making the diff/history more difficult to follow
There was a problem hiding this comment.
I would keep these as the file also includes a couple of real changes - adding a new comment, and removing an unnecessary one. But I noted your feedback for the future!
…ed (#3530) Co-authored-by: Andrei Zhaleznichenka <[email protected]>
…ed (cloudscape-design#3530) Co-authored-by: Andrei Zhaleznichenka <[email protected]>
Updating tests to the masking utils
This pull request adds more comprehensive tests for the useMask hook in the masked-input component. The new tests cover the following scenarios:
Date Masks
Time Masks
General Improvements
These changes ensure that the useMask hook can handle a wider range of date and time formats, and that the masking functionality is thoroughly tested across various use cases.
Related links, issue #, if available: n/a
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.