Configurable snapshots directory for colocated tests

[holic]

The React community seems to be moving towards colocating tests with components. I appreciate the changes to the default testRegex that looks for any .test.js file.

However, using this colocation method means a bunch of scattered __snapshots__ directories throughout the codebase. Since snapshots seem to be more for machine readability than human readability, I would tend to throw them all in a single top-level directory (like you might have for build/dist files). What are your thoughts on making the snapshots directory configurable?

[cpojer]

snapshots are colocated with tests and they are definitely not meant to be for machine readability. They are meant to be reviewed by humans, both the author of a test and the code reviewers.

[cpojer]

Jest also ensures that there are never any stale snapshots in the system. This means we both need a way to map from a snapshot to its associated test and back. This seems kinda hard to configure via string values in the JSON config :(

[holic]

Thanks for the quick reply!

I understand that the snapshots are meant to be read and reviewed. It just seems a bit messy to have __snapshots__ littered throughout the codebase when I want to have tests colocated with components. This may also come from a pattern of grouping components by functionality, rather than having a components directory (e.g. user/Profile.js and layout/Nav.js).

In the meantime, I've just started dropping tests into __tests__ to get the __snapshots__ into a single place but this isn't my ideal.

I also understand this is a bit hard to configure. My thought was to define some top level directory for __snapshots__ and have the directory structure within it match the path to the component from that top level.

It might just be me that finds this messy. Curious to hear from others that colocate tests with components and have multiple directories of components.

[cpojer]

I don't really quite understand what you mean. The snapshots are colocated with tests and there is a 1:1 mapping between testfilename.js and __snapshots__/testfilename.js.snap?

[holic]

Currently my directory looks a bit like this:

js/
  user/
    __snapshots__/
      Avatar.test.js.snap
      Profile.test.js.snap
    Avatar.js
    Avatar.test.js
    Profile.js
    Profile.test.js
  layout/
    __snapshots__/
      App.test.js.snap
      Nav.test.js.snap
    App.js
    App.test.js
    Nav.js
    Nav.test.js

The __snapshots__ in this way feels a little messy to me (but this may just be me).

As an alternative, my thought was a directory structure like this might be easier to understand/comb through:

js/
  __snapshots__/
    user/
      Avatar.test.js.snap
      Profile.test.js.snap
    layout/
      App.test.js.snap
      Nav.test.js.snap
  user/
    Avatar.js
    Avatar.test.js
    Profile.js
    Profile.test.js
  layout/
    App.js
    App.test.js
    Nav.js
    Nav.test.js

[cpojer]

but that is the opposite of colocating snapshot files with their tests, isn't it?

[holic]

I'm proposing that the snapshots don't necessarily need to be colocated with tests. I find it more important that tests are colocated with components. Having all three colocated feels messy, mostly due to the several __snapshots__ directories that are created/scattered throughout the codebase (at least in the way I've chosen to organize my components).

[cpojer]

I'm not religiously opposed to this if you want to do all the work necessary to support this, find a way to make this maintainable and test it well.

[cpojer]

We can also help on our discord channel and happy to review code, of course :)

[holic]

I think my main argument is that the snapshots are auto generated and aren't meant to be modified manually, nor really imported directly into the codebase. So it doesn't feel right to have them intermixed within the code which you are actively editing/maintaining/importing.

I'm not particularly married to this idea either :) Just wanted to propose it to see what others thought. If I'm not alone in this, I may take the time to mock up a PR and see what it might look like.

[cpojer]

I'm sure you find many people that share your opinion. Testing is like a religion to people and I'm happy to support all kinds of doing this as long as it integrates well with Jest and is well tested itself.

[cpojer]

This is something where we'll need some help from the community. I think a good way to support this is by adding a config option resolveSnapshots that points to a module with two functions:

exports.resolveSnapshotFile = testPath => snapshotFilePath;
exports.resolveTestFile = snapshotFilePath => testPath;

this way we can call these methods to resolve the snapshot path from a test path and the other way round and it will enable use to keep snapshot files consistent.

[cpojer]

I'm gonna close this given the inactivity. If somebody would like to work on this, I'm happy to reopen this and review a PR :)

[develar]

I have the same problem as #1653.

preprocessor approach is not good in case of

1. TypeScript and IntelliJ IDEA.

   Because IntelliJ IDEA has built-in very fast TS compiler. So, during editing no compiler output. And you add "Compile TypeScript" task to run configuration — output will be dumped from memory to FS on run. No need to use slow (because ts must operate on the whole project, — opposite to babel) runtime preprocessor.

2. TypeScript on CI server. Because on CI server in any case you compile all sources before test run — again because TS / tslint operates on the whole project, not on individual file.

Well, as a workaround, out/__snapshots__ can be added to VCS.

[lucasfeliciano]

👀 Following this to work on it in a near future

[cpojer]

Actually at this point I don't really think I want to support this feature. It adds unnecessary complexity and gives almost no value.

[luispuig]

So. How did end it?. I really like to have each test with its component. But I'd prefer to get all snapshot file in a unique folder, not around the code.

[lucasfeliciano]

Yeah, I think would be good to have this feature. I'm not 100% happy with my snapshots all over the project. :(

[luispuig]

And I don't like either to replicate the project structure on test folder...

[porto88]

+1, I would also like to have all snapshots moved to one folder.

[lucasfeliciano]

Cool, I'll try to work on it this weekend and propose a PR.

Then we move this discussion there.

[luispuig]

Great!

Let me know if I can help you. I've been thinking about complexity.

The main problem is what happens if there are Test with same file name. Probably best option would be to get relative path of test file and replicate folder structure on snapshot folder.

[luispuig]

@cpojer told me that @ljharb was interested on this feature too.

[andreyluiz]

@lucasfeliciano @luispuig Any progress on this? Do yout want some help with something?

[luispuig]

I didn't try...

[wesleyhall]

Just looking at this myself. To be honest, I kind of despise this double underscore padded directory name thing. Guido has a lot of answer for! These things are making their way to Javascript as well now :). I happen to think they look a right mess, but sure... subjective.

I'm personally in the directory per component camp, and sit my unit tests as a sibling to component code using the component.test.js convention.

I think it would be quite nice to have capability to configure the snapshots to also be generated as siblings to the component and tests. They are, after all, just resource files and are to my tests what css files are to the component (which I also keep as siblings).

The magic seems to happen here.. https://github.com/facebook/jest/blob/master/packages/jest-snapshot/src/utils.js#L96

I'm wondering how difficult it would be to pass this hardcoded directory name through from the package.json config (making current value default for backwards compatibility). I'm still too much of a JS newb to really know exactly how that process works.

In any case, opening support for *.test.js style, but not supporting the same sibling structure for snapshot files does feel like a bit of an inconsistency.

[luispuig]

Hi,

I was just thinking on this. I get why main developers doesn't want to change this feature.

If you use a testing folder yo git everything on same folder. But if your work on different proyecta it's worse to move componentes becouse you got to copy the component and the test on a different folder.

But if we use the test with the component it get full of snapshot folders.

To maintain a different folder for snapshots needs to care a lot about the directory structure becouse we can have 2 tests/components with same name on different folders. And again, to share de component, the snapshot is part of the test.

I got a cuestión. Would be posible that jest uses the same test-file to Store the snapshots? This would be great and satisfy the 2 trends.

[wesleyhall]

@luispuig

I get why the developers might advise in the docs and FAQ that moving the snapshots out from under the component in the structure is a bad idea. Personally, I would not do this anyway, because I would agree with them on this point.

A style that is quickly becoming the norm though, is to place unit test files as siblings to the code under test. This is why jest now supports *.test.js files.

I actually don't get why they wouldn't want to change it. We are talking about a testing tool that imposes a hardcoded opinion over the structure of the user's source tree.

The intention presumably is consistency with the __tests__ idea, but this is a very different thing. Actually, in isolation, if the snapshot directory name were to be imposed, a dot folder would be a better choice because it contains, "stuff you rarely need to mess with", obviously this is distinct from tests, which are code under maintenance by the user.

I'm fairly new to the JS world to be honest (I have 20 years of JVM development xp), and it does feel like the "standards war" is still ongoing in this ecosystem. In any case though, a testing library is not really a suitable battleground for that kind of thing in my opinion. If a tool like, "create-react-app", were to configure jest in an opinionated way my gripe would be softer, because those kinds of templating apps are reasonable places to fight the standards war. People that use those things are in someway consenting to having somebody else's opinion over structure applied to their project. I still might complain that it would be nice if I could override it (without "ejecting"), but I feel that the case would be weaker.

...but a testing library that applies it's opinion over the structure of my git repo... nope. Can't get behind that really.

[luispuig]

@Wesley hall

I agree with you. Keeping the snapshot file with the test file is the best practice.

Getting the whole structure full of snapshot folders is what's bothering me. I suggest to put the snapshots inside the test file instead of creating a snapshot folder on test file level. Would it be possible?

[wesleyhall]

That's my view too.

In a post above I pasted a link to the line in the code where the __snapshots__ exists as a hardcoded string.

Since jest has the capability of being configured from the package.json file (https://facebook.github.io/jest/docs/configuration.html), what I would tend to do is add a new configuration key called, "snapshotDirectory", that allows for configuration of this. Personally, I would be happy with this value being expressed relative to the test (so the behaviour we are both describing would be configurable by specifying either "." or ""), and leaving the current value as the default.

I might try to pull together a pull request on this over the weekend, but I am not really familiar with the mechanism used to pass these configuration values down. If you, or anyone reading, knows how this is done, i'm definitely all ears on that, otherwise i'll figure it out.

[wesleyhall]

I should say, I don't think they can be put inside the test file, if this is what you meant. There are mechanisms within jest to update the snapshots etc. I wouldn't really want the system changing some portion of the my unit test js file. I would put them still as separate files but as simple siblings to the test js file.

[luispuig]

Maybe it's not possible, but it would be great.

I really don't like that behavior. I eve prefer filename.test.js.snapshot

The proposal of config a relative path won't work becouse each file is in diferente place on the structure. Maybe is ok for what I just sayed. But honestly, I prefer to have no option to config it. It's better to have a convention and push community to use it. It reduces fatigue and it makes sharing code easier.

[wesleyhall]

@luispuig

I think (more or less) the "filename.test.js.snapshot" thing is what I am talking about anyway. Essentially, just write the exact same files that are being written now, as siblings to the test files rather than in the __snapshots__ subdirectory.

I think I have decided that I will probably just fork jest, and simply remove the subdirectory hardcoded string in the line of code I linked above, that will do the trick. Wont be configurable, but i'll just use this forked version. It'll be easy enough to rebase over each future release of jest unless they make a change to support this behaviour in the core branch.

I don't imagine I stand much chance of getting this merged back in, but if anyone is reading this and finds themselves particularly interested in this minor fork, I can probably make it public somewhere.

Thanks

[ljharb]

@wesleyhall please file the PR to jest if you write the code; it will help the discussion if a tested, clean PR exists that allows the location of snapshots to be configured.

[wesleyhall]

@ljharb

Having had a bit more of a look at this, I actually think it might be a relatively straightforward change.

The engine of the whole snapshot affair is the SnapshotState class defined in State.js inside the jest-snapshot package. If you look at the constructor (https://github.com/facebook/jest/blob/master/packages/jest-snapshot/src/State.js#L48), you'll see that the snapshot path attribute is set from a constructor argument. Only if it's "falsey" does it go off to the "getSnapshotPath()" function which is where the hardcoding of the __snapshots__ directory gets applied. If a path is passed to the constructor it seems it will use that instead.

The object is constructed by the initializeSnapshotState function in the jest-snapshot index.js file (https://github.com/facebook/jest/blob/master/packages/jest-snapshot/src/index.js#L58), which takes the snapshot location as an argument and passes it on to the constructor.

This function is exported, and seems to be only called within the jest-jasmine2 package, inside a file called setup-jest-globals.js (https://github.com/facebook/jest/blob/master/packages/jest-jasmine2/src/setup-jest-globals.js#L115).

Notice that the third argument there is an empty string. That's the snapshot path, so essentially this is getting passed through all the way to the SnapshotState constructor, and since the empty string is 'falsey', "getSnapshotPath()" is called every time.

That exported function in setup-jest-globals.js has access to the config object as an argument though, so I imagine, it might just be a matter of replacing the empty string with something like, 'config.snapshotDirectory'. The function also has access to the testPath (the path to the file being tested), so it should even be possible to support snapshot paths relative to the test location with a "\<testDir>/some/location" style format similar to how "\<rootDir>" is used in some other configuration values.

With this, we would have the capability to set both project and test relative snapshot directories, and could default the value to an empty string to maintain backwards compatibility with the existing behaviour.

The problems I am having are two-fold. Firstly, the config system for accepting these arguments is fairly complex, and I am struggling to see how it all fits together. Secondly, I am not exactly a JS veteran and getting a functional local test environment for my changes is proving to be what I generally call, "a ball-ache".

I know it sounds a bit lazy, but assuming my analysis above is somewhere close to correct, I imagine that somebody who works with this codebase regularly and already has a functional local development setup would probably find the change a relatively straightforward thing to do. Most of the work seems to be in adding the config option and passing the value through.

I do think it would be a nice feature, so maybe i'll find the time and patience to figure out the config engine in the coming weeks and get the PR in if nobody with better working knowledge of the codebase is driven to pick it up. You might have to bear with me though.

[ljharb]

@wesleyhall Thanks! (http://nvm.sh and nvm install node should theoretically get you a functioning environment unless you're on Windows)

[wesleyhall]

@ljharb :) Well, not a vet but I can get a little further than that ;).

It's more about the yarn link stuff, and getting my local version to properly reflect in a separate project that I can use to test the changes. The contributor docs talk a little about this and linking up the jest-cli module. This didn't work for me at the first attempt, but probably something silly and obvious.

The bigger problem is probably the complexities in the jest-cli and jest-config modules. The configuration options are mentioned and referenced in numerous places. My IDE finds at least 6 different source files that have objects with the snapshotSerializers key. These files are not commented so it's fairly hard to tell what each of them are for and what I need to change to support a new config option. I'll have to deep dive a bit, but i'll have a crack at it when I get chance.

[wyze]

Don't let the number of results scare you when you search for a config key like that. You probably get results for the src and build directories, along with the types for the key.

I have most of the code changed to add this functionality. I need to actually test it then write some tests. I won't continue if you would like to dig in deeper and solve this yourself, and I can help point you in the right direction.

[luispuig]

@weslyhall @wyze

Thanks a lot for your work. I really believe this important feature will close the discussion on a good mood.

[wesleyhall]

@wyze

No, please by all means continue. I can look at your PR and see how close (or far) I was :).

I haven't really looked much into the whole flow stuff yet, so there are some gaps in my experience here. Really only started any serious JS work in January of this year, so yeah, n00b :). 20 years on the JVM though. It's all just zeros and ones at the end of the day ;).

I might stick around and learn from the assembled crowd of wizards.

Thanks for your help.

[wyze]

No problem!

I've been working with Reason lately and starting to get to writing snapshot tests for my components, but the problem was when the .re files get converted to .js they are output in another directory. So my snapshots end up in a directory that is in my .gitignore because they are generated test .js files.

This option is exactly what I am looking for so I can store my snapshots in a directory with my tests that are written in .re and checked into Git.

[wesleyhall]

@wyze

Excellent. Thank you again.

Small suggestion having looked at the PR. One of the things I actually wanted to do was get rid of the __snapshots__ directory by configuration. It's all subjective of course, but I don't really like these double underscore directories. The PR looks like it still requires this style though.

If you restore the:

this._snapshotPath = snapshotPath || getSnapshotPath(testPath);

In the SnapshotState constructor, and perhaps rename the function to something like getDefaultSnapshotPath, then I think we would now have complete control over the snapshot directory. Could still, of course, drop the __snapshots__ directory into the config value if you wanted to follow that as convention but just locate them in another place, but would not be forced to.

Perhaps I should be making these comments on the PR itself, but it's just a thought :).

[wesleyhall]

@wyze

Just realised I misread that a bit. The snapshot path attribute should be the full file path not just the directory of course.

Could also change:

path.join(snapshotPath || path.dirname(testPath), '__snapshots__')

from the getSnapshotPath function to something like:

snapshotPath || path.join(path.dirname(testPath), '__snapshots__')

I.e. take the specified path if given, or instead take the testPath with __snapshots__ appended.

[lukescott]

I have a directory structure that currently looks like this:

app/
    components/
        Foo/
            __snapshots__
                component.test.js.snap
                reducer.test.js.snap
            component.js
            component.test.js
            reducer.js
            reducer.test.js
        Bar/
            __snapshots__
                component.test.js.snap
                reducer.test.js.snap
            component.js
            component.test.js
            reducer.js
            reducer.test.js

If putting them into a single root __snapshots__ folder is out of the question, would something like below be more acceptable?

app/
    components/
        Foo/
            component.js
            component.test.js
            component.test.js.snap
            reducer.js
            reducer.test.js
            reducer.test.js.snap
        Bar/
            component.js
            component.test.js
            component.test.js.snap
            reducer.js
            reducer.test.js
            reducer.test.js.snap

Our app is plugin/feature based, so each component is in a separate directory, which contains several files related to it.

[SgtPooki]

@lukescott we have a modified version we're using that puts the snapshots next to the test as your comment says, simply by removing the snapshots from this line: https://github.com/facebook/jest/blob/master/packages/jest-snapshot/src/utils.js#L94

[assertchris]

I like @lukescott's idea. They're still technically JS files, so something like:

• component.js
• component.test.js
• component.snap.js

[ljharb]

That doesn't address my use case fwiw; I want nothing but tests in one root dir, nothing but production code in another root dir, and nothing but snapshots in another root dir - nothing should ever be colocated.

[ghernandez345]

I'm colocating tests with my code and I've found placing tests under a spec directory helps hide the snapshot directories from your source code, but still keeps it closely tied to the relevant test files. Might be useful for those who don't want to immediately see the snapshots directories in your project.

app/
    components/
        Foo/
            spec/
                __snapshots__/
                    component.test.js.snap
                    reducer.test.js.snap
                component.test.js
                reducer.test.js
            component.js
            reducer.js

[aminroosta]

@cpojer Will you merge a PR implementing your suggested approach?

by adding a config option resolveSnapshots that points to a module with two functions:

exports.resolveSnapshotFile = testPath => snapshotFilePath;
exports.resolveTestFile = snapshotFilePath => testPath;

[aminroosta]

@cpojer we can also implement it via two regex patterns for the 1:1 mappings. Right now we have in jest-resolve-dependencies

import {replacePathSepForRegex} from 'jest-regex-util';

const snapshotDirRegex = new RegExp(replacePathSepForRegex('/__snapshots__/'));
const snapshotFileRegex = new RegExp(
  replacePathSepForRegex('__snapshots__/(.*).snap'),
);
const isSnapshotPath = (path: string): boolean =>
!!path.match(snapshotDirRegex);

We can get snapshotFileRegex & snapshotFileRegex from jest config.