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 11 #8008

Merged
merged 5 commits into from
Mar 2, 2019
Merged

expect: Improve report when matcher fails, part 11 #8008

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6340287
expect: Improve report when matcher fails, part 11
pedrottimark Feb 28, 2019
7969645
Update CHANGELOG.md
pedrottimark Feb 28, 2019
be2b993
Write concise test for null or undefined
pedrottimark Mar 1, 2019
08b54d2
Separate code for string from array-iterable
pedrottimark Mar 1, 2019
ed4f245
Remove ternary expression in indexable assignment
pedrottimark Mar 2, 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
123 changes: 70 additions & 53 deletions packages/expect/src/matchers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,9 +375,10 @@ const matchers: MatchersObject = {
received: ContainIterable | string,
expected: unknown,
) {
const isNot = this.isNot;
const options: MatcherHintOptions = {
comment: 'indexOf',
isNot: this.isNot,
isNot,
promise: this.promise,
};

Expand All @@ -391,48 +392,65 @@ const matchers: MatchersObject = {
);
}

const isArray = Array.isArray(received); // has indexOf
const isString = typeof received === 'string'; // has indexOf
const converted: any =
isArray || isString ? received : Array.from(received);
if (typeof received === 'string') {
const index = received.indexOf(String(expected));
Copy link
Member

Choose a reason for hiding this comment

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

too lazy to check, but this reads to be like

expect('hello null in the middle').toContain(null) would pass? Since String(null) === 'null'

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, good eyes. It is for correct typing of dubious behavior.

  1. Do you think it should throw matcher error if received is string and expected is not?
  2. If yes, is that a breaking change that we wait to add at Jest 25.

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will double-check but the original code is .indexOf(value) which implicitly coerces to string:

  test('null', () => {
    expect('isnullornot'.indexOf(null)).toBe(2);
  });

Copy link
Member

Choose a reason for hiding this comment

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

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

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 if I include the code and tests commented out, with a comment that we can find: // BREAKING change to include in Jest 25

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

feel free to open one - we can stick all the tiny breakages we wanna make there

Copy link
Contributor

Choose a reason for hiding this comment

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

Created #8023

Copy link
Contributor

Choose a reason for hiding this comment

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

@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.

const pass = index !== -1;

const message = () => {
const labelExpected = `Expected ${
typeof expected === 'string' ? 'substring' : 'value'
}`;
const labelReceived = 'Received string';
const printLabel = getLabelPrinter(labelExpected, labelReceived);

return (
matcherHint('toContain', undefined, undefined, options) +
'\n\n' +
`${printLabel(labelExpected)}${isNot ? 'not ' : ''}${printExpected(
expected,
)}\n` +
`${printLabel(labelReceived)}${isNot ? ' ' : ''}${
isNot
? printReceivedStringContainExpectedSubstring(
received,
index,
String(expected).length,
)
: printReceived(received)
}`
);
};

return {message, pass};
}

// At this point, we're either a string or an Array,
// which was converted from an array-like structure.
const index = converted.indexOf(expected);
const indexable = Array.isArray(received) ? received : Array.from(received);
pedrottimark marked this conversation as resolved.
Show resolved Hide resolved
const index = indexable.indexOf(expected);
pedrottimark marked this conversation as resolved.
Show resolved Hide resolved
const pass = index !== -1;

const message = () => {
const labelExpected = `Expected ${
isString && typeof expected === 'string' ? 'substring' : 'value'
}`;
const labelExpected = 'Expected value';
const labelReceived = `Received ${getType(received)}`;
const printLabel = getLabelPrinter(labelExpected, labelReceived);

return pass
? matcherHint('toContain', undefined, undefined, options) +
'\n\n' +
`${printLabel(labelExpected)}not ${printExpected(expected)}\n` +
`${printLabel(labelReceived)} ${
Array.isArray(received)
? printReceivedArrayContainExpectedItem(received, index)
: typeof received === 'string'
? printReceivedStringContainExpectedSubstring(
received,
index,
String(expected).length,
)
: printReceived(received)
}`
: matcherHint('toContain', undefined, undefined, options) +
'\n\n' +
`${printLabel(labelExpected)}${printExpected(expected)}\n` +
`${printLabel(labelReceived)}${printReceived(received)}` +
(converted instanceof Array &&
converted.findIndex(item =>
equals(item, expected, [iterableEquality]),
) !== -1
? `\n\n${SUGGEST_TO_CONTAIN_EQUAL}`
: '');
return (
matcherHint('toContain', undefined, undefined, options) +
'\n\n' +
`${printLabel(labelExpected)}${isNot ? 'not ' : ''}${printExpected(
expected,
)}\n` +
`${printLabel(labelReceived)}${isNot ? ' ' : ''}${
isNot && Array.isArray(received)
? printReceivedArrayContainExpectedItem(received, index)
: printReceived(received)
}` +
(!isNot &&
indexable.findIndex(item =>
equals(item, expected, [iterableEquality]),
) !== -1
? `\n\n${SUGGEST_TO_CONTAIN_EQUAL}`
: '')
);
};

return {message, pass};
Expand All @@ -443,9 +461,10 @@ const matchers: MatchersObject = {
received: ContainIterable,
expected: unknown,
) {
const isNot = this.isNot;
const options: MatcherHintOptions = {
comment: 'deep equality',
isNot: this.isNot,
isNot,
promise: this.promise,
};

Expand All @@ -459,9 +478,8 @@ const matchers: MatchersObject = {
);
}

const converted = Array.isArray(received) ? received : Array.from(received);

const index = converted.findIndex(item =>
const indexable = Array.isArray(received) ? received : Array.from(received);
const index = indexable.findIndex(item =>
equals(item, expected, [iterableEquality]),
);
const pass = index !== -1;
Expand All @@ -471,19 +489,18 @@ const matchers: MatchersObject = {
const labelReceived = `Received ${getType(received)}`;
const printLabel = getLabelPrinter(labelExpected, labelReceived);

return pass
? matcherHint('toContainEqual', undefined, undefined, options) +
'\n\n' +
`${printLabel(labelExpected)}not ${printExpected(expected)}\n` +
`${printLabel(labelReceived)} ${
Array.isArray(received)
? printReceivedArrayContainExpectedItem(received, index)
: printReceived(received)
}`
: matcherHint('toContainEqual', undefined, undefined, options) +
'\n\n' +
`${printLabel(labelExpected)}${printExpected(expected)}\n` +
`${printLabel(labelReceived)}${printReceived(received)}`;
return (
matcherHint('toContainEqual', undefined, undefined, options) +
'\n\n' +
`${printLabel(labelExpected)}${isNot ? 'not ' : ''}${printExpected(
expected,
)}\n` +
`${printLabel(labelReceived)}${isNot ? ' ' : ''}${
isNot && Array.isArray(received)
? printReceivedArrayContainExpectedItem(received, index)
: printReceived(received)
}`
);
};

return {message, pass};
Expand Down