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

feat(jest-editor-snapshot): Add Snapshots metadata #4570

Merged
merged 4 commits into from
Sep 30, 2017

Conversation

vvo
Copy link
Contributor

@vvo vvo commented Sep 30, 2017

Summary

Following #2629, I rebased the code on current master. I still have to
actually test the feature with jest-community/vscode-jest#73.

What i would like to add ultimately is the ability to:

  • preview the snapshot
  • have the snapshot diff
  • have a button to accept diff per snapshot (would update it)

Changes from #2629 to rebase on master are in ff657a0

WDYT? cc @orta @bookman25

PS: first ever contribution here, looking forward to feedback/comment/requests for change

Test plan

Same as in #2629

Cristian Carlesso and others added 4 commits September 30, 2017 13:56
Following jestjs#2629, I rebased the code on current master. I still have to
actually test the feature with jest-community/vscode-jest#73.

What i would like to add ultimately is the ability to:
- preview the snapshot
- have the snapshot diff
- have a button to accept diff per snapshot (would update it)

WDYT?
@codecov-io
Copy link

Codecov Report

Merging #4570 into master will decrease coverage by 0.49%.
The diff coverage is 7.81%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #4570     +/-   ##
=========================================
- Coverage   56.16%   55.66%   -0.5%     
=========================================
  Files         185      186      +1     
  Lines        6285     6345     +60     
  Branches        3        3             
=========================================
+ Hits         3530     3532      +2     
- Misses       2754     2812     +58     
  Partials        1        1
Impacted Files Coverage Δ
packages/jest-editor-support/src/Snapshot.js 0% <0%> (ø)
packages/jest-snapshot/src/index.js 42.22% <0%> (ø) ⬆️
packages/jest-editor-support/src/index.js 0% <0%> (ø) ⬆️
.../jest-editor-support/src/parsers/babylon_parser.js 98.61% <100%> (+0.03%) ⬆️

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 9af7bef...ff657a0. Read the comment docs.

@orta
Copy link
Member

orta commented Sep 30, 2017

I don't have any feedback here, this looks really solid - great job. babel-traverse is 👍.

It's worth keeping in mind the assumption that only people using babel will have these snapshots ( e.g. TypeScript users won't reliably get results ) which is fine, so the results should be considered nullable. With Babel 7 we can consolidate those code-paths in the future.

const describeVariants = Object.assign(
(Object.create(null): {[string]: boolean}),
{
describe: true,
Copy link
Member

Choose a reason for hiding this comment

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

I feel like it would be good to put all this in a common package with eslint-plugin-jest, because we are duplicating all of this in multiple places.

@cpojer cpojer merged commit e81f1fb into jestjs:master Sep 30, 2017
@cpojer
Copy link
Member

cpojer commented Sep 30, 2017

Let's do it, I think the last commits + this rebase are addressing most of the comments I have. Let's fix the remaining things as we go.

I didn't realize there was a PR for vscode-jest already, looking forward to seeing it work end-to-end!

Not sure how far you are willing to take this feature – are you planning to work on it beyond this initial integration of snapshots into the editor? I think there are tons more snapshot related things we could do inside the editor without starting up Jest. One of the things that would be great to have is a snapshot approval mode, where in the editor you can accept/decline updates to individual snapshots, one-by-one.

@vvo
Copy link
Contributor Author

vvo commented Sep 30, 2017

One of the things that would be great to have is a snapshot approval mode, where in the editor you can accept/decline updates to individual snapshots, one-by-one.

@vvo
Copy link
Contributor Author

vvo commented Sep 30, 2017

^ exactly what I was looking for and willing to do so yes planning to contribute more soon enough

tabrindle pushed a commit to tabrindle/jest that referenced this pull request Oct 2, 2017
* first iteration

* Use babel-traverse

* comments on the pr

* feat(jest-editor-support): Add Snapshot metadata

Following jestjs#2629, I rebased the code on current master. I still have to
actually test the feature with jest-community/vscode-jest#73.

What i would like to add ultimately is the ability to:
- preview the snapshot
- have the snapshot diff
- have a button to accept diff per snapshot (would update it)

WDYT?
@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 13, 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.

5 participants