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.

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

Make toContain more strict with the received type #10119

Merged
merged 4 commits into from Dec 5, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
1 change: 1 addition & 0 deletions CHANGELOG.md
Expand Up @@ -10,6 +10,7 @@
- `[jest-jasmine2]` Stop adding `:` after an error that has no message ([#9990](https://github.com/facebook/jest/pull/9990))
- `[jest-diff]` Control no diff message color with `commonColor` in diff options ([#9997](https://github.com/facebook/jest/pull/9997))
- `[jest-snapshot]` Fix TypeScript compilation ([#10008](https://github.com/facebook/jest/pull/10008))
- `[expect]` Make `toContain` more strict with the received type ([#10119](https://github.com/facebook/jest/pull/10119))
ghostd marked this conversation as resolved.
Show resolved Hide resolved

### Chore & Maintenance

Expand Down
31 changes: 31 additions & 0 deletions packages/expect/src/__tests__/__snapshots__/matchers.test.js.snap
Expand Up @@ -1958,6 +1958,37 @@ exports[`.toContain(), .toContainEqual() error cases 1`] = `
Received has value: <r>null</>
`;

exports[`.toContain(), .toContainEqual() error cases 2`] = `
<d>expect(</><r>-0</><d>).</>toContain<d>(</><g>0</><d>) // indexOf</>

<b>Matcher error</>: <g>expected</> value must be a string if <r>received</> value is a string

Expected has type: number
Expected has value: <g>-0</>
Received has type: string
Received has value: <r>"-0"</>
`;

exports[`.toContain(), .toContainEqual() error cases 3`] = `
<d>expect(</><r>null</><d>).</>toContain<d>(</><g>null</><d>) // indexOf</>

<b>Matcher error</>: <g>expected</> value must be a string if <r>received</> value is a string

Expected has value: <g>null</>
Received has type: string
Received has value: <r>"null"</>
`;

exports[`.toContain(), .toContainEqual() error cases 4`] = `
<d>expect(</><r>undefined</><d>).</>toContain<d>(</><g>undefined</><d>) // indexOf</>

<b>Matcher error</>: <g>expected</> value must be a string if <r>received</> value is a string

Expected has value: <g>undefined</>
Received has type: string
Received has value: <r>"undefined"</>
`;

exports[`.toContain(), .toContainEqual() error cases for toContainEqual 1`] = `
<d>expect(</><r>received</><d>).</>toContainEqual<d>(</><g>expected</><d>) // deep equality</>

Expand Down
7 changes: 7 additions & 0 deletions packages/expect/src/__tests__/matchers.test.js
Expand Up @@ -1463,6 +1463,13 @@ describe('.toContain(), .toContainEqual()', () => {

test('error cases', () => {
expect(() => jestExpect(null).toContain(1)).toThrowErrorMatchingSnapshot();
expect(() => jestExpect('-0').toContain(-0)).toThrowErrorMatchingSnapshot();
expect(() =>
jestExpect('null').toContain(null),
).toThrowErrorMatchingSnapshot();
expect(() =>
jestExpect('undefined').toContain(undefined),
).toThrowErrorMatchingSnapshot();
});

[
Expand Down
29 changes: 29 additions & 0 deletions packages/expect/src/matchers.ts
Expand Up @@ -472,6 +472,35 @@ const matchers: MatchersObject = {
}

if (typeof received === 'string') {
const wrongTypeErrorMessage = `${EXPECTED_COLOR(
'expected',
)} value must be a string if ${RECEIVED_COLOR(
'received',
)} value is a string`;

if (expected == null) {
Copy link
Contributor

@dubzzz dubzzz Dec 6, 2020

Choose a reason for hiding this comment

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

My understanding of the PR is that it should prevent users from passing wrongly typed values for toContain. With current PR it seems that expectation like expect("0").toContain(0); will no longer pass while it did in version 26. (Clearly a cool change IMO)

What about those ones (might already been covered)?

  • expect("false").toContain(false);
  • expect("0").toContain(0n);

The check for typeof expected only seems to handle the case of a number. But there are possibly other cases that may be problematic (booleans, bigints).

Copy link
Member

Choose a reason for hiding this comment

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

they should all fail IMO. If they don't we should fix it. If they do we should add tests

Copy link
Contributor

Choose a reason for hiding this comment

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

I have not tested it yet to be honest. I was just surprised about the typeof check for numbers

Copy link
Contributor

Choose a reason for hiding this comment

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

I confirm the fact that expect("1").toContain(1n) and expect("false").toContain(false) are passing even with the fix. I am opening a PR for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

PR opened: #10929

throw new Error(
matcherErrorMessage(
matcherHint(matcherName, received, String(expected), options),
wrongTypeErrorMessage,
printWithType('Expected', expected, printExpected) +
'\n' +
printWithType('Received', received, printReceived),
),
);
}
if (typeof expected === 'number') {
throw new Error(
matcherErrorMessage(
matcherHint(matcherName, received, String(expected), options),
wrongTypeErrorMessage,
printWithType('Expected', expected, printExpected) +
'\n' +
printWithType('Received', received, printReceived),
),
);
}

const index = received.indexOf(String(expected));
const pass = index !== -1;

Expand Down