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

Make jest-snapshot less jest/jasmine-specific #1413

Closed
wants to merge 4 commits into from

Conversation

suchipi
Copy link
Contributor

@suchipi suchipi commented Aug 12, 2016

WIP - DO NOT MERGE (yet)

Work in progress as per discussion in #1341. I still need to add some tests (around processSnapshot (aka the "raw matcher") and createSnapshotState), but wanted to get some feedback first.

  • Adds three new named exports to jest-snapshot: processSnapshot, createSnapshotState, and patchJasmine
    • processSnapshot performs all the business logic of creating, updating, and comparing snapshots, then returns either {pass: true} or {pass: false, expected: any}.
    • createSnapshotState creates a SnapshotState object for a given file path
    • patchJasmine is the same function that was previously defined in index.js
  • Removes the getSnapshotState export from jest-snapshot, which created a snapshot state and then patched jasmine with it.
  • Updates jest-jasmine2 to use createSnapshotState and patchJasmine instead of getSnapshotState.

Edit 8/22/16: I changed the interface of processSnapshot a bit from my original PR, so I have updated the description to match.

@ghost ghost added the CLA Signed ✔️ label Aug 12, 2016
@suchipi
Copy link
Contributor Author

suchipi commented Aug 22, 2016

Sorry I haven't rebased this yet, I'll try to get to that sometime this week.

@suchipi suchipi force-pushed the make-less-jasmine-specific branch from 7ef891b to 78e2db7 Compare August 23, 2016 02:10
@ghost ghost added the CLA Signed ✔️ label Aug 23, 2016
@codecov-io
Copy link

codecov-io commented Aug 23, 2016

Current coverage is 63.31% (diff: 16.66%)

Merging #1413 into master will increase coverage by <.01%

@@             master      #1413   diff @@
==========================================
  Files            96         98     +2   
  Lines          3608       3617     +9   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2284       2290     +6   
- Misses         1324       1327     +3   
  Partials          0          0          

Powered by Codecov. Last update 081bdd5...78e2db7

@suchipi
Copy link
Contributor Author

suchipi commented Aug 26, 2016

This is ready for review 😃

@turadg
Copy link

turadg commented Sep 5, 2016

LGTM. I'm looking forward to this merged to use snapshots with plain Jasmine and https://github.com/turadg/lazyspec

@suchipi
Copy link
Contributor Author

suchipi commented Sep 9, 2016

@cpojer Is there still interest in this effort? There are some conflicts with #1499 which seems to have more momentum than this PR. Can you advise?

@cpojer
Copy link
Member

cpojer commented Sep 9, 2016

@suchipi yeah absolutely, we just had absolutely no time at all to think about this or review code. I'm sorry. We'll get back to this soon and are working on merging the snapshot feature into the new matchers and I'll take a look at your PR.

@ghost ghost added the CLA Signed ✔️ label Sep 9, 2016
@suchipi
Copy link
Contributor Author

suchipi commented Sep 9, 2016

No worries, I understand you guys are doing a lot right now. 👍

@cpojer
Copy link
Member

cpojer commented Sep 17, 2016

Hey @suchipi,

I'm sorry I let this diff sit for an entire month. This is not ok and not our usual behavior. We were occupied with the Jest 15 release and @DmitriiAbramov took on the work to rewrite our snapshot implementation to work with Jest and also to work better with external frameworks. It goes a bit further than this diff here and lays the groundwork for future additions to snapshots. See #1721.

This means that your PR is no longer applicable to Jest. I apologize that we aren't going to merge your code even though you did all this work. I hope we don't lose you as a future contributor given that the work @DmitriiAbramov did was entirely inspired by the discussion you started. I hope to have more discussions with you on where to take the snapshot feature in the future. You are welcome to join us on discord (see facebook.github.io/jest/support.html )

@suchipi
Copy link
Contributor Author

suchipi commented Sep 17, 2016

No worries; what is right is more important than who is right. I'm not offended :)

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

4 participants