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

[Feature]: opt-in one-snapshot-per-file #13951

Closed
seansfkelley opened this issue Feb 24, 2023 · 14 comments
Closed

[Feature]: opt-in one-snapshot-per-file #13951

seansfkelley opened this issue Feb 24, 2023 · 14 comments

Comments

@seansfkelley
Copy link

seansfkelley commented Feb 24, 2023

🚀 Feature Proposal

(This is a revival of #2676, which was auto-closed.)

A new expectation, toMatchIndependentSnapshot, that puts the corresponding snapshot in its own file, rather than every test in the same file dumping into the same file. Additionally, each file would be the raw serialized content, not wrapped in exports[...] =.

Motivation

Sometimes snapshots are unavoidably large:

Further, large snapshots being wrapped in exports[...] = `...` means that tooling that could help understand the snapshot -- say, XML-aware code folding or XPath navigation -- does not work.

At the moment, I basically throw up my hands and skim changes to large snapshots, generally assuming that if my random samples of the changes look good, then the whole snapshot (or changes to it) are good.

Tests with snapshot this large are typically "integration" tests (in the sense that they attempt to exercise large portions of code, not necessarily that they require running services). I can, and have, simplified many of the tests into small unit tests, sometimes even eliminating snapshotting entirely. But these new tests serve a different purpose, and not all large-snapshot tests can be effectively replaced by downsized unit tests.

Example

(Note: I'm not married to any specific names or call styles, I just picked some to have something to talk about.)

Here is a notionalized example that I'm working on. The context is manipulation of DOCX files, for which we convert the input files to an internal format. DOCX files are composed of many XML files internally and require a lot of bookkeeping to make sure the various files are in agreement, so we have tests to make sure we're parsing and manipulating them in a way that is consistent across all the contained XML.

// docxfile.test.ts
it('should apply edits that delete comments', () => {
  const fileWithComments: DocxFile = await loadDocxFile('comments.docx');
  const edits: Edit[] = await loadEdits('comments-edits.json');
  fileWithComments.applyEdits(edits);
  expect(fileWithComments.document).toMatchIndependentSnapshot();
  expect(fileWithComments.comments).toMatchIndependentSnapshot();
  // there's actually 3 more comments files but I won't bore you
});

edits is a list of edits that a user might perform to the DOCX file, including ones that delete comments in the file. Comments themselves are spread over several of the internal XML files. While this test could be written with a whole bunch of assertions looking for this comment ID there, or that snippet of text there, such a test is much more work and, put simply, unlikely to be written in the first place. Also, this particular test notionalizes well but is far from the most involved.

Coupled with a custom serializer that prints XML, I would want to end up with something in __snapshots__/docxfile.test/should_apply_edits_that_delete_comments/1.snap, which would contain something like:

<w:document ...>
  <!-- you get the idea -->
</w:document>

Similarly, 2.snap would contain the comments XML, etc.

As a workaround, one could theoretically split a test like this up into multiple files and just run the same test body, but with a different assertion, in each file. But this is a lot of overhead both in developer time and test runtime to achieve an inferior result (in that the files still have the exports wrapping).

Pitch

The only existing prior art I've seen for this is https://github.com/igor-dv/jest-specific-snapshot. By its own admission it's quite hacky, using private APIs and not correctly interacting with e.g. custom serializers. It also still prints files in exports[...] = style, so it doesn't help external tools. It also specifically warns you off of using .snap to avoid confusing Jest, which is a footgun.

It seems that the only way to implement the feature is in Jest core, or to have Jest expose more information about snapshots so this could be implemented with a plugin.

@nutchanon-c
Copy link

nutchanon-c commented Mar 17, 2023

I know that this may be raised before but could doing this potentially solve the issues of concurrent tests as well? Sounds like a proper implementation of a multi-snapshots-per-file approach could potentially solve this issue as well. I thought jest-specific-snapshot was going to solve concurrency issues but it does not. Would love to be able to run in concurrent.

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Apr 16, 2023
@seansfkelley
Copy link
Author

.

@mrazauskas
Copy link
Contributor

Did you try jest-file-snapshot?

@github-actions github-actions bot removed the Stale label Apr 16, 2023
@seansfkelley
Copy link
Author

seansfkelley commented Apr 24, 2023

Ah, I had not found that package anywhere. It gets the job done better than jest-specific-snapshot, but still has a number of flaws:

  • it hooks into Jest's private internals to check for whether snapshots are being updated (Jest does not seem to expose this)
  • unlike proper Jest snapshots, it can't detect obsolete ones and won't delete ones whose tests have been removed
  • it isn't aware of the serializers available to Jest, so you have to serialize to a string/buffer manually (unsure if Jest exposes this or not)

Despite all this, the end result is very usable and the downsides are very manageable, and I've started using it. However, I still think this is something that Jest should support first-class, as there are a number of footguns (listed above) that seem to have no resolution without Jest's involvement.

@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label May 26, 2023
@seansfkelley
Copy link
Author

.

@github-actions github-actions bot removed the Stale label May 26, 2023
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jun 25, 2023
@seansfkelley
Copy link
Author

.

@github-actions github-actions bot removed the Stale label Jun 25, 2023
@github-actions
Copy link

This issue is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 30 days.

@github-actions github-actions bot added the Stale label Jul 25, 2023
@seansfkelley
Copy link
Author

It would be nice if a human commented on this ticket instead of a robot.

@github-actions github-actions bot removed the Stale label Jul 25, 2023
@mrazauskas
Copy link
Contributor

Ah, I had not found that package anywhere. It gets the job done better than jest-specific-snapshot, but still has a number of flaws:

  • it hooks into Jest's private internals to check for whether snapshots are being updated (Jest does not seem to expose this)
  • unlike proper Jest snapshots, it can't detect obsolete ones and won't delete ones whose tests have been removed
  • it isn't aware of the serializers available to Jest, so you have to serialize to a string/buffer manually (unsure if Jest exposes this or not)

Despite all this, the end result is very usable and the downsides are very manageable, and I've started using it. However, I still think this is something that Jest should support first-class, as there are a number of footguns (listed above) that seem to have no resolution without Jest's involvement.

You see the problems very well. Jest is open source project, feel free to find solutions and implement the feature.

It was long ago agreed that the needed private method can become public. See #6383

Also a feature request to integrate jest-file-snapshot is open for some time. See #12734

I browsed around to find answers for you, so I think to close this as duplicate of #12734

@seansfkelley
Copy link
Author

Thanks. I didn't turn any of that up when I first filed the issue.

FWIW I really appreciated the communication on the pinned issue at #12496:

That said, I understand it's frustrating to see your issue or PR you spent time putting together, and which might still be relevant, be closed as stale by some stupid bot. So feel free to provide feedback here in this issue if there's anything in the process we can improve!

But ironically that ticket was locked so I couldn't provide feedback that the system wasn't working.

Jest is open source project, feel free to find solutions and implement the feature.

A feature request is not a request to drop everything right now and fix the problem. I didn't mind having the ticket left open; I have had my years-old tickets get picked up by another contributor, or even done the same myself. I did mind being told to shut up and go away by an automated process with no recourse except to go out of my way to annoy the maintainers.

Anyway, thanks for finding the relevant issues that I didn't. I'll chime in there if relevant.

@github-actions
Copy link

This issue 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 Aug 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants