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

Asymmetric matchers expect.stringMatches and expect.stringContains throw Error and let (otherwise successful) tests fail #7055

Closed
ryx opened this issue Sep 27, 2018 · 9 comments · Fixed by #7107

Comments

@ryx
Copy link

ryx commented Sep 27, 2018

🐛 Bug Report

The asymmetric string matching throws an error on non-string values, which leads to tests that throw and fail before they can succeed.

To Reproduce

Take this example:

const foo = jest.fn();
foo(42);
foo('I am a valid string');

expect(foo).toHaveBeenCalledWith(expect.stringContaining('valid'));

It results in this error being thrown:

Actual is not a string

      1 |       foo('I am a valid string');
      2 |
    > 3 |       expect(foo).toHaveBeenCalledWith(expect.stringContaining('valid'));

Expected behavior

I would expect the test to succeed because the second call matches the expectation. If I use toHaveBeenLastCalled in the previous code then the test is green, so the matching as such seems to be accurate.

Link to repl or repo (highly encouraged)

I don't have a public link but the problem is related to this line: https://github.com/facebook/jest/blob/master/packages/expect/src/asymmetric_matchers.js#L221 ... In my opinion it is wrong to throw an Error here because it breaks the test in any case. Instead it should just return false and let the match fail for this single evaluation.

Run npx envinfo --preset jest

Paste the results here:

System:
    OS: macOS High Sierra 10.13.6
    CPU: x64 Intel(R) Core(TM) i7-7820HQ CPU @ 2.90GHz
Binaries:
    Node: 8.11.3 - ~/.nvm/versions/node/v8.11.3/bin/node
    Yarn: 1.7.0 - /usr/local/bin/yarn
    npm: 6.1.0 - ~/dev/webchannel-tracking-service/node_modules/.bin/npm
npmPackages:
    jest: ^23.5.0 => 23.5.0
@ryx
Copy link
Author

ryx commented Sep 27, 2018

Of course one could argue that it is bad practice to provide arguments with varying types to the same function. But I'd argue that it is not the test framework's responsibility to enforce strict type checking on function parameters.

@SimenB
Copy link
Member

SimenB commented Sep 27, 2018

I think it makes sense to throw in the asymmetric matcher, but we could catch that here: https://github.com/facebook/jest/blob/728ca114c06daa1c980d2d114142c8d6d3dd0dfb/packages/expect/src/spy_matchers.js#L185-L187
and partition it correctly. Wanna send a PR for that?

@pedrottimark
Copy link
Contributor

Ouch, #5021 still fails like this with the changes in #5069 that supposedly fixed it.

The original problem was calling other.includes(sample) when other is not a string.

I agree with @ryx about returning false: isA('String', other) && other.includes(sample)

Before we make the change, we need to think if it also makes sense for inverse matchers.

@pedrottimark
Copy link
Contributor

Forgot to include link to similar matcher in Jasmine which doesn’t throw, but now Jest does:

https://github.com/jasmine/jasmine/blob/master/src/core/asymmetric_equality/StringMatching.js#L11-L13

@pedrottimark
Copy link
Contributor

pedrottimark commented Sep 30, 2018

@ryx @SimenB /cc @thymikee

  • If y’all agree with the modified descriptions, I will submit PR with changes to docs, code, tests.
  • Especially because of changes to inverse matchers, do y’all think it is a breaking change?
assertion description
.stringContaining(string) matches the received value if it is a string that contains the exact expected string.
.not.stringContaining(string) matches the received value if it is not a string or if it is a string that does not contain the exact expected string.
.stringMatching(string | regexp) matches the received value if it is a string that matches the expected string or regular expression.
.not.stringMatching(string | regexp) matches the received value if it is not a string or if it is a string that does not match the expected string or regular expression.

For example, the following test throws an error now, but would pass according to proposed change:

test('.not.stringContaining', () => {
  const foo = jest.fn();
  foo(42);
  foo('I am a valid string');
  
  expect(foo).toHaveBeenCalledWith(expect.not.stringContaining('valid'));
});

EDITED P.S. This test would pass with assertions for both:

  • ordinary matcher for call with arg 'I am a valid string'
  • inverse matcher for call with arg 42

@SimenB
Copy link
Member

SimenB commented Sep 30, 2018

@rickhanlonii @mattphillips thoughts on ^? You're way better at APIs than I am

@pedrottimark
Copy link
Contributor

Is behavior of string asymmetric matchers when composed with function call and return matchers worth a new e2e-integration test?

@SimenB
Copy link
Member

SimenB commented Oct 2, 2018

Unless you wanna assert that the code frame is correct (which might be a good idea?) unit tests should suffice

@github-actions
Copy link

This issue 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 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants