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 resolver for custom snapshots paths #6143

Merged
merged 17 commits into from Sep 26, 2018

Conversation

Projects
None yet
@viddo
Copy link
Contributor

viddo commented May 6, 2018

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

This comment has been minimized.

Copy link
Collaborator

SimenB commented May 6, 2018

CI is failing, mind looking into that test?

@viddo

This comment has been minimized.

Copy link
Contributor Author

viddo commented May 6, 2018

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...

@@ -725,6 +725,30 @@ monkey-patching the jasmine API. If you wanted to add even more jasmine plugins
to the mix (or if you wanted some custom, project-wide matchers for example),
you could do so in this module.

### `snapshotResolver` [string]

This comment has been minimized.

@rickhanlonii

rickhanlonii May 6, 2018

Member

This should not be added to the 22.4 docs as it will not land in 22.4

This comment has been minimized.

@viddo

viddo May 7, 2018

Author Contributor

My bad; I'll remove it

const snapshotPath = custom.resolveSnapshotPath(testPath, DOT_EXTENSION);
return snapshotPath.endsWith(DOT_EXTENSION)
? snapshotPath
: snapshotPath + DOT_EXTENSION;

This comment has been minimized.

@thymikee

thymikee May 6, 2018

Collaborator

I think falling back to appending DOT_EXTENSION is not necessary. We shouldn't provide a layer of indirection in snapshot resolution logic but rely on user's implementation, just like in resolveTestPath below.

This comment has been minimized.

@viddo

viddo May 7, 2018

Author Contributor

Yeah, I was a bit reluctant about this part too.


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

This comment has been minimized.

@thymikee

thymikee May 6, 2018

Collaborator

How about:

Custom snapshot resolver must implement a `resolveSnapshotPath` function.

Documentation: https://facebook.github.io/jest/docs/en/configuration.html#snapshotResolver

This comment has been minimized.

@viddo

viddo May 7, 2018

Author Contributor

👍

@sidferreira

This comment has been minimized.

Copy link
Contributor

sidferreira commented May 7, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

viddo commented May 7, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

viddo commented May 11, 2018

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

This comment has been minimized.

Copy link

codecov-io commented May 11, 2018

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Collaborator

SimenB left a comment

This needs an entry on the changelog

Left some test nits


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

This comment has been minimized.

@SimenB

SimenB May 22, 2018

Collaborator

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


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

This comment has been minimized.

@SimenB

SimenB May 22, 2018

Collaborator

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


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

This comment has been minimized.

@SimenB

SimenB May 22, 2018

Collaborator

should just drop the array here


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

This comment has been minimized.

@SimenB

SimenB May 22, 2018

Collaborator

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

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

This comment has been minimized.

@SimenB

SimenB May 22, 2018

Collaborator

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

This comment has been minimized.

@viddo

viddo May 31, 2018

Author Contributor

👍 better now?

This comment has been minimized.

@viddo

viddo Jun 4, 2018

Author Contributor

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


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

This comment has been minimized.

@SimenB

SimenB May 22, 2018

Collaborator

should the heading be bold?

@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented May 22, 2018

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

This comment has been minimized.

Copy link
Contributor Author

viddo commented May 22, 2018

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 viddo force-pushed the viddo:snapshot-resolver branch from a676ffa to 2786bfd May 29, 2018

@viddo

This comment has been minimized.

Copy link
Contributor Author

viddo commented May 29, 2018

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 viddo force-pushed the viddo:snapshot-resolver branch 8 times, most recently from 8fe886f to 49d2e7a May 29, 2018

@viddo

This comment has been minimized.

Copy link
Contributor Author

viddo commented Jun 20, 2018

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

This comment has been minimized.

Copy link

gardner commented Jul 12, 2018

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

SimenB approved these changes Jul 13, 2018

Copy link
Collaborator

SimenB left a comment

Needs a rebase, but code LGTM

@viddo viddo force-pushed the viddo:snapshot-resolver branch from db041eb to fb04013 Sep 26, 2018

@viddo

This comment has been minimized.

Copy link
Contributor Author

viddo commented Sep 26, 2018

@viddo mind fixing the comment? Then we can merge 🙂

Great! Rebased and addressed the comments + added test. Verified working locally, 🙏for CI to be all green now

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

10 checks passed

ci/circleci: lint-and-typecheck Your tests passed on CircleCI!
Details
ci/circleci: test-browser Your tests passed on CircleCI!
Details
ci/circleci: test-jest-circus Your tests passed on CircleCI!
Details
ci/circleci: test-node-10 Your tests passed on CircleCI!
Details
ci/circleci: test-node-6 Your tests passed on CircleCI!
Details
ci/circleci: test-node-8 Your tests passed on CircleCI!
Details
ci/circleci: test-or-deploy-website Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@SimenB

This comment has been minimized.

Copy link
Collaborator

SimenB commented Sep 26, 2018

Nice job, thanks for sticking with us!

@nicoabie

This comment has been minimized.

Copy link

nicoabie commented Jan 9, 2019

@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

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

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

@nicoabie

This comment has been minimized.

Copy link

nicoabie commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

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

This comment has been minimized.

Copy link

nicoabie commented Jan 10, 2019

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

This comment has been minimized.

Copy link
Collaborator

SimenB commented Jan 10, 2019

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 🙂

@penx penx referenced this pull request Jan 16, 2019

Merged

Input #8

penx added a commit to penx/govuk-frontend-react that referenced this pull request Jan 18, 2019

Input (#8)
- 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 (facebook/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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment