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

Conversation

ghostd
Copy link
Contributor

@ghostd ghostd commented Jun 2, 2020

Should fix #8023

Test plan

I added the following cases as expected errors:

expect('-0').toContain(-0)
expect('null').toContain(null)
expect('undefined').toContain(undefined)

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

Thanks! I think this makes sense, although I'd consider it a breaking change. Will land in 27 👍

CHANGELOG.md Outdated Show resolved Hide resolved
@SimenB SimenB added this to the Jest 27 milestone Oct 19, 2020
@SimenB SimenB merged commit 8020c31 into jestjs:master Dec 5, 2020
@ghostd ghostd deleted the add-match-errors branch December 6, 2020 10:06
'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

@github-actions
Copy link

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.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

expect('null').toContain(null) should fail
4 participants