-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Conversation
There was a problem hiding this 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 👍
Co-authored-by: Simen Bekkhus <sbekkhus91@gmail.com>
'received', | ||
)} value is a string`; | ||
|
||
if (expected == null) { |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR opened: #10929
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. |
Should fix #8023
Test plan
I added the following cases as expected errors: