test: add missing test for suggestions #1215
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 183d075:
|
src/__tests__/suggestions.js
Outdated
| <button data-testid="foot">another</button>`) | ||
|
|
||
| expect(() => screen.getByTestId('foo', {suggest: true})).toThrowError() | ||
| configure({throwSuggestions: true}) |
There was a problem hiding this comment.
These setup+cleanup tasks need to be in a beforeEach/afterEach. Otherwise the whole test run would be tainted if this single test fails.
There was a problem hiding this comment.
Since this is the only test needing them, I didn't want to add the overhead of running that code in a 'beforeEach' and 'afterEach' so I changed it at the beginning and returned it to what the 'beforeAll' hook defined. Do you think it will be better in this case to do it in the hooks instead?
There was a problem hiding this comment.
every setup/cleanup for tests needs to be in hooks for the the reason I explained: once a test fails, the whole test run is tainted because because its cleanup didn't run. This can be super confusing if you try to debug the last test that fails when the only reason for that test failure is the first test.
Concretely:
say we have test 1 and 2. 1 needs environment A and 2 needs B. 1 sets up env A but fails. Now 2 also fails because it has env A+B. If you don't check the implementation of test 1 first, you might start debugging test 2 even though there's nothing in test 2 that needs fixing.
There was a problem hiding this comment.
You're right, that's a good point, thanks.
I've put the cleanup as part of the afterEach that was already there but I believe that the setup should remain within the test as it's the only test that needs this setup. Wdyt?
Codecov Report
@@ Coverage Diff @@
## main #1215 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 24 24
Lines 992 992
Branches 322 322
=========================================
Hits 992 992
Flags with carried forward coverage won't be shown. Click here to find out more. 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
src/__tests__/suggestions.js
Outdated
| <button data-testid="foo">submit</button> | ||
| <button data-testid="foot">another</button>`) | ||
|
|
||
| expect(() => screen.getByTestId('foo', {suggest: true})).toThrowError() |
There was a problem hiding this comment.
This isn't really testing whether we're suggesting anything. Just that an error is thrown. I'll see if a snapshot makes more sense
What: Add a test that was missing for throwing suggestions
Why: While working on the docs requested in the discussion between @eps1lon and @timdeschryver, I saw that based on the implementation, there's a missing test.
Checklist:
docs site - N/A