Don't suppress jsdom error reporting in our tests #13401
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gaearon
changed the title
|
ReactDOM: size: -0.3%, gzip: -0.3% Details of bundled changes.Comparing: 69e2a0d...fffe608 react-dom
react-art
react-test-renderer
Generated by 🚫 dangerJS |
bvaughn
approved these changes
Aug 15, 2018
| // Manually override console.error() because our toWarn() matchers | ||
| // filter out precisely the messages we want to test for in this file. | ||
| const oldConsoleError = console.error; | ||
| console.error = jest.fn(); |
There was a problem hiding this comment.
Why not do this override-and-restore stuff in beforeEach/afterEach ? Seems more "standard"
There was a problem hiding this comment.
I wanted it to really jump in front of you because I think it might be easy to miss that in this file console.error setup differs from our normal one. I don't feel strongly about this.
There was a problem hiding this comment.
Gotcha. I also don't feel strongly about this, was just curious.
scripts/jest/matchers/toWarnDev.js
Outdated
| const createConsoleSpy = methodName => (format, ...args) => { | ||
| // Ignore uncaught errors reported by jsdom | ||
| // and React addendums because they're too noisy. | ||
| if (methodName === 'error' && shouldIgnoreConsoleError(format, args)) { |
There was a problem hiding this comment.
Totally unimportant but I'm just curious– was changing consoleSpy to createConsoleSpy actually unnecessary? Couldn't methodName have just used consoleMethod?
There was a problem hiding this comment.
Didn't realize it was in scope lol.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Fixes #8260.
It removes the
errorevent listener which caused all jsdom errors to be ignored. We still silence some of the errors, but atconsole.errorcall time. The benefit of this approach compared to our existingevent.preventDefault()call is that we still fail the test if the error was unexpected (like described in #8260). And it's nice to remove the weird hack with mucking the error prototype.I implemented silencing by adding a few special cases to our
console.errormocks that ignore certain messages (seeshouldIgnoreConsoleError.js). I've spent a few days trying several other explicit approaches before that. They were all too annoying and complicated, and led to too much noise in tests themselves. Especially because we have many different configurations (dev, prod, bundles). Ignoring ended up the most straightforward (and hopefully maintainable) solution. And that's what we were effectively doing anyway, except at an earlier stage.For the few cases where we were testing logging itself, I changed those to directly overwrite
console.errorand assert on the mock calls. Seems easier than remembering what gets mocked and when. I also made the test non-internal while I was at it.