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

Fix pretty print for closeTo matcher #12626

Merged
merged 15 commits into from
Apr 14, 2022
Merged

Conversation

L4vlet
Copy link
Contributor

@L4vlet L4vlet commented Apr 2, 2022

Summary

This PR fixes print for the closeTo matcher. Currently, it breaks with the val.toAsymmetricMatcher is not a function error.

Test plan

  • Ran yarn test. Everything passed except for two tests related to Mercurial. They were as well failing on my machine before I made any changes
  • Wrote two additional unit-tests using objectContaining as an example

@mrazauskas
Copy link
Contributor

Good catch.

It could be good to add a test case without precision arg: expect.closeTo(4.23). And perhaps an edge case with Infinity and something like this as well: expect.closeTo(1.56e-3, 4).

Another though. If I get it right, Expected: NumberCloseTo 0.2345 (2 digit) means that a number close to 0.23 was expected? Wouldn’t it be enough to print: Expected: NumberCloseTo 0.23? Or something like: Expected: Number 0.23 ± 0.005?

@L4vlet
Copy link
Contributor Author

L4vlet commented Apr 3, 2022

Good note. Thank you. I've added more tests (infinity, scientific notation and without precision).

Regarding print format, I don't think it's a good idea to use the format you've suggested.

  1. We'll have to bring rounding code to the serialize function to correctly display an expected number
  2. We'll change the display of the input user has put into their tests (in comparison with their code) which may lead to a confusion

However, I'm not sure about my version either. I'd be glad to see any other proposals.

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.

Any particular reason why we cannot implement toAsymmetricMatcher on https://github.com/facebook/jest/blob/b82fd89d16fdbe74d9bc80dd86ae4912966adc1d/packages/expect/src/asymmetricMatchers.ts#L293? Instead of adding to pretty-format. Same for the existent types.

Also, we should throw a more useful error if toAsymmetricMatcher is not present - it's optional: https://github.com/facebook/jest/blob/b82fd89d16fdbe74d9bc80dd86ae4912966adc1d/packages/expect/src/asymmetricMatchers.ts#L84


I don't have strong opinions about the exact message when serialized btw, I think the current one seems fine?

@SimenB
Copy link
Member

SimenB commented Apr 8, 2022

ping @L4vlet 🙂

@L4vlet
Copy link
Contributor Author

L4vlet commented Apr 8, 2022

Hey, thank you for the ping. Got a bit busy.

I am not sure about the reasons why .toAsymmetricMatcher has not been implemented before. I am not that familiar with the internals yet. Let me try to check if there was a reason for that

@L4vlet
Copy link
Contributor Author

L4vlet commented Apr 9, 2022

Any particular reason why we cannot implement toAsymmetricMatcher on ... ? Instead of adding to pretty-format. Same for the existent types.

If I got it right, originally (PR) it was implemented that way. However, later author decided to create a plugin for the pretty-print to

  • remove complex printing code
  • support various client settings (spaces, identation and so on) which makes sense for complex structures (Arrays, Objects)

In our case, it's not that necessary as printing is relatively simple. We can put it inside .toAssymetricMatcher as we do for the Any matcher.

Also, we should throw a more useful error if toAsymmetricMatcher is not present - it's optional:

I suggest we write a test which will check all matchers to see if jest can correctly print them. Not sure about the special error handling, though. It is not a kind of error client can fix by themselves so there won't be much use from a pretty message.

@SimenB
Copy link
Member

SimenB commented Apr 9, 2022

Error handling would be Asymmetric matcher *name* does not implement `toAsymmetricMatcher` instead of val.toAsymmetricMatcher is not a function

@L4vlet L4vlet changed the base branch from main to gh-pages April 10, 2022 17:20
@L4vlet L4vlet changed the base branch from gh-pages to main April 10, 2022 17:20
@L4vlet L4vlet requested a review from SimenB April 10, 2022 17:20
@L4vlet
Copy link
Contributor Author

L4vlet commented Apr 10, 2022

@SimenB, good time of the day. Could you take a look again, please? I've moved the formatting from pretty-print to class function and added error handling.

As for the test I was going to write, I couldn't find a proper way to extract all AsymmetricMatcher-s from the expect object. Maybe you have an idea of how to do that?

@L4vlet L4vlet requested a review from SimenB April 14, 2022 08:30
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!

@SimenB SimenB merged commit 045367a into jestjs:main Apr 14, 2022
@SimenB
Copy link
Member

SimenB commented Apr 19, 2022

@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 20, 2022
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.

4 participants