-
Notifications
You must be signed in to change notification settings - Fork 6
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
ERM-3395, Complete absent test cases for stripes-erm-components document filter #709
Conversation
License CLA Stuck? (Developer should make sure that it is really stuck before clicking) |
Jest Unit Test Results 1 files ± 0 42 suites ±0 1m 20s ⏱️ +5s Results for commit 92815e4. ± Comparison against base commit 241dc35. This pull request removes 24 and adds 45 tests. Note that renamed tests count towards both.
♻️ This comment has been updated with latest results. |
@@ -139,7 +126,4 @@ describe('DocumentFilter', () => { | |||
}); | |||
}); | |||
}); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This todo hasn't been done yet, so shouldn't be removed
if (expectedFields > 0) { | ||
test('renders the add rule button', async () => { | ||
await Button('Add rule').exists(); | ||
it('renders DocumentFilterRule component with the correct number of times by clicking the "Add rule" button', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a little overcomplicated. The big if/else isn't needed here. We can instead test:
- Does the right number of DocumentFilterRule exist? (Use queryAllByText so the 0 case works)
- Does add rule exist
- Describe clicking add rule
- Has number of DocumentFilterRules gone up by one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general the point of the .each
at the top is so that this kind of case by case logic isn't needed inside the tests themselves. Let me know if you wan tme to walk through this case with you cos it's an interesting shift in test writing
await Button('Add filter').exists(); | ||
await waitFor(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don't do this in a test.
Describe is for actions, test is for outcomes
@@ -51,52 +54,65 @@ describe('DocumentFilterRule', () => { | |||
await Button('Submit').exists(); | |||
}); | |||
|
|||
it('renders Name option', () => { | |||
test('renders the correct options for the Attribute dropdown', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I separated these deliberately... I want each test to be a single granular check. If this fails we don't know which field broke.
}); | ||
|
||
it('renders is option', () => { | ||
test('renders the correct options for the Comparator dropdown', async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing as above though... pleasee keep individual checks in their own test/it
Make use of new testSelect feature in stripes-erm-testing (Bump major version of stripes-erm-testing)
Tweaked test case for DocumentFilterRule to align with describe-action, test-outcome paradigm ERM-3395
Quality Gate passedIssues Measures |
No description provided.