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

expect: Return false from asymmetric matchers if received value isn’t string #7107

Merged
merged 5 commits into from Dec 24, 2018

Conversation

Projects
None yet
5 participants
@pedrottimark
Copy link
Collaborator

pedrottimark commented Oct 5, 2018

Summary

Fixes #7055

Problem: Although it seemed to make sense at the time to throw an error if received value isn’t string when asymmetric matcher is value of property in .toEqual assertion, it contradicts reasonable expectation in other scenarios:

  • .toBeCalledWith assertion for polymorphic function can throw even if at least one call has an argument that matches
  • ordinary matcher and inverse matcher can both throw instead of returning opposite boolean values

The constructors still throw if the expected value has incorrect type.

Solution:

  • Move isA('String', other) into conditional expression of .stringContaining
  • Delete type check from .stringMatching because regular expression test method returns false if argument isn’t string

Test plan

Rewrite 2 tests 'StringContaining throws for non-strings' as:

  • 'StringContaining throws if expected value is not string'
  • 'StringContaining returns false if received value is not string' (failed before code change)

Rewrite 2 tests 'StringNotContaining throws for non-strings' as:

  • 'StringNotContaining throws if expected value is not string'
  • 'StringNotContaining returns true if received value is not string' (failed before code change)

Rewrite 1 test StringMatching throws for non-strings and non-regexps as:

  • StringMatching throws if expected value is neither string nor regexp

Rewrite 1 test 'StringMatching throws for non-string actual values' as:

  • 'StringMatching returns false if received value is not string' (failed before code change)

Rewrite 1 test StringNotMatching throws for non-strings and non-regexps as:

  • StringNotMatching throws if expected value is neither string nor regexp

Rewrite 1 test 'StringNotMatching throws for non-string actual values' as:

  • 'StringNotMatching returns true if received value is not string' (failed before code change)
@SimenB

SimenB approved these changes Oct 5, 2018

Copy link
Collaborator

SimenB left a comment

Yeah, I agree this makes sense!

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 5, 2018

Codecov Report

Merging #7107 into master will decrease coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7107      +/-   ##
==========================================
- Coverage   66.67%   66.66%   -0.02%     
==========================================
  Files         253      253              
  Lines       10597    10593       -4     
  Branches        3        4       +1     
==========================================
- Hits         7066     7062       -4     
  Misses       3530     3530              
  Partials        1        1
Impacted Files Coverage Δ
packages/expect/src/asymmetric_matchers.js 92.55% <100%> (-0.31%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d59a4d3...17b67af. Read the comment docs.

@pedrottimark

This comment has been minimized.

Copy link
Collaborator Author

pedrottimark commented Oct 5, 2018

Uh oh, the asymmetry of one matcher with && and the other without (pardon pun :) is a mistake.

Added failing test 'StringMatching returns false even if coerced non-string received value matches pattern' and then corrected the code so that the test passes.

@thymikee
Copy link
Collaborator

thymikee left a comment

@pedrottimark mind resolving conflicts? :)

@SimenB SimenB merged commit a733592 into facebook:master Dec 24, 2018

9 of 10 checks passed

ci/circleci: test-node-8 Your tests failed on CircleCI
Details
ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details

@pedrottimark pedrottimark deleted the pedrottimark:asymmetric-string branch Dec 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment