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

Add support for Jest Snapshots #5567

Merged
merged 3 commits into from
Sep 13, 2021
Merged

Add support for Jest Snapshots #5567

merged 3 commits into from
Sep 13, 2021

Conversation

Nixinova
Copy link
Contributor

@Nixinova Nixinova commented Sep 11, 2021

Description

Support Jest Snapshot files.
Basically JavaScript, but has its own tm-scope to support the highlighting inside `backticks`.
Classify these as generated.

Checklist

  • I am adding a new language.
    • The extension of the new language is used in hundreds of repositories on GitHub.com.
      • Search results for each extension:
    • I have included a real-world usage sample for all extensions added in this PR:
    • I have included a syntax highlighting grammar: source, preview (slightly broken)
    • I have updated the heuristics to distinguish my language from others using the same extension.

@Nixinova Nixinova requested a review from a team as a code owner September 11, 2021 05:33
Copy link
Collaborator

@Alhadis Alhadis left a comment

Choose a reason for hiding this comment

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

LGTM. 👍

@lildude lildude merged commit 971e152 into github-linguist:master Sep 13, 2021
kalkin added a commit to kalkin/file-expert that referenced this pull request Sep 19, 2021
@joscha
Copy link

joscha commented Nov 17, 2021

@lildude @Alhadis

We just noticed this change in a pull request review on github and I consider it a regression.

Snapshot tests are often used to accompany meaningful changes. They are more of a blueprint than a fixture in the sense that they can be autogenerated easily and thus may change within a pull request by just adding --update-snapshot to a jest run.
Such changes should not be considered correct, just because the file is generated automatically.

Quite the contrary, the changes in the snapshots are the meaningful change of the test fixture and are thus to be reviewed specifically. As such I believe it is incorrect to mark them as autogenerated by default, as linguist-generated is meant to reduce noise and improve meaningfulness when it comes to displaying a change set. Therefore this change is doing the contrary: it reduces meaninfulness of the displayed change set by hiding potentially vital changes in behaviour of business logic by not displaying the resulting changes in the snapshot fixture.
If (and it is questionable if that makes sense, as it more or less suggests that the snapshot content is meaningless and thus the snapshot change or the code producing the snapshot is meaningless) a snapshot should be hidden I believe this decision should be done on a per-project / repo / folder basis, not as a general change to Linguist's behaviour for all projects based on it. This should be easy by adding a .gitattributes file with **/*.snap linguist-generated=true for the projects that want this.

In github's case there is no way for us to opt out of the default behaviour on an org level for example, making this change even more grave.

@joscha
Copy link

joscha commented Nov 17, 2021

@Alhadis
Copy link
Collaborator

Alhadis commented Nov 17, 2021

@joscha Urgh. 😞 I'm so sorry about this. My memory—sharp as it is—isn't infallible, and relying on it to catch these types of scenarios isn't a good idea. We really need to add some sort of record to keep track of important reversions like this, as I predict something like this will happen again in future.

In the mean time, I'll revert the change again and also add a regression test to assert that Jest snapshots are not marked generated by default.

@joscha
Copy link

joscha commented Nov 17, 2021

Thank you @Alhadis!

Alhadis added a commit that referenced this pull request Nov 17, 2021
Alhadis added a commit that referenced this pull request Nov 25, 2021
@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.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for jest snapshots
4 participants