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.

Sign up for GitHub

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

Jump to bottom

Check generated Jest snap file #3874

Merged
merged 3 commits into from
Jan 11, 2018
Merged

Check generated Jest snap file #3874

merged 3 commits into from
Jan 11, 2018
+42 −0

Conversation

yuya-takeyama
Copy link
Contributor

This is to mark snapshot files generated by Jest (a testing framework by Facebook) as generated.
I want such files to be collapsed in GitHub diff.

ref: https://daveceddia.com/snapshot-testing-react-with-jest/

btw, does this project have a guideline for adding generated file rules?

pchaigno
pchaigno suggested changes Oct 23, 2017
View reviewed changes
Copy link
Contributor

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

@yuya-takeyama Thanks for the pull request!
I left a few comments to address inline.

We don't already have guidelines for adding generated entries. We should probably write some, even if it's simple.

@lildude There are only 195 such files (but well distributed: 1-2 per repository). Is that enough? The rules run for every single file, so we probably want to impose some adoption requirement here as well...

lib/linguist/generated.rb Outdated
@@ -82,7 +82,8 @@ def generated?
generated_roxygen2? ||
generated_jison? ||
generated_yarn_lock? ||
generated_grpc_cpp?
generated_grpc_cpp? ||
generated_jest_snap?
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you add it after generated_by_zephir? in the list? We prefer to place first the rules that don't require opening the file (to open it only if necessary).

lib/linguist/generated.rb Outdated
#
# Return true or false
def generated_jest_snap?
name.include?("__snapshots__") && name.end_with?(".snap")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you switch the two predicates? I expect the extension to be a better (and cheaper) discriminator than the path.

@yuya-takeyama
Copy link
Contributor Author

@pchaigno
Thanks for the quick reaction!
I addressed the 2 points, thanks.

@lildude
Copy link
Member

lildude commented Jan 10, 2018

@pchaigno nudgy nudge nudge 😉

@pchaigno
Copy link
Contributor

@lildude There are only 195 such files (but well distributed: 1-2 per repository). Is that enough? The rules run for every single file, so we probably want to impose some adoption requirement here as well...

@lildude nudgy nudge nudge back 😜

@lildude
Copy link
Member

lildude commented Jan 10, 2018

Ooops. Missed my own ping 😊

Looks like we're up to 249 files. Still not really popular enough. Tagging as such.

@yuya-takeyama
Copy link
Contributor Author

@pchaigno @lildude
Looks like the search query is not correct.
__snapshots__ directories can not be in the root of the project.

Found 64k files in this query.
https://github.com/search?utf8=%E2%9C%93&q=extension%3Asnap+Jest+Snapshot&type=Code

@pchaigno
Copy link
Contributor

Uh, you're right. I updated mine and found ~150K files matching. (Looks like path takes an absolute path whereas in:path doesn't.) Let's go for this then!

lildude
lildude approved these changes Jan 11, 2018
View reviewed changes
Copy link
Member

@lildude lildude left a comment

Choose a reason for hiding this comment

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

And with the correct search string we get the popularity we need. Thanks for the correction and additions.

Welcome to Linguist.

@lildude lildude merged commit ca71434 into github-linguist:master Jan 11, 2018
@yuya-takeyama yuya-takeyama deleted the ignore-jest-snapshot-files branch January 11, 2018 09:25
@yuya-takeyama
Copy link
Contributor Author

Thank you! 🎉 😂

@lildude
Copy link
Member

lildude commented Jan 12, 2018

Oh 💩 I'm going to have to revert this PR. Sorry @yuya-takeyama.

We marked Jest snapshot files as generated before in #3572 and had to revert it in #3579 following feedback from the community. This PR has effectively re-introduced the first PR and I completely forgot and missed it.

We have #3584 open for discussion, though that discussion seems to have stalled. Please feel free to wake it up by commenting in that PR.

If you want snapshots in your projects to be collapsed in diffs, please use the linguist-generated override.

lildude added a commit that referenced this pull request Jan 12, 2018
@lildude
Revert "Check generated Jest snap file (#3874)"
d972805 
This reverts commit ca71434.
@lildude lildude mentioned this pull request Jan 12, 2018
Merged
@pchaigno
Copy link
Contributor

Arf! I forgot that and did not check :-/

lildude added a commit that referenced this pull request Jan 12, 2018
@lildude
Revert "Check generated Jest snap file" (#3984)
4340234 
* Revert "Remove Arduino as a language (#3933)"

This reverts commit 8e628ec.

* Revert "Check generated Jest snap file (#3874)"

This reverts commit ca71434.
@lildude lildude mentioned this pull request Jan 26, 2018
Merged
@joscha joscha mentioned this pull request Nov 17, 2021
Merged
5 tasks
Alhadis added a commit that referenced this pull request Nov 17, 2021
@Alhadis
Stop flagging Jest snapshots as generated files
4c04414 
See: #5567 (comment)

References: #3579, #3874, #4507
Alhadis added a commit that referenced this pull request Nov 25, 2021
@Alhadis
Stop flagging Jest snapshots as generated files (#5646)
68631fb 
See: #5567 (comment)

References: #3579, #3874, #4507
@github-linguist github-linguist locked as resolved and limited conversation to collaborators Jun 17, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lildude lildude lildude approved these changes

@pchaigno pchaigno pchaigno approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@yuya-takeyama @lildude @pchaigno