Update additional tests to use .toWarnDev() matcher #11957
Conversation
Note this test previous had some hacky logic to verify errors were reported against unique line numbers. Since the new matcher doesn't suppor this, I replaced this check with an equivalent (I think) comparison of unique DOM elements (eg div -> span)
… in its warning. Also updated its test to use .toWarnDev() matcher.
|
|
||
| //verify line number has a proper relative difference, | ||
| //since hard coding the line number would make test too brittle | ||
| expect(parseInt(previousLine, 10) + 12).toBe(parseInt(currentLine, 10)); |
There was a problem hiding this comment.
I think this was intentional although I don’t remember what exactly it’s trying to verify. Maybe it’s not that important.
There was a problem hiding this comment.
I read the intent here as making sure that we warned about both issues with distinct component stacks (pointing to the right element for class and onclick). I'm not positive but that seemed to be the case. I approximated this by changing the element type and asserting with that instead.
There was a problem hiding this comment.
It looks like several of these lines were added by you a year ago 😅 with PR #8570
The others were added 2 years ago with PR #6398 in order to verify that our error message pointed to the correct component+line. (See issue #6062) I think the new approach I'm taking also satisfies this goal! So it should be okay. 👍
|
I should note that I believe there are only 2 things left to update for the new matcher. One of them, the |
|
Here's an opportunity for you to simplify it 😄 (Seriously though, if you have ideas on how to make it better without exploding the whole suite to be 5x bigger and error-prone, let's do it) |
|
FWIW I'm also running into issues with warnings in the integration suite. Particularly with the fact that soon we'll need to pass different expected warning count for client and server, as I explained in #11956 (comment). If you can solve that while you port it to |
|
I think I can slightly improve it (by using a variation of the new matcher) but I'm not sure if I want to take on the scope of refactoring all of those tests at the moment 😄 Cool. I'll add the other to my list of things to think about 👍 |
|
Actually I'm not sure how easy this is going to be to do after all, since Jest doesn't seem to support async custom matchers (jestjs/jest/issues/3874, jestjs/jest/issues/4223) and |
* Migrated several additional tests to use new .toWarnDev() matcher * Migrated ReactDOMComponent-test to use .toWarnDev() matcher Note this test previous had some hacky logic to verify errors were reported against unique line numbers. Since the new matcher doesn't suppor this, I replaced this check with an equivalent (I think) comparison of unique DOM elements (eg div -> span) * Updated several additional tests to use the new .toWarnDev() matcher * Updated many more tests to use .toWarnDev() * Updated several additional tests to use .toWarnDev() matcher * Updated ReactElementValidator to distinguish between Array and Object in its warning. Also updated its test to use .toWarnDev() matcher. * Updated a couple of additional tests * Removed unused normalizeCodeLocInfo() methods
* Migrated several additional tests to use new .toWarnDev() matcher * Migrated ReactDOMComponent-test to use .toWarnDev() matcher Note this test previous had some hacky logic to verify errors were reported against unique line numbers. Since the new matcher doesn't suppor this, I replaced this check with an equivalent (I think) comparison of unique DOM elements (eg div -> span) * Updated several additional tests to use the new .toWarnDev() matcher * Updated many more tests to use .toWarnDev() * Updated several additional tests to use .toWarnDev() matcher * Updated ReactElementValidator to distinguish between Array and Object in its warning. Also updated its test to use .toWarnDev() matcher. * Updated a couple of additional tests * Removed unused normalizeCodeLocInfo() methods
Continuation of PR #11952
Update additional unit tests to use
.toWarnDev()instead ofconsolespies and if-DEV conditionals.Note commit 806035a specifically, as it slightly tweaks a
ReactElementValidatorwarning message to differentiate between Array and Object types. (Is this okay? Seems like an improvement- and since it's a dev-only warning, I don't think it's particularly unsafe.)After this PR, only two files remain that spy on
console:Both are a little involved, so I'll handle them with a subsequent PR.