expect: Improve report when matcher fails, part 11 #8008
Conversation
For now, I'd say "no". If (when) we add support for Looking forward to your screenshots 😀 |
|
Example pictures baseline at left and improved at right Matcher name is regular black:
Possible future improvement to highlight item found according to deep equality:
Can highlight substring in string:
Cannot (yet) highlight value in non-array iterable object:
Matcher name is regular black:
Can highlight item in array:
Align expected and received values:
Can highlight pattern in string:
|
Codecov Report
@@ Coverage Diff @@
## master #8008 +/- ##
==========================================
+ Coverage 64.12% 64.24% +0.12%
==========================================
Files 259 260 +1
Lines 10150 10162 +12
Branches 1754 1764 +10
==========================================
+ Hits 6509 6529 +20
+ Misses 3255 3245 -10
- Partials 386 388 +2
Continue to review full report at Codecov.
|
| const converted: any = | ||
| isArray || isString ? received : Array.from(received); | ||
| if (typeof received === 'string') { | ||
| const index = received.indexOf(String(expected)); |
There was a problem hiding this comment.
too lazy to check, but this reads to be like
expect('hello null in the middle').toContain(null) would pass? Since String(null) === 'null'
There was a problem hiding this comment.
Yes, good eyes. It is for correct typing of dubious behavior.
- Do you think it should throw matcher error if received is string and expected is not?
- If yes, is that a breaking change that we wait to add at Jest 25.
There was a problem hiding this comment.
it fails today, doesn't it? I'd say it's a bug. If an expect fails with "matcher error" or "null is not in string 'null'" shouldn't matter?
Or does expect('hello null in the middle').toContain(null) pass today?
There was a problem hiding this comment.
I will double-check but the original code is .indexOf(value) which implicitly coerces to string:
test('null', () => {
expect('isnullornot'.indexOf(null)).toBe(2);
});There was a problem hiding this comment.
Oh, I didn't know indexOf coerced to string! TIL. Same for includes actually ('isnullornot'.includes(null) === true). I'd still say it's a false positive, but I don't feel strongly about it
There was a problem hiding this comment.
What do you think if I include the code and tests commented out, with a comment that we can find: // BREAKING change to include in Jest 25
There was a problem hiding this comment.
For tracking the breaking change to be included, we should create an issue assigned to the milestone like #7749.
I'd prefer not to put significant amounts of commented out code into master, if you've already written it maybe push it to a branch and link that in the issue so it doesn't get lost?
There was a problem hiding this comment.
feel free to open one - we can stick all the tiny breakages we wanna make there
There was a problem hiding this comment.
@SimenB Do you mean one for all of them? I think I'd prefer separate issues unless there is a group of breaking changes that have something in common. The small issues will already be grouped using the milestone.
|
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
For
.toContain,.toContainEqual, and.toMatch:rejectsorresolvesin matcher hinttoContainortoContainEqualthrow matcher errornotfollowing expected labelFor more information, see discussion with @jeysal in #7795
Your critique about clarity is especially welcome, because I had to wrestle with the code not to add any more
anytype than the one already intoContainmatcher.Design discussion your thoughts are welcome for a possible future pull request:
// indexOfcomment in matcher hint fortoContainconfusing if received is iterable (for example, set) instead of array?.toContainreferential identity fails for item in array but deep equality succeeds, to highlight the item?.toContainEqualmatcher to be less wordy and more neutral (for example, to fix mistake in Redux reducer,.toContainmight fail in first phase of TDD even though referential identity is the correct criterion)Residue for future pull requests:
.toThrow(string | RegExp)matcher.execmethod in addition to.testmethodTest plan
Updated existing snapshot tests:
.toContain.toContainEqual.toMatchSee also pictures in following comment.