Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

expect: Improve report when matcher fails, part 19 #8367

Merged
merged 5 commits into from
Apr 25, 2019
Merged

expect: Improve report when matcher fails, part 19 #8367

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
66d1fc3
expect: Improve report when matcher fails, part 19
pedrottimark Apr 23, 2019
9954ac1
Delete obsolete code from printConstructorName
pedrottimark Apr 23, 2019
f55e9ed
Update CHANGELOG.md
pedrottimark Apr 23, 2019
b2af4b5
Call printReceivedConstructorName with constructor arg
pedrottimark Apr 24, 2019
d0ac050
Append extends to be clearer when negative assertion fails
pedrottimark Apr 24, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
- `[docs]` Add DynamoDB guide ([#8319](https://github.com/facebook/jest/pull/8319))
- `[expect]` Improve report when matcher fails, part 17 ([#8349](https://github.com/facebook/jest/pull/8349))
- `[expect]` Improve report when matcher fails, part 18 ([#8356](https://github.com/facebook/jest/pull/8356))
- `[expect]` Improve report when matcher fails, part 19 ([#8367](https://github.com/facebook/jest/pull/8367))

### Fixes

Expand Down
23 changes: 15 additions & 8 deletions packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1265,6 +1265,7 @@ exports[`.toBeInstanceOf() failing "a" and [Function String] 1`] = `
"<dim>expect(</><red>received</><dim>).</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: <green>String</>

Received value has no prototype
Received value: <red>\\"a\\"</>"
`;
Expand All @@ -1274,13 +1275,14 @@ exports[`.toBeInstanceOf() failing /\\w+/ and [Function anonymous] 1`] = `

Expected constructor name is an empty string
Received constructor: <red>RegExp</>
Received value: <red>/\\\\w+/</>"
"
`;

exports[`.toBeInstanceOf() failing {} and [Function A] 1`] = `
"<dim>expect(</><red>received</><dim>).</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: <green>A</>

Received value has no prototype
Received value: <red>{}</>"
`;
Expand All @@ -1290,21 +1292,22 @@ exports[`.toBeInstanceOf() failing {} and [Function B] 1`] = `

Expected constructor: <green>B</>
Received constructor: <red>A</>
Received value: <red>{}</>"
"
`;

exports[`.toBeInstanceOf() failing {} and [Function RegExp] 1`] = `
"<dim>expect(</><red>received</><dim>).</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: <green>RegExp</>
Received constructor name is an empty string
Received value: <red>{}</>"
"
`;

exports[`.toBeInstanceOf() failing 1 and [Function Number] 1`] = `
"<dim>expect(</><red>received</><dim>).</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: <green>Number</>

Received value has no prototype
Received value: <red>1</>"
`;
Expand All @@ -1313,6 +1316,7 @@ exports[`.toBeInstanceOf() failing null and [Function String] 1`] = `
"<dim>expect(</><red>received</><dim>).</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: <green>String</>

Received value has no prototype
Received value: <red>null</>"
`;
Expand All @@ -1321,6 +1325,7 @@ exports[`.toBeInstanceOf() failing true and [Function Boolean] 1`] = `
"<dim>expect(</><red>received</><dim>).</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: <green>Boolean</>

Received value has no prototype
Received value: <red>true</>"
`;
Expand All @@ -1329,6 +1334,7 @@ exports[`.toBeInstanceOf() failing undefined and [Function String] 1`] = `
"<dim>expect(</><red>received</><dim>).</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: <green>String</>

Received value has no prototype
Received value: <red>undefined</>"
`;
Expand All @@ -1337,35 +1343,36 @@ exports[`.toBeInstanceOf() passing [] and [Function Array] 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: not <green>Array</>
Received value: <red>[]</>"
"
`;

exports[`.toBeInstanceOf() passing {} and [Function A] 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: not <green>A</>
Received value: <red>{}</>"
"
`;

exports[`.toBeInstanceOf() passing {} and [Function B] 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: not <green>B</>
Received value: <red>{}</>"
Received constructor: <red>C</>
"
`;

exports[`.toBeInstanceOf() passing {} and [Function name() {}] 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor name is not a string
Received value: <red>{}</>"
"
`;

exports[`.toBeInstanceOf() passing Map {} and [Function Map] 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toBeInstanceOf<dim>(</><green>expected</><dim>)</>

Expected constructor: not <green>Map</>
Received value: <red>Map {}</>"
"
`;

exports[`.toBeInstanceOf() throws if constructor is not a function 1`] = `
Expand Down
52 changes: 36 additions & 16 deletions packages/expect/src/__tests__/__snapshots__/toThrowMatchers.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -89,16 +89,16 @@ Received message: <red>\\"apple\\"</>
exports[`toThrow error class did not throw at all 1`] = `
"<dim>expect(</><red>received</><dim>).</>toThrow<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err\\"</>
Expected constructor: <green>Err</>

Received function did not throw"
`;

exports[`toThrow error class threw, but class did not match (error) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toThrow<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err2\\"</>
Received name: <red>\\"Error\\"</>
Expected constructor: <green>Err2</>
Received constructor: <red>Err</>

Received message: <red>\\"apple\\"</>

Expand All @@ -108,17 +108,27 @@ Received message: <red>\\"apple\\"</>
exports[`toThrow error class threw, but class did not match (non-error falsey) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toThrow<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err2\\"</>
Expected constructor: <green>Err2</>

Received value: <red>undefined</>
"
`;

exports[`toThrow error class threw, but class should not match (error subclass) 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toThrow<dim>(</><green>expected</><dim>)</>

Expected constructor: not <green>Err</>
Received constructor: <red>SubErr</>

Received message: <red>\\"apple\\"</>

<dim>at jestExpect (</>packages/expect/src/__tests__/toThrowMatchers-test.js<dim>:24:74)</>"
`;

exports[`toThrow error class threw, but class should not match (error) 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toThrow<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err\\"</>
Received name: <red>\\"Error\\"</>
Expected constructor: not <green>Err</>

Received message: <red>\\"apple\\"</>

Expand Down Expand Up @@ -174,8 +184,8 @@ Received function did not throw"
exports[`toThrow promise/async throws if Error-like object is returned threw, but class did not match 1`] = `
"<dim>expect(</><red>received</><dim>).</>rejects<dim>.</>toThrow<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err2\\"</>
Received name: <red>\\"Error\\"</>
Expected constructor: <green>Err2</>
Received constructor: <red>Err</>

Received message: <red>\\"async apple\\"</>

Expand Down Expand Up @@ -364,16 +374,16 @@ Received message: <red>\\"apple\\"</>
exports[`toThrowError error class did not throw at all 1`] = `
"<dim>expect(</><red>received</><dim>).</>toThrowError<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err\\"</>
Expected constructor: <green>Err</>

Received function did not throw"
`;

exports[`toThrowError error class threw, but class did not match (error) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toThrowError<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err2\\"</>
Received name: <red>\\"Error\\"</>
Expected constructor: <green>Err2</>
Received constructor: <red>Err</>

Received message: <red>\\"apple\\"</>

Expand All @@ -383,17 +393,27 @@ Received message: <red>\\"apple\\"</>
exports[`toThrowError error class threw, but class did not match (non-error falsey) 1`] = `
"<dim>expect(</><red>received</><dim>).</>toThrowError<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err2\\"</>
Expected constructor: <green>Err2</>

Received value: <red>undefined</>
"
`;

exports[`toThrowError error class threw, but class should not match (error subclass) 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toThrowError<dim>(</><green>expected</><dim>)</>

Expected constructor: not <green>Err</>
Received constructor: <red>SubErr</>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should print something for subclasses? It might not always be obvious why this failed if the names look unrelated

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it isn’t obvious. Your thoughts how to make it clear are welcome. It reminds me of (positive and negative) assertions for < and so on, where report shows the comparison operator.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of this?

not_toBeInstanceOf_base_class

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reminds of the joke:

I’m glad I don’t like X, because if I liked it, I would have to eat it, but I don’t like it!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think of this?

The idea is super awesome, unfortunately it's not quite correct though, C.prototype instanceof B would be but that's clunky.
What do you think about C extends B?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, your suggestion is very intuitive!

Do we extend the notation (pardon pun ;) for C extends MiddleClass extends B

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if it's not too much work, why not. Are there many hundred prototypes long chains in the real world? If so, we might need to limit it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here is a compromise that might balance clear and correct without over-explaining:

Expected constructor: not Parent
Received constructor:     Child extends Parent

Expected constructor: not Ancestor
Received constructor:     Descendant extends … extends Ancestor

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extends ... extends is exactly what I thought of 👌


Received message: <red>\\"apple\\"</>

<dim>at jestExpect (</>packages/expect/src/__tests__/toThrowMatchers-test.js<dim>:24:74)</>"
jeysal marked this conversation as resolved.
Show resolved Hide resolved
`;

exports[`toThrowError error class threw, but class should not match (error) 1`] = `
"<dim>expect(</><red>received</><dim>).</>not<dim>.</>toThrowError<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err\\"</>
Received name: <red>\\"Error\\"</>
Expected constructor: not <green>Err</>

Received message: <red>\\"apple\\"</>

Expand Down Expand Up @@ -449,8 +469,8 @@ Received function did not throw"
exports[`toThrowError promise/async throws if Error-like object is returned threw, but class did not match 1`] = `
"<dim>expect(</><red>received</><dim>).</>rejects<dim>.</>toThrowError<dim>(</><green>expected</><dim>)</>

Expected name: <green>\\"Err2\\"</>
Received name: <red>\\"Error\\"</>
Expected constructor: <green>Err2</>
Received constructor: <red>Err</>

Received message: <red>\\"async apple\\"</>

Expand Down
17 changes: 17 additions & 0 deletions packages/expect/src/__tests__/toThrowMatchers.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,6 +146,15 @@ class customError extends Error {
});

describe('error class', () => {
class SubErr extends Err {
constructor(...args) {
super(...args);
// In a carefully written error subclass,
// name property is equal to constructor name.
this.name = this.constructor.name;
}
}

it('passes', () => {
jestExpect(() => {
throw new Err();
Expand Down Expand Up @@ -189,6 +198,14 @@ class customError extends Error {
}).not[toThrow](Err);
}).toThrowErrorMatchingSnapshot();
});

test('threw, but class should not match (error subclass)', () => {
expect(() => {
jestExpect(() => {
throw new SubErr('apple');
}).not[toThrow](Err);
}).toThrowErrorMatchingSnapshot();
});
});

describe('error-message', () => {
Expand Down
55 changes: 25 additions & 30 deletions packages/expect/src/matchers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ import {
import {MatchersObject, MatcherState} from './types';
import {
printDiffOrStringify,
printExpectedConstructorName,
printReceivedArrayContainExpectedItem,
printReceivedConstructorName,
printReceivedStringContainExpectedResult,
printReceivedStringContainExpectedSubstring,
} from './print';
Expand Down Expand Up @@ -267,45 +269,38 @@ const matchers: MatchersObject = {

const pass = received instanceof expected;

const NAME_IS_NOT_STRING = ' name is not a string\n';
const NAME_IS_EMPTY_STRING = ' name is an empty string\n';

const message = pass
? () =>
matcherHint(matcherName, undefined, undefined, options) +
'\n\n' +
// A truthy test for `expected.name` property has false positive for:
// function with a defined name property
// class with a static name method
(typeof expected.name !== 'string'
? 'Expected constructor' + NAME_IS_NOT_STRING
: expected.name.length === 0
? 'Expected constructor' + NAME_IS_EMPTY_STRING
: `Expected constructor: not ${EXPECTED_COLOR(expected.name)}\n`) +
`Received value: ${printReceived(received)}`
printExpectedConstructorName('Expected constructor', expected, true) +
(received.constructor != null &&
received.constructor.name !== expected.name
? printReceivedConstructorName(
'Received constructor',
received,
true,
)
: '')
: () =>
matcherHint(matcherName, undefined, undefined, options) +
'\n\n' +
// A truthy test for `expected.name` property has false positive for:
// function with a defined name property
// class with a static name method
(typeof expected.name !== 'string'
? 'Expected constructor' + NAME_IS_NOT_STRING
: expected.name.length === 0
? 'Expected constructor' + NAME_IS_EMPTY_STRING
: `Expected constructor: ${EXPECTED_COLOR(expected.name)}\n`) +
printExpectedConstructorName(
'Expected constructor',
expected,
false,
) +
(isPrimitive(received) || Object.getPrototypeOf(received) === null
? 'Received value has no prototype\n'
? `\nReceived value has no prototype\nReceived value: ${printReceived(
received,
)}`
: typeof received.constructor !== 'function'
? ''
: typeof received.constructor.name !== 'string'
? 'Received constructor' + NAME_IS_NOT_STRING
: received.constructor.name.length === 0
? 'Received constructor' + NAME_IS_EMPTY_STRING
: `Received constructor: ${RECEIVED_COLOR(
received.constructor.name,
)}\n`) +
`Received value: ${printReceived(received)}`;
? `\nReceived value: ${printReceived(received)}`
: printReceivedConstructorName(
'Received constructor',
received,
false,
));

return {message, pass};
},
Expand Down
Loading