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

Amend jest snapshots to tie in nicely with Ava #2130

Merged
merged 1 commit into from Nov 29, 2016

Conversation

lithin
Copy link
Contributor

@lithin lithin commented Nov 18, 2016

Summary

I am currently working on implementation of jest snapshots into Ava (avajs/ava#1113) and wanted to smooth a few rough edges when implementing with another runner.

In particular:

  • jest-snapshots relies on a context that has dontThrow function - that however depends too much on the implementation
  • message on failure contains information about where the failure occurred; that way, the first line of the message says expect(value).toMatchSnapshot() - which again displays an implementation detail that isn't necessary

I'm happy to change the solution but it would be great to address these issues.

Test plan

When integrated with Ava, we get this kind of output:

screen shot 2016-11-18 at 2 16 55 pm

(Amazingly, when combined with nyc for test coverage, it works seamlessly without any additional work!)

@facebook-github-bot
Copy link
Contributor

Thank you for your pull request and welcome to our community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. In order for us to review and merge your code, please sign up at https://code.facebook.com/cla - and if you have received this in error or have any questions, please drop us a line at cla@fb.com. Thanks!

If you are contributing on behalf of someone else (eg your employer): the individual CLA is not sufficient - use https://developers.facebook.com/opensource/cla?type=company instead. Contact cla@fb.com if you have any questions.

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@codecov-io
Copy link

Current coverage is 88.32% (diff: 100%)

Merging #2130 into master will not change coverage

@@             master      #2130   diff @@
==========================================
  Files            38         38          
  Lines          1371       1371          
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
  Hits           1211       1211          
  Misses          160        160          
  Partials          0          0          

Powered by Codecov. Last update 6ca7542...ae98ed5

@cpojer
Copy link
Member

cpojer commented Nov 19, 2016

Hey! Thanks for this. I'll be out on vacation next week but happy to look after then.

() => matcherHint('.toMatchSnapshot', 'value', '') + '\n\n' +
`${RECEIVED_COLOR('Received value')} does not match ` +
const report =
() => `${RECEIVED_COLOR('Received value')} does not match ` +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not quite sure how to handle this particular bit. Report is very similar to the message - message only has one more line at the beginning that is implementation-specific.

I'm also confused that message is either a function or a string, depending on whether the test was successful or not.

Copy link
Member

Choose a reason for hiding this comment

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

We are going to change the interface for message to always be a function.

@lithin
Copy link
Contributor Author

lithin commented Nov 21, 2016

Thanks @cpojer, looking forward to your feedback :) Happy Thanksgiving!

@cpojer cpojer merged commit 7a36a0d into jestjs:master Nov 29, 2016
@cpojer
Copy link
Member

cpojer commented Nov 29, 2016

I'm not super excited about the dontThrow check but I'm also not feeling strongly about it so I went ahead and merged it.

@lithin
Copy link
Contributor Author

lithin commented Nov 29, 2016

Thank you @cpojer !

@cpojer
Copy link
Member

cpojer commented Nov 29, 2016

I'll publish a new version of Jest sometime this week once I get through all of these issues. Sorry for making you wait.

@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 14, 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.

None yet

4 participants