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

toMatchFileSnapshot does not detect obsolete snapshots #5094

Closed
6 tasks done
HansAarneLiblik opened this issue Feb 1, 2024 · 4 comments
Closed
6 tasks done

toMatchFileSnapshot does not detect obsolete snapshots #5094

HansAarneLiblik opened this issue Feb 1, 2024 · 4 comments

Comments

@HansAarneLiblik
Copy link

Describe the bug

I'm switching over from Jest to Vitest and we are using Storybook snapshots. They have an excellent guide on how to add snapshot testing using vitest - https://storybook.js.org/docs/writing-tests/storyshots-migration-guide#vitest

Since I have ~630 snapshots, I want them to be in separate files (Even for the same component) named like

  • MyComponent.stories.tsx has 2 stories Error and Loading which would generate the following storyshots
    • MyComponent.Error.storyshot
    • MyComponent.Loading.storyshot
  • MagicInput.stories.tsx has 2 stories Invalid and Valid which would generate the following storyshots
    • MagicInput.Invalid.storyshot
    • MagicInput.Valid.storyshot

These *.stories.tsx files are gathered by storybook.test.tsx and made into storyshots.

If new stories appear, new storyshot files are generated successfully via

const snapshotPath = path.join(storyDir, options.snapshotsDirName, `${componentName}.${name}.storyshot`);
expect(rendered).toMatchFileSnapshot(snapshotPath)

but if some stories are

  • renamed
  • deleted
  • component removed & stories.tsx file removed,

then the useless / obsolete storyshot files are not being detected & reported or removed

While the above is a description of a very specific usecase, imagine the following (Also available as a reproduction)

import { assert, describe, expect, it } from 'vitest';

describe('Test snapshots', () => {
  it('Should generate snapshot - file', () => {
    expect('lalala').toMatchFileSnapshot('lalala-1.storyshot');
  });
  it('Should generate snapshot - generic', () => {
    expect('generic-lalala').toMatchSnapshot();
  });

  // Comment these tests out.
  // `lalala-2.storyshot` IS NOT removed
  it('Should generate snapshot 2 - file', () => {
    expect('lalala2').toMatchFileSnapshot('lalala-2.storyshot');
  });
  // `Should generate snapshot 2 - generic` IS removed from `suite.test.ts.snap`
  it('Should generate snapshot 2 - generic', () => {
    expect('generic-lalala2').toMatchSnapshot();
  });
});

If I comment out my last 2 tests, the generic snapshot is removed from suite.test.ts.snap while lalala-2.storyshot is NOT removed and snapshot file still exists

Reproduction

https://stackblitz.com/edit/vitest-dev-vitest-opw4bq?file=test%2Fsuite.test.ts

System Info

System:
    OS: macOS 14.3
    CPU: (12) arm64 Apple M3 Pro
    Memory: 56.38 MB / 36.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 20.11.0 - ~/.nvm/versions/node/v20.11.0/bin/node
    Yarn: 1.22.21 - ~/.nvm/versions/node/v20.11.0/bin/yarn
    npm: 10.2.4 - ~/.nvm/versions/node/v20.11.0/bin/npm
  Browsers:
    Chrome: 121.0.6167.139
    Safari: 17.3
  npmPackages:
    @vitejs/plugin-react-swc: 3.5.0 => 3.5.0 
    @vitest/coverage-v8: 1.2.1 => 1.2.1 
    vite: 5.0.12 => 5.0.12 
    vitest: 1.2.1 => 1.2.1

Used Package Manager

yarn

Validations

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 6, 2024

Hey, thanks for raising an issue (or could it be a feature request?)

I'm not familiar with general storybook/storyshots practice, so can you clarify how you were setting up separate storyshots on Jest?

Storybook's documentation seems to suggest using https://github.com/igor-dv/jest-specific-snapshot, but from a quick look, it doesn't support cleaning up old snapshots either? It looks like Jest team also had this concern and that's the reason Jest doesn't have this feature builtin jestjs/jest#4231 (comment), jestjs/jest#2676 (comment)

(Digging further, similar feature requests are still tracked in jestjs/jest#12734 and jestjs/jest#6383)

@HansAarneLiblik
Copy link
Author

Hi!

Thanks for taking a look!

It seems that Storybook had implemented its own IntegrityTest, to detect (and remove) abandoned storyshots https://github.com/storybookjs/storybook/blob/v7.5.0/code/addons/storyshots-core/src/api/integrityTestTemplate.ts

This seems to be missing from the new Vitest integration (or at least I can't find it) so I implemented this myself!

@hi-ogawa
Copy link
Contributor

hi-ogawa commented Feb 6, 2024

I see, so storybook is deprecating storyshots-core and recommending people to migrate to Vitest as a one of options, but I suppose Vitest's toMatchFileSnapshot might not be as convenient as what they think

From Vitest perspective, I don't think it's possible to make general use case of toMatchFileSnapshot to be aware of obsolete snapshots. So, like storybook did in IntegrityTest, it probably requires some custom solution based on some globing and special file extension.

Btw, in their guide https://storybook.js.org/docs/writing-tests/storyshots-migration-guide, they say they want feedback about migration, so maybe it might worth raising a issue on their repo as well?

@sheremet-va
Copy link
Member

From my understanding, it is impossible to track custom-named snapshots, so we cannot remove obsolete ones since they are not referenced anywhere anymore. I would recommend using a resolveSnapshotPath config option instead.

@github-actions github-actions bot locked and limited conversation to collaborators Feb 21, 2024
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