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

[proposal] Fixture/snapshot tests - write in separate files #6383

Open
Andarist opened this issue Jun 3, 2018 · 23 comments
Open

[proposal] Fixture/snapshot tests - write in separate files #6383

Andarist opened this issue Jun 3, 2018 · 23 comments

Comments

@Andarist
Copy link
Contributor

Andarist commented Jun 3, 2018

🚀 Feature Proposal

Allow each fixture/snapshot test to be written to separate file WITHOUT wrapping snapshot output into exports property.

Motivation

It would be highly useful for projects like babel to treat their current fixture tests (actual.js / expected.js pairs) as Jest snapshots. This would allow integrating with jest more easily and using snapshot features for those tests (like -u)

Example

API would have to be decided, this is just a one proposal:

expect('test/fixtures/**/actual.js', ({ filename, code }) =>
  babel.transform(code, { 
    filename
  }).code
).toMatchFixtureSnapshot('expected.js')

Pitch

This with inline snapshot features would make a nice, complete package and cover most use cases.

@SimenB
Copy link
Member

SimenB commented Jun 3, 2018

Is the exports causing issues? I don't get why this new matcher would improve on the current approach

@Andarist
Copy link
Contributor Author

Andarist commented Jun 3, 2018

I don't get why this new matcher would improve on the current approach

The problem with current approach is that snapshots are colocated to the file that is doing an expect call. With fixtures such as in babel desired scenario is to keep expected snapshots colocated with input fixtures.

Is the exports causing issues?

Taking babel as an example. Keeping expected.js without exports wrapper is way nicer, because the output is actual code and when inspecting such an output file you get i.e. syntax highlighting for free.

@bfirsh
Copy link

bfirsh commented Aug 18, 2018

I also need this for Engrafo, a LaTeX to HTML converter. I use Jest snapshots to ensure the converted HTML is correct, but it would be really nice if the HTML snapshots were real files on disk so I could open them up in a browser, use them in other tools, etc.

@SimenB
Copy link
Member

SimenB commented Sep 15, 2018

Is it possible to implement this in userland, kinda like what https://github.com/americanexpress/jest-image-snapshot is doing? Something like this might prove useful for both transpilers and bundlers

Potential API if the matcher is implemented in user-land as toMatchFileSnapshot:

const inputFiles = glob
  .sync('test/fixtures/**/actual.js')
  .map(filename => ({ filename, code: fs.readFileSync(filename, 'utf8') }));

describe('transform files', () => {
  test.each(inputFiles)('testing my file', ({ filename, code }) => {
    const result = babel.transform(code, { filename }).code;

    expect(result).toMatchFileSnapshot();
  });
});

@satya164
Copy link
Contributor

I had the same use case and wrote this: https://github.com/satya164/jest-file-snapshot

The code was mostly from what jest-image-snapshot has. But I have to use this private API: https://github.com/satya164/jest-file-snapshot/blob/master/index.js#L19

Would be nice if I could implement it in userland without using the private API.

cc @thymikee

@Andarist
Copy link
Contributor Author

@satya164 this is amazing! i would still argue that this should be the part of the jest's core though, super useful feature for multitude of tooling

@SimenB
Copy link
Member

SimenB commented Sep 30, 2018

@satya164 This is great! What would you need in the snapshotstate to implement it the way you want?

@satya164
Copy link
Contributor

@Andarist it'll definitely be nice if it was in core.

@SimenB currently the only private API I'm using is snapshotState._updateSnapshot so that I know whether I should write the snapshot to disk. I think making it public should be enough for this.

@SimenB
Copy link
Member

SimenB commented Sep 30, 2018

@rickhanlonii thoughts on the API?

@rickhanlonii
Copy link
Member

@Andarist sorry for the delay

Just for clarity, can you add an example snapshot file output to OP? How would multiple snapshot tests in one test file work?

@satya164
Copy link
Contributor

satya164 commented Dec 2, 2018

For my use case, each test will have a different snapshot file regardless of where it is. The library I published requires you to specify the path of the file. Maybe it can also automatically determine a filename based on the title of the test in future.

I'm also writing a tool for testing babel plugins where you can specify a directory containing fixtures like so:

.
├── function-expression
│   ├── code.js
│   └── output.js
├── invalid-syntax
│   ├── code.js
│   └── error.js
└── simple-variable
    ├── code.js
    └── output.js

So the specifying the name of the file part is abstracted away.

What I need in the matcher is just an API to determine if we're updating a snapshot instead to avoid accessing private properties.

@rickhanlonii
Copy link
Member

Can you be more specific about what you need?

@satya164
Copy link
Contributor

satya164 commented Dec 2, 2018

@rickhanlonii
Copy link
Member

I'm cool with making that public if you want to submit the PR

@github-actions
Copy link

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

@github-actions github-actions bot added the Stale label Feb 25, 2022
@Jack-Works
Copy link

ping

@github-actions github-actions bot removed the Stale label Feb 26, 2022
@SimenB
Copy link
Member

SimenB commented Feb 26, 2022

We should stick links to https://github.com/americanexpress/jest-image-snapshot and https://github.com/satya164/jest-file-snapshot in https://jestjs.io/docs/snapshot-testing.

PR is still welcome to make _updateSnapshot public - probably under a more descriptive name?

@github-actions
Copy link

This issue is stale because it has been open for 1 year 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 Feb 26, 2023
@Andarist
Copy link
Contributor Author

It still would be cool to have this feature - bump for a stale bot

@github-actions github-actions bot removed the Stale label Feb 26, 2023
@mrazauskas
Copy link
Contributor

The requested is somewhat similar to #12734.

@seansfkelley
Copy link

seansfkelley commented Jul 26, 2023

I think this is more involved than "just publicize _updateSnapshot" for a number of reasons.

Returning to the initial request independent of jest-file-snapshot (which I use!), I think an effective implementation of this feature should at the very least:

  • be able to delete outdated snapshots (thus: it needs to have opinions about what directory they can be found in)
  • use the already-registered set of snapshot serializers
  • ignore these snapshots for file watching without any additional configuration

(jest-file-snapshot is a great bridge, but it can't do any of these things because Jest doesn't expose them/isn't aware these snapshots are a special thing.)

Additionally, to make this similarly ergonomic and pleasant to use as the existing snapshot matchers, I think it should also:

  • do something useful when the matcher is called with no arguments (thus: it needs to have opinions about what the snapshots are named by default)
  • permit specifying a file extension without also specifying the basename of the fixture
  • have a symmetrical interface with the existing snapshot matchers, including properties and hint

So here's a new proposal for how this could look.

// file.test.js
it('should do something', () => {
  // default case
  // __tests__/__snapshots__/file.test.js/should_do_something_1.snap
  expect(someValue).toMatchStandaloneSnapshot();

  // a la existing snapshot helpers: allow passing property matchers to fail fast
  // __tests__/__snapshots__/file.test.js/should_do_something_2.snap (2: second matcher in this test)
  expect(someValue).toMatchStandaloneSnapshot({ field: expect.any(Date) });

  // you can also specify the desired extension to assist other tools out of the box (e.g. syntax highlighters)
  // __tests__/__snapshots__/file.test.js/should_do_something_3.snap.xml
  expect(someValue).toMatchStandaloneSnapshot('xml');

  // matchers, then extension
  // __tests__/__snapshots__/file.test.js/should_do_something_4.snap.xml
  expect(someValue).toMatchStandaloneSnapshot({ field: expect.any(Date) }, 'xml');

  // you can also pass a filename hint
  // __tests__/__snapshots__/file.test.js/should_do_something_just_checking_5.snap.xml
  expect(someValue).toMatchStandaloneSnapshot('just checking', 'xml');

  // or all three arguments
  // __tests__/__snapshots__/file.test.js/should_do_something_just_checking_6.snap.xml
  expect(someValue).toMatchStandaloneSnapshot({ field: expect.any(Date) }, 'just checking', 'xml');

  // the numbering "namespace" is distinct between regular snapshots and standalone ones to avoid
  // unnecessary file churn if you rearrange the matchers relative to each other
  // __tests__/__snapshots__/file.test.js.snap > exports['should do something 1'] = ...;
  expect(someValue).toMatchSnapshot();
});

This proposal introduces no new configuration, in particular, there is no separate configuration for where standalone snapshots go. My understanding is that you can configure the snapshot directory, and then Jest is assumed to own all the files in there. This simplifying assumption is important to make cleaning up snapshots for deleted matchers work.

Jest can distinguish between the existing snapshots and the new standalone snapshots by how deep they are in the file tree: regular snapshots are in file.test.js.snap, and standalone snapshots are in the sibling directory file.test.js. Since Jest owns this directory, it knows that it can delete any files that didn't have a corresponding snapshot (when using -u). It's a bit weird that the directory has a file-like name, but I figure that's less confusing than any alternative like putting the snapshots somewhere else or slugifying the name to something like file_test_js.

(The matcher's name is, of course, subject to bikeshed. I avoided "fixture" since I think that can be a number of things, and it isn't immediately clear what would distinguish a "fixture"-type snapshot from an unqualified snapshot. I avoided "file" because regular snapshots also end up in a file.)

I poked around a preliminary implementation, but don't have anything to show yet. I'd like to get some opinions on this direction before I pursue this any farther.

@SimenB
Copy link
Member

SimenB commented Jul 27, 2023

I poked around a preliminary implementation, but don't have anything to show yet. I'd like to get some opinions on this direction before I pursue this any farther.

I think your ideas sounds great! Would love to see a PR exploring this.

Copy link

This issue is stale because it has been open for 1 year 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 26, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

9 participants