fix: clear errors from invalid group email csv upload #1428
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1428 +/- ##
==========================================
+ Coverage 86.46% 86.48% +0.01%
==========================================
Files 660 660
Lines 14921 14936 +15
Branches 3159 3106 -53
==========================================
+ Hits 12902 12917 +15
- Misses 1949 1952 +3
+ Partials 70 67 -3 ☔ View full report in Codecov by Sentry. |
| expect(screen.getByText('bademails.csv')).toBeInTheDocument(); | ||
| expect(screen.getByText("Members can't be invited as entered.")).toBeInTheDocument(); | ||
| expect(screen.getByText('iamnotanemail is not a valid email.')).toBeInTheDocument(); | ||
| }, { timeout: EMAIL_ADDRESSES_INPUT_VALUE_DEBOUNCE_DELAY + 1000 }); |
There was a problem hiding this comment.
Can you try if this works as well? We want to run the tests as quickly as possible
| }, { timeout: EMAIL_ADDRESSES_INPUT_VALUE_DEBOUNCE_DELAY + 1000 }); | |
| }, { timeout: EMAIL_ADDRESSES_INPUT_VALUE_DEBOUNCE_DELAY + 20 }); |
There was a problem hiding this comment.
While it worked (as in the test didn't fail when I ran it), we should be clear that the timeout value here is how long the test will look for the awaited conditions before failing, not how long it will wait in the case of a successful test. In a success case, the test will proceed as soon as the conditions are met.
As such, reducing the timeout value would only give us faster failures in the best case, but noisy tests that fail at random in the worst case, which is something I'd like to avoid.
There was a problem hiding this comment.
@marlonkeating thanks, very good point. That's something I hadn't considered!
| expect(screen.getByText('goodemails.csv')).toBeInTheDocument(); | ||
| expect(screen.getByText('Summary (1)')).toBeInTheDocument(); | ||
| expect(screen.getAllByText('[email protected]')).toHaveLength(2); | ||
| }, { timeout: EMAIL_ADDRESSES_INPUT_VALUE_DEBOUNCE_DELAY + 1000 }); |
There was a problem hiding this comment.
Could you try
| }, { timeout: EMAIL_ADDRESSES_INPUT_VALUE_DEBOUNCE_DELAY + 1000 }); | |
| }, { timeout: EMAIL_ADDRESSES_INPUT_VALUE_DEBOUNCE_DELAY + 20 }); |
| @@ -348,6 +348,40 @@ describe('<CreateGroupModal />', () => { | |||
| expect(screen.getAllByText('[email protected]')).toHaveLength(2); | |||
| }, { timeout: EMAIL_ADDRESSES_INPUT_VALUE_DEBOUNCE_DELAY + 1000 }); | |||
| }); | |||
| it('uploads csv file with bad email, re-uploads csv file with good email', async () => { | |||
There was a problem hiding this comment.
This test name is a good description of what the test does. Maybe we could improve it further by describing what the test expects? Something like should show an error message after csv upload with invalid emails, and not after re-upload with valid emails.
You may be interested in this ADR I wrote in another repo explaining why and how to write good test names. Incidentally, I intend to turn this into an OEP soon.
There was a problem hiding this comment.
Thanks, there were some good suggestions in that ADR. I edited the names of the new tests in this PR to be more in line with your recommendations.
test: better test names
marlonkeating
force-pushed
the
mkeating/ENT-10024
branch
from
dd51621 to
3ccddc2
Compare
Jira Ticket
This change makes it so that if an invalid email csv file is uploaded, the error will clear once a valid csv file is uploaded.
Testing Instructions
For all changes
Only if submitting a visual change