Fixes #24336 - upgrade jest and fix errors #7555
Conversation
|
Issues: #24336 |
|
combined with #7538, this should make our tests noise-free 😎 |
|
to test |
webpack/components/Search/index.js
Outdated
| @@ -39,7 +39,7 @@ class Search extends Component { | |||
| text: label.trim(), | |||
| })), | |||
| }); | |||
| }); | |||
| }).catch(() => {}); | |||
There was a problem hiding this comment.
@tstrachota This is empty to prevent the unhandled promise warning, but do we want to catch errors here?
There was a problem hiding this comment.
If something breaks it would produce an error on each type in the field, right? In this case I'd avoid propagating the errors as toast notifications but I think it would make sense to log them into console.
There was a problem hiding this comment.
Nice fixes, thanks @johnpmitsch.
With your patch tests fail for me locally as well as in jenkins. RecommendedRepositorySetsToggler › rendering › renders recommended-repository-sets-toggler doesn't match the stored snapshot (that you're changing in the PR btw).
| // output and traceback for actual error. | ||
| global.console.error = (error) => { | ||
| throw new Error(error); | ||
| }; |
johnpmitsch
force-pushed
the
upgrade_jest
branch
2 times, most recently
from
355776c
to
f6276bb
Compare
|
@tbrisker thanks, I guess I had something wrong with my environment, on a new environment I was able to reproduce the bad snapshot and corrected it (back to the original snapshot) I modified the Search test to log the Promise errors to the console. To make sure nothing prints out during the test, I mocked the API. I also added a |
johnpmitsch
force-pushed
the
upgrade_jest
branch
2 times, most recently
from
1d90a5c
to
62ca993
Compare
|
@tbrisker @waldenraines I've updated this and added fixes for the breakages in master. Since this is a small PR, I recommend we merge this if everyone is ok with it to get master branch back to a healthy state 🌡️ let me know your thoughts |
webpack/components/Search/index.js
Outdated
| }) | ||
| .catch((error) => { | ||
| // eslint-disable-next-line no-console | ||
| console.log(error); |
There was a problem hiding this comment.
Why do we want to console.log() this error? We should do something useful with the error or do nothing at all.
There was a problem hiding this comment.
@waldenraines This is roughly the same question I asked @tbrisker here, maybe he can expand on it? (the comment was collapsed so it was hidden).
There was a problem hiding this comment.
@waldenraines The reason I added the catch block is otherwise node throws a "unhandled promise" warning, but running this again, I see this is only because I was using a newer version of nodejs. I completely removed the catch block without any warnings, I think this is fine for now (until we update node versions)
This does a few things: - upgrades jest to 23.4.1 and adds new snapshots to be compatible with the new version. - adds a script to run before every test that makes the test fail when there are console errors. This is helpful in catching missing propTypes from the tests, since those will print console errors, but not actually fail the test itself. - Fixes PropType errors. - Fixes unhandled promise warning.
johnpmitsch
force-pushed
the
upgrade_jest
branch
from
62ca993
to
48a1570
Compare
|
@johnpmitsch you seem to be mentioning me instead of @tstrachota ;) |
|
@tbrisker sorry, I think that is the 2nd time I've done that 🙈 |
|
I'm merging this so we can fix tests in master |
|
and I accidently hit "close and comment" so have to wait for jenkins again... not my day today 😅 |
This does a few things:
to be compatible with the new version.
the test fail when there are console errors. This
is helpful in catching missing propTypes from the
tests, since those will print console errors, but
not actually fail the test itself.