Add resolver for custom snapshots paths #6143

viddo · 2018-05-06T18:34:41Z

Summary

Provides a solution to #1650, by adding a configuration option to define a custom module that can resolve test<->snapshot paths. This enables people to decide where the snapshot files gets written on disk.

The implementation was based on this suggestion: #1650 (comment)

I left the code to still assume .snap extension on the snapshot files however, since I wasn't convinced how useful it would be to override that too. Could be fairly easily solved by consolidating the EXTENSION usages.

Motivation

My primary motivation for this PR stems from constraints by the Bazel build system, as also explained on a comment on Add support for --preserve-symlinks:

"Bazel runs build tools and binaries inside sandboxes, temporary directories …"

The sandbox is discarded after the test run, so any written files that are wanted afterwards must be written elsewhere (special directories also provided by Bazel, as it doesn't allow to write wherever you want outside the sandbox).

Judging by the reactions there are probably also other use-cases and rationales that motivates this feature.

Test plan

Existing tests covers the default resolver behavior well. I've added some new unit and integration tests to cover the configurable part.

I could add some more test cases of course, but wanted to get some initial feedback before spending more time on this.

SimenB · 2018-05-06T18:48:20Z

CI is failing, mind looking into that test?

viddo · 2018-05-06T19:35:41Z

Yes, will do. Haven't managed to reproduce with neither yarn test, yarn run test-ci, or yarn run test-ci-partial yet though, it passes locally (Mac OSX 11.12, Node v8.11), so I guess something else that differs. Will continue to investigate...

website/versioned_docs/version-22.4/Configuration.md

packages/jest-snapshot/src/snapshot_resolver.js

thymikee · 2018-05-06T21:55:13Z

packages/jest-snapshot/src/snapshot_resolver.js

+
+  if (typeof custom.resolveSnapshotPath !== 'function') {
+    throw new TypeError(
+      'snapshotResolver does not have a resolveSnapshotPath function',


How about:

Custom snapshot resolver must implement a `resolveSnapshotPath` function. Documentation: https://facebook.github.io/jest/docs/en/configuration.html#snapshotResolver

sidferreira · 2018-05-07T14:04:13Z

@viddo What if, instead of picking a resolver to the snapshots, the resolver resolves the Snapshot module? By default returning JestSnapshot but allowing us to override all the JestSnapshot at our own risks?

viddo · 2018-05-07T18:56:44Z

@viddo What if, instead of picking a resolver to the snapshots, the resolver resolves the Snapshot module? By default returning JestSnapshot but allowing us to override all the JestSnapshot at our own risks?

@sidferreira, I'm not sure what you refer to by Snapshot module and JestSnapshot. Are you referring to the whole jest-snapshot package export? The SnapshotState class? Something else?

Either way, I'm reluctant to expose more internals than necessary, as it makes it harder to change those in the future as they practically then becomes a "public API" and affects the semver.

Also, while I don't mind changing the implementation, the core team have the final say so I leave it to them to answer this.

viddo · 2018-05-11T02:18:47Z

Finally managed to get my hands on a Windows computer so I could reproduce and fix the failing tests.

Any more feedback or thoughts on this?

codecov-io · 2018-05-11T02:21:22Z

Codecov Report

Merging #6143 into master will decrease coverage by 3.51%.
The diff coverage is 71.11%.

@@            Coverage Diff             @@
##           master    #6143      +/-   ##
==========================================
- Coverage   66.97%   63.45%   -3.52%     
==========================================
  Files         250      236      -14     
  Lines       10355     9137    -1218     
  Branches        3        3              
==========================================
- Hits         6935     5798    -1137     
+ Misses       3419     3338      -81     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-config/src/ValidConfig.js	`100% <ø> (ø)`	⬆️
packages/jest-config/src/index.js	`37.73% <ø> (ø)`	⬆️
packages/jest-config/src/normalize.js	`91.33% <ø> (ø)`	⬆️
packages/jest-snapshot/src/utils.js	`100% <ø> (ø)`	⬆️
packages/jest-cli/src/lib/is_valid_path.js	`100% <ø> (ø)`	⬆️
packages/jest-cli/src/TestScheduler.js	`38.34% <ø> (-5.93%)`	⬇️
.../src/legacy_code_todo_rewrite/jest_adapter_init.js	`0% <0%> (ø)`	⬆️
packages/jest-cli/src/SearchSource.js	`43.36% <0%> (-1.29%)`	⬇️
packages/jest-jasmine2/src/setup_jest_globals.js	`0% <0%> (ø)`	⬆️
packages/jest-snapshot/src/State.js	`0% <0%> (ø)`	⬆️
... and 29 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 21e35cc...612b178. Read the comment docs.

SimenB

This needs an entry on the changelog

Left some test nits

SimenB · 2018-05-22T12:39:13Z

integration-tests/__tests__/snapshot_resolver.test.js

+
+const snapshotDir = path.resolve(
+  __dirname,
+  path.join('..', 'snapshot-resolver', '__snapshots__'),


path.resolve normalizes, so just path.resolve(__dirname, '../snapshot-resolver/__snapshots__'); should be enough

SimenB · 2018-05-22T12:40:25Z

integration-tests/__tests__/snapshot_resolver.test.js

+
+const fileExists = filePath => {
+  try {
+    return fs.statSync(filePath).isFile();


fs.existsSync is not deprecated, please use it (async is deprecated)

SimenB · 2018-05-22T12:40:58Z

integration-tests/__tests__/snapshot_resolver.test.js

+
+describe('Custom snapshot resolver', () => {
+  const cleanup = () => {
+    [snapshotFile].forEach(file => {


should just drop the array here

SimenB · 2018-05-22T12:41:28Z

integration-tests/__tests__/snapshot_resolver.test.js

+
+    // $FlowFixMe dynamic require
+    const content = require(snapshotFile);
+    expect(content['snapshots are written to custom location 1']).not.toBe(


expect(content['snapshots are written to custom location 1']).toBeDefined()

SimenB · 2018-05-22T12:43:16Z

packages/jest-snapshot/src/__tests__/snapshot_resolver.test.js

@@ -0,0 +1,100 @@
+'use strict';


genereal comment, path.resolve normalizes, so you don't need to comma separate the varargs

👍 better now?

…realized I only fixed it on the other files, also updated this one now.

SimenB · 2018-05-22T12:44:00Z

packages/jest-snapshot/src/snapshot_resolver.js

+
+function mustImplement(functionName: string) {
+  return (
+    `Custom snapshot resolver must implement a \`${functionName}\` function.\n\n` +


should the heading be bold?

SimenB · 2018-05-22T12:45:14Z

Also, please rebase to ensure the snapshots are correct. I have a feeling some trailing newlines are gone, so it would fail when merged to master

viddo · 2018-05-22T23:37:20Z

Will do, thanks for the additional review!
edit: ~~probably will have time to address the review comments tomorrow (Wednesday).~~ ETA sometime next week as it looks now :(

viddo · 2018-05-29T22:36:21Z

This needs an entry on the changelog

Good call ✅

Also, please rebase to ensure the snapshots are correct. I have a feeling some trailing newlines are gone, so it would fail when merged to master

Gotcha. Rebased but didn't see any trouble, but probably better safe than sorry. :)

viddo · 2018-06-20T18:04:26Z

Friendly bump; I believe I've addressed all the code review comments that's been brought up so far, what's expected to be changed/done for this PR to be accepted at this point?

re: the changelog, it keeps getting conflicts due to other updates, so I'll fix that as a last task.

gardner · 2018-07-12T22:13:48Z

We are targeting web, ios, and android with a single code base. Having this feature will allow us to share more code between the different platforms.

SimenB

Needs a rebase, but code LGTM

packages/jest-snapshot/src/snapshot_resolver.js

SimenB · 2018-09-26T06:47:50Z

Nice job, thanks for sticking with us!

nicoabie · 2019-01-09T19:54:58Z

@viddo thanks for the great work, it is very useful indeed!!! and @SimenB for the reviews an merging.

I'm using it and works very well except for one little detail. Both testPath and snapshotFilePath args are absolute paths. I think it would be desirable they be relative to the CWD.

In the meantime, this is a hack what works around the issue.

module.exports = {
  // resolves from test to snapshot path
  resolveSnapshotPath: (testPath, snapshotExtension) =>
    '__snapshots__/' + /src\/(.*)/.exec(testPath)[1] + snapshotExtension,

  // resolves from snapshot to test path
  resolveTestPath: (snapshotFilePath, snapshotExtension) =>
    ('src/' + /__snapshots__\/(.*)/.exec(snapshotFilePath)[1]).slice(0, -snapshotExtension.length),

  // Example test path, used for preflight concistency check of the implementation above
  testPathForConsistencyCheck: 'src/components/example.test.js',
};

SimenB · 2019-01-10T07:46:31Z

It might make sense to pass in config, then you can do path.relative(config.rootDir, testPath) if you want?

nicoabie · 2019-01-10T10:32:51Z

I think there isn't any use case where you will need absolute paths so users will always end up transforming to relative.

The absolute path of any file will always differ between different computers, so for the same test file you will end up having a different snapshot per user.

SimenB · 2019-01-10T10:55:40Z

I think there isn't any use case where you will need absolute paths so users will always end up transforming to relative.

If you need to read the file or something. IDK, but it's easier to say "here is the file" and let people do whatever they want with the path rather than making it relative.

If we make it relative, what should we make it relative to? cwd? rootDir? Your config file?

nicoabie · 2019-01-10T11:13:52Z

I may be wrong but I can only imagine "src" folder.
And I know it is a convention to call it that way.

But I believe that is the more common use case and should be the default. Your idea about passing in a config is great but should be for the other cases.

Does that makes sense?

SimenB · 2019-01-10T12:17:18Z

Jest for instance does not use src/. Anyways, please open up a new issue for discussion here, no reason to have it in the PR and ping all who have contributed here 🙂

- Input component, with supporting Hint, Label and ErrorMessage components. - Use template tests from `govuk-frontend` via `govuk-frontend-template-spec` Note: At the moment, Jest snapshots are manually copied across. When Jest 0.24 comes out there will be a feature to allow the snapshot test location to be configured (jestjs/jest#6143), so will keep the manual process for now, with the aim to load them in from node_modules when 0.24 is out.

G-Rath · 2019-04-14T01:29:38Z

@SimenB if you have a quick minute, could you confirm for me the version this was released in?

I can't find any mention of snapshotResolver in the master changelog, nor any blog posts.

I'm trying to get as much info as I can for the typings :)

Also, do you know if its possible to use a .ts file for snapshotResolver? I've tried a number of ways, with no luck. I've searched around, but can't find any issues or other articles on the web.

Please let me know if there's value in me making an issue on the jest repo. Thanks!

SimenB · 2019-04-14T06:45:21Z

24.0.0

I can't find any mention of snapshotResolver in the master changelog, nor any blog posts.

Easiest is to search for the pr number in the changelog.

It's also mentioned in the release blog post: https://jestjs.io/blog/2019/01/25/jest-24-refreshing-polished-typescript-friendly#other-highlights

Also, do you know if its possible to use a .ts file for snapshotResolver?

I don't know, I've never used this option. I don't see why it shouldn't work though

G-Rath · 2019-04-14T06:52:01Z

It's also mentioned in the release blog post:

Thanks a bunch! No idea how I missed that 😂

I don't see why it shouldn't work though

Thats what I thought, but Jest complains:


  ● Test suite failed to run

    Jest encountered an unexpected token

    This usually means that you are trying to import a file which Jest cannot parse, e.g. it's not plain JavaScript.

    By default, if Jest sees a Babel config, it will use that to transform your files, ignoring "node_modules".

    Here's what you can do:
     • To have some of your "node_modules" files transformed, you can specify a custom "transformIgnorePatterns" in your config.
     • If you need a custom transformation specify a "transform" option in your config.
     • If you simply want to mock your non-JS modules (e.g. binary assets) you can stub them out with the "moduleNameMapper" config option.

    You'll find more details and examples of these config options in the docs:
    https://jestjs.io/docs/en/configuration.html

    Details:

    C:\Users\G-Rath\workspace\projects-personal\strongly-typed-event-emitter\test\snapshotResolver.ts:3
      resolveSnapshotPath: (testPath: string, snapshotExtension: string) => {
                                    ^

    SyntaxError: Unexpected token :

      at ScriptTransformer._transformAndBuildScript (node_modules/@jest/transform/build/ScriptTransformer.js:471:17)
      at ScriptTransformer.transform (node_modules/@jest/transform/build/ScriptTransformer.js:513:25)

It works fine if I convert it to .js. This is my transformers:

    "transform": {
      ".(ts|tsx)": "ts-jest"
    }

I also tried putting in ts-node, but it changed nothing. From my very limited understanding of Jest, it looks like maybe it's doing snapshotResolver before it loads/handles transformers?

SimenB · 2019-04-14T06:55:43Z

Oh sorry, I misread the question as resolving to ts files, not using ts in a resolver.

That probably doesn't work, but we can (probably) make it work. Mind opening up a feature request?

G-Rath · 2019-04-14T07:03:56Z

Mind opening up a feature request?

Just what I wanted to hear ;)

I've made #8330 for this.

CaerusKaru · 2020-03-04T06:54:26Z

@viddo I know it's been a while, but is there any chance you could provide the setup that you use to get this to work with Bazel? Might be helpful to document this in the rules_nodejs repository example for jest integration.

longlho · 2020-07-11T07:02:45Z

@CaerusKaru you're probably looking for https://github.com/formatjs/formatjs/blob/main/tools/jest.bzl#L46-L66 most things work out of the box including snapshot update

github-actions · 2021-05-11T05:08:20Z

This pull request 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.

viddo requested a review from orta as a code owner May 6, 2018 18:34

facebook-github-bot added the cla signed label May 6, 2018

This was referenced May 6, 2018

Adds optional configuration snapshotTag #5838

Closed

Configurable snapshots directory for colocated tests #1650

Closed

rickhanlonii reviewed May 6, 2018

View reviewed changes

website/versioned_docs/version-22.4/Configuration.md Outdated Show resolved Hide resolved

thymikee reviewed May 6, 2018

View reviewed changes

packages/jest-snapshot/src/snapshot_resolver.js Outdated Show resolved Hide resolved

thymikee reviewed May 6, 2018

View reviewed changes

SimenB mentioned this pull request May 22, 2018

How can I have all my tests and mocks inside a folder "tests" next to my "app" source code? #2726

Open

SimenB requested changes May 22, 2018

View reviewed changes

viddo force-pushed the snapshot-resolver branch from a676ffa to 2786bfd Compare May 29, 2018 22:28

viddo force-pushed the snapshot-resolver branch 8 times, most recently from 8fe886f to 49d2e7a Compare June 4, 2018 21:35

SimenB approved these changes Jul 13, 2018

View reviewed changes

SimenB reviewed Sep 26, 2018

View reviewed changes

packages/jest-snapshot/src/snapshot_resolver.js Show resolved Hide resolved

Update snapshot_resolver.js

0167ff7

SimenB merged commit 8eefa96 into jestjs:master Sep 26, 2018

viddo deleted the snapshot-resolver branch September 26, 2018 15:51

SimenB mentioned this pull request Oct 20, 2018

Allow custom cleanup functions in TestScheduler #7093

Closed

fbartho mentioned this pull request Oct 24, 2018

[jest-snapshot] Custom Snapshot Directories confuse the Obsolete Snapshot Logger #7257

Closed

Globegitter mentioned this pull request Oct 31, 2018

WIP: feat(cypress_test): introduce rule bazelbuild/rules_typescript#261

Closed

viddo mentioned this pull request Nov 11, 2018

Require an example path for custom snapshot resolver consistency check #7356

Merged

penx mentioned this pull request Jan 16, 2019

Input penx/govuk-frontend-react#8

Merged

SimenB mentioned this pull request Jan 31, 2019

Jest chokes on unexpected snapshot file when --no-cache and --watch are used together #7636

Closed

G-Rath mentioned this pull request Apr 14, 2019

Support transforming resolves (to allow for .ts-based snapshotResolver) #8330

Closed

royra mentioned this pull request Mar 4, 2020

Support different coverage options in projects #9633

Closed

CaerusKaru mentioned this pull request Mar 9, 2020

build: add Bazel build system cdk8s-team/cdk8s#54

Closed

github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021

Add resolver for custom snapshots paths #6143

Add resolver for custom snapshots paths #6143

Conversation

viddo commented May 6, 2018 • edited

Summary

Motivation

Test plan

SimenB commented May 6, 2018

viddo commented May 6, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sidferreira commented May 7, 2018

viddo commented May 7, 2018

viddo commented May 11, 2018

codecov-io commented May 11, 2018 • edited

Codecov Report

SimenB left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SimenB commented May 22, 2018

viddo commented May 22, 2018 • edited

viddo commented May 29, 2018

viddo commented Jun 20, 2018

gardner commented Jul 12, 2018

SimenB left a comment

Choose a reason for hiding this comment

SimenB commented Sep 26, 2018

nicoabie commented Jan 9, 2019

SimenB commented Jan 10, 2019

nicoabie commented Jan 10, 2019

SimenB commented Jan 10, 2019

nicoabie commented Jan 10, 2019

SimenB commented Jan 10, 2019

G-Rath commented Apr 14, 2019 • edited

SimenB commented Apr 14, 2019 • edited

G-Rath commented Apr 14, 2019 • edited

SimenB commented Apr 14, 2019

G-Rath commented Apr 14, 2019

CaerusKaru commented Mar 4, 2020

longlho commented Jul 11, 2020

github-actions bot commented May 11, 2021

viddo commented May 6, 2018 •

edited

viddo commented May 6, 2018 •

edited

codecov-io commented May 11, 2018 •

edited

viddo commented May 22, 2018 •

edited

G-Rath commented Apr 14, 2019 •

edited

SimenB commented Apr 14, 2019 •

edited

G-Rath commented Apr 14, 2019 •

edited