Fixed potential false-positive in toWarnDev matcher #11898
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
4 tasks
bvaughn
commented
Jan 2, 2018
scripts/jest/setupTests.js
Outdated
| const noop = function() {}; | ||
| const spyOn = function(object, methodName) { | ||
| if (object === console) { |
There was a problem hiding this comment.
Note to self: Some of our tests spy on console.log (eg ReactDOMComponent-test) so this would also need to check if methodName === "warn" || methodName === "error"
There was a problem hiding this comment.
We could also change them to use jest.fn or something directly.
bvaughn
force-pushed
the
spyOn-console-warning
branch
from
9dbe23c
to
5880076
Compare
bvaughn
changed the title
bvaughn
changed the title
Also updated (most of) ReactIncrementalErrorLogging-test.internal to use the new matcher
bvaughn
changed the title
bvaughn
force-pushed
the
spyOn-console-warning
branch
from
988f976
to
1a8c80e
Compare
yenshih
pushed a commit
to yenshih/react
that referenced
this pull request
Jan 6, 2018
* Warn about spying on the console * Added suppress warning flag for spyOn(console) * Nits * Removed spy-on-console guard * Fixed a potential source of false-positives in toWarnDev() matcher Also updated (most of) ReactIncrementalErrorLogging-test.internal to use the new matcher * Removed unused third param to spyOn * Improved clarity of inline comments * Removed unused normalizeCodeLocInfo() method
ManasJayanth
pushed a commit
to ManasJayanth/react
that referenced
this pull request
Jan 12, 2018
* Warn about spying on the console * Added suppress warning flag for spyOn(console) * Nits * Removed spy-on-console guard * Fixed a potential source of false-positives in toWarnDev() matcher Also updated (most of) ReactIncrementalErrorLogging-test.internal to use the new matcher * Removed unused third param to spyOn * Improved clarity of inline comments * Removed unused normalizeCodeLocInfo() method
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.
Spying on(This has been removed.)console.errororconsole.warnwill now fail tests by default, with an error suggesting the use of the newtoWarnDevmatcher instead.Updated
.toWarnDev()matcher to report a failed expectation to Jest even if anErrorwas thrown by its callback. Giving precedence to failed expectations prevents a thrown error from potentially masking failed expectations (and removes a source of potential false-positives from our tests).As of this point, all but 2 of our tests have been updated to the new matcher:
ReactIncrementalErrorLogging-test.internal: There's one test case that I'm unsure of how to update. I'll look at it again tomorrow.ReactDOMServerIntegrationTestUtils: It's possible to update this test util but it will take time since several, procedurally-generated tests use it. For the time being, I've added a TODO comment for following up on it.