Adds optional configuration snapshotTag

[sidferreira]

Summary

This is a WIP PR. It is based mainly on #1650 and my needs to have the same React Native test snapshots for each of the different platforms. I'm adding it here to both have help to finish it (as I don't have that much time available) but also have more input about the best solution.

The snapshotPath parameter already existed on SnapshotState, I just added it as a project / global config or a command line argument.

Test plan

Currently, on my machine, there are +- 4 (HG-related) tests failing. Those errors failed before and after the changes. After adding the parameters on package.json it still has the same results, but of course, it doesn't mean it is working properly.

[codecov-io]

Codecov Report

Merging #5838 into master will decrease coverage by 0.03%.
The diff coverage is 37.5%.

@@            Coverage Diff             @@
##           master    #5838      +/-   ##
==========================================
- Coverage   64.12%   64.09%   -0.04%     
==========================================
  Files         217      217              
  Lines        8332     8341       +9     
  Branches        4        4              
==========================================
+ Hits         5343     5346       +3     
- Misses       2988     2994       +6     
  Partials        1        1

Impacted Files	Coverage Δ
packages/jest-config/src/normalize.js	93.77% <ø> (ø)	⬆️
packages/jest-config/src/index.js	36% <ø> (ø)	⬆️
packages/jest-snapshot/src/State.js	0% <0%> (ø)	⬆️
packages/jest-jasmine2/src/setup_jest_globals.js	0% <0%> (ø)	⬆️
.../src/legacy_code_todo_rewrite/jest_adapter_init.js	0% <0%> (ø)	⬆️
packages/jest-snapshot/src/utils.js	100% <100%> (ø)	⬆️
packages/jest-cli/src/test_scheduler.js	37.89% <100%> (ø)	⬆️
packages/jest-snapshot/src/index.js	58% <16.66%> (-5.05%)	⬇️
... and 1 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 e58d3db...ea3a830. Read the comment docs.

[SimenB]

hg is mercurial, so just install it to have the tests pass. If you're on mac, a quick brew install hg should do it.

@cpojer what is your stance on this option these days?

[cpojer]

I’m not a fan of thi. I think to support platform extensions we should add custom support in Jest itself to work properly. There is an old open issue somewhere about making platform extensions first class throughout Jest with a “jest.setPlatform” or “jest.forEachPlatform” function. That one could append the .ios or .android to snapshot files. Note that we also need a reverse lookup function from snapshot file to test file, which we could bake into Jest as well without having to expose anything to users.

[sidferreira]

@cpojer Thanks for the input, let me break it into parts to better advance the talk:

• Snapshot Extension Prefix: Your comment on the platform thing sounded like a Snapshot Extension Prefix (or Snapshot Name Sufix). I think it would work GREAT, even better than a different path, for platform related. And I do believe suffix and prefix options for snapshots may be a great deal. I can't see much reason to support both ATM but if we keep the prefix to platforms, maybe give user's access to the suffix (to do whatever they want) is an option

• Snapshot Path: I still think, at least for now, that users should have a way to set this path. Or, at least, I can't see a reason to forbid the users from doing so. Also, as the parameter was around on SnapshotState, why not use it.

• Reverse lookup for tests: Now that you mentioned this, take a look at Fix Snapshot Extensions References #5842

Still, I'm open to help with both. I must admit I'm not a Jest Master, but helping is a great way to learn.

[ArtemGovorov]

@sidferreira

Snapshot Path: I still think, at least for now, that users should have a way to set this path. Or, at least, I can't see a reason to forbid the users from doing so.

As with every new configuration setting, I think the question should be not "why not to add", but "why to add" the configuration setting.

• More configuration settings - more moving parts - more reasons for things to break in certain user scenarios.
• Less configuration settings (and more sane defaults) make it much easier to integrate with Jest from external tools.

[sidferreira]

@ArtemGovorov Well... thinking like this... and in the work needed so far... Indeed the point is should we add it?. Still, as mentioned, the SnapshotState already had an unused SnapshotPath option. Which is ignored if not set, and this behavior didn't change.

[sidferreira]

@cpojer I did a small test locally with the platform thing. It worked great indeed, but I'm not sure if the way I implemented it is actually good. Based on my tests the best would be to get defaultPlatform from jest-resolve/Resolver. To do so I would have to add a getResolver or getPlatform on jest-runtime/Runtime and add it as a parameter to SnapshotState just like I did the snapshotPath so far. The "ugly" part is that, in the end, defaultPlatform is just config.haste.defaultPlatform.
So, in my test, I just added:

  const haste = config.haste || globalConfig.haste || {}
  const platform = haste.defaultPlatform || '' 
  const snapshotState = new _jestSnapshot.SnapshotState(testPath, { expand, updateSnapshot, platform });

or

  const snapshotState = new _jestSnapshot.SnapshotState(testPath, {
    expand,
    updateSnapshot,
    platform: (config.haste || globalConfig.haste || {}).defaultPlatform || ''
  });

to https://github.com/facebook/jest/blob/69c11a0c8192e643f4aa4c0aa79ce09cdd1ab155/packages/jest-jasmine2/src/setup_jest_globals.js#L103

add options.platform as a parameter to getSnapshotPath at https://github.com/facebook/jest/blob/69c11a0c8192e643f4aa4c0aa79ce09cdd1ab155/packages/jest-snapshot/src/State.js#L44

and, finally, replace the '.' by (platform ? `.${platform}.` : '.') on https://github.com/facebook/jest/blob/69c11a0c8192e643f4aa4c0aa79ce09cdd1ab155/packages/jest-snapshot/src/utils.js#L94

As I said, it works but doesn't look that good. You mentioned jest.setPlatform or jest.forEachPlatform, but I must admit I don't understand that much on Jest's internals to do so.

If you think it is an acceptable implementation, let me know so I can make some more tests on it.

All this test also made me think on a testLabel or something to act just like the platform in this case: create a .somelabel.snap extension... but that's just a thought.

[sidferreira]

@ArtemGovorov @cpojer @SimenB Sorry guys to mark you on this.
But I would like to check the chances of this going on.
If you want me to improve the solution, feel free to give some input.

Thanks in advance

[SimenB]

@mjesun @rickhanlonii thoughts?

[cpojer]

I don't think this works with the reverse lookups. I have previously voiced my concerns. I don't think this is something I'm happy to add to Jest at this time, I see little upside of the complexity.

[sidferreira]

@cpojer @SimenB This week I took some time to go deep on that. And just like @cpojer mentioned it sounds WAY easier than it is. The main issue is indeed the places where the snapshot extensions are searched.

One great example is https://github.com/facebook/jest/blob/3341d162715679a596bfbbec8a7a5c5676c5f1b7/packages/jest-snapshot/src/index.js#L26 which uses the snapshot path to resolve the test path.

To solve it, in the way that this PR suggests, any reference to the snapshot path or extension would require a function so it would access the SnapshotState property that sets the correct snapshot test path and test name resolver. Still, I'm not sure if this solution would be desirable (which is the reason I didn't continue in the tests).

I also tried to set a name to the snapshot but it ended causing conflicts anyways (as I use 2 jest projects pointing to the same tests, changing only the environment).

I still looking for a way to use only one written test to all platforms tested (React Native ios/android/windows) but at this moment I can't see any other way other than the functions mentioned.

As a last option, I'm about to try some sort of loop that "overrides" the it and applies the tests for each platform.

Being so, if the team is up to take a look at the function solution (which I mean I would develop it and after that, we would discuss if it sounds good or not) I would work a bit more.
If the team decided that it isn't according to the Jest best practices/goals, I would say it still a no fix issue.

Regards,

[rickhanlonii]

I'm feel the same as @cpojer, so the preferable route for me would be to do this in a custom matcher as a separate lib. If there are limitations to doing that, let's build tools that enable what the custom matcher would need

[sidferreira]

@rickhanlonii As far as I tested, the problems are:

1. My personal goal is to have only one test (describe/it) that will run for both android and ios. As the SnapshotState is reset after each test, I would need to:

A run all tests for both platforms in the same _it_, with so we avoid that after the test is finished, the snapshots for the other platform won't be seen as obsolete. And in this case we need to set hasteMap's defaultPlatform and be sure it will update the imported modules (which I'm a bit skeptic)

B Have one test file for each platform, which means duplicated code

C Have one snapshot file for each platform, but there's no proper way to the SnapshotState access the current platform. Maybe using Platform.OS, but I'm still not sure if this would work out.

2. Even if we pick the option C, there are many modules that use SNAPSHOT_EXTENSION ( check Fix Snapshot Extensions References #5842 ) as a constant. I did a really quick and shallow research in the last hour and I noticed that we would need a way to resolve the test names just like Packages and Modules are resolved in https://github.com/facebook/jest/blob/72983d4b78de7d1745f9efb0b64a28e1121fedd6/packages/jest-resolve/src/index.js#L110 .

Right now it seems that the JestSnapshot/SnapshotState deeply tied to the project, maybe an option to override the component, and using resolvers, would give me a way to achieve the solution I want.

When you described the the custom matcher, I feel that the changes required in item 2 will be basically the same: have a dynamic way to resolve the names.
Maybe handling the Platform should be an external lib as you say, but it seems to me that many of the changes will remain the same.

That's why I proposed try this function (now called resolver) route to achieve such solution.

ps: Sorry if I insist in a bad idea, I'm trying to figure out a solution based on your feedback. And thanks for the patience 👍

[cpojer]

I think I prefer option C with a jest.forEachPlatform method that we add. Jest has the platforms specified via the "haste" config option so we should be able to wire it all up. I'd recommend those files to be called .<platform>.snap.

[sidferreira]

@cpojer I remember you mentioned it. I'll take a look on how it works and how tie them together. But any suggestion is welcome.

EDIT: Does forEachPlatform already exist? I searched in git and couldn't find it. Also, as it is jest. forEachPlatform I'm wondering if it would work something like

forEachPlatform(
    describe('Suite Name', () => {
        it('Test Name', () =>  {})
  })
)

[SimenB]

It does not exist, it would be a new thing

[sidferreira]

@SimenB in this case, is my assumption right? Should I create it or you guys plan to add it in a near future?

[SimenB]

Go for it 🙂

[sidferreira]

Ok, in this case I'll break it into pieces. I'm just wondering if I also split them in smaller PR or not.

• Ensure that all places that SNAPSHOT_EXTENSION is used, we can access the SnapshotState
• Ensure all mentions to snapshot extension is moved to a SnapshotState method,
• Add a forEachPlatform method that apply the spec for each platform inside the suite (or something like that)
• Have the SnapshotExtension methods to access the currentPlatform
• Only use it if forEachPlatform is in place

[sidferreira]

@cpojer I spent some time doing some tests to plan the steps but I found out a lot of limitations. For this kind of change, I'm always in favor of doing the smallest change possible. With that in mind, follows the challenges I found and ideas I had:

I just found out that SnapshotState https://github.com/facebook/jest/blob/70e7ac2689403529b8694f41e17911512441e917/packages/jest-jasmine2/src/index.js#L107-L114 is created before the moment that the test is actually loaded https://github.com/facebook/jest/blob/70e7ac2689403529b8694f41e17911512441e917/packages/jest-jasmine2/src/index.js#L162

It seems to me that we could even move the first block to be just before the line 162, but we still have the problem that we create the SnapshotState before access the test itself. So, except if we do some parsing on the test file, we have no way to know if the test will have the forEachPlatform.

Other than that, the config.haste.defaultPlatform (should rename to currentPlatform?) is used in the module mapper, so, we need to set the platform before any require or import be called.

Basically, it seems to me that this is the place we would have to patch (still checking how it would work on Circus) but I can't find a good solution to be aware of the multiple platform tests without parsing the file (I don't like the idea of adding a test parsing feature). Maybe a pattern in file path/name (I'm not a fan of it too).

I ignored the issue above, and tried to simulate the forEachPlatform with a nested describe. It properly patched the snapshot names, but then we need a way to load the specs (and imports/requires) after the defaultPlatform being changed, this would require to load the specs from another file or any other way it makes the imports be called after the change. We wouldn't need different files for each platform, but we probably would have to make some changes in jest-jasmine2.

In short:

• SnapshotState isn't aware of test internals
• defaultPlatform is used for imports, so needs to be changed before the file is loaded/executed
• forEachPlatform inside the test specs will not be enough

If it isn't acceptable to have a default identifier in test's name, or path, the changes are big and complex, hitting strong the @ArtemGovorov's concern about moving parts.

Maybe there's a way which I missed, but, even wanting a lot this feature, I'm getting skeptical.

(if confusing, let me know)

EDIT: I tried to use two Jest Projects pointing to the same tests and tried to use the nested describe trick to set the Platform name. It works, but as they are different suites, the last test makes the first obsolete. In this case, using the defaultPlatform as snapshot extension would help, but would not use the forEachPlatform solution. We would still need to fix the resolving for test paths and etc. (Basically an acceptable option)

[cpojer]

I don't think we need to do static analysis here. We can change behavior at runtime and adjust everything to be configurable, like jest.forEachPlatform(callback) will call snapshotState.setPlatform(…) which enumerates over config.haste.platforms. When using jest.forEachPlatform, we should be calling jest.resetModules() implicitly and change all the places where we pass the platform to the current one.

This may be a larger change, yes, but that should be ok.

[sidferreira]

@cpojer Makes sense and actually sounds smaller than expected. Thanks for this input!

If I got it right jest.forEachPlatform would be like a describe with a predefined beforeAll that is the setPlatform and resetModules(). And as the SnapshotState is called before any code is run, the snapshot file will remain the same, we will only change the snapshot names (appending the platform in the beginning as it behaves like a describe).

Am I right?

[barklund]

I tried to use two Jest Projects pointing to the same tests and tried to use the nested describe trick to set the Platform name. It works, but as they are different suites, the last test makes the first obsolete.

@sidferreira this is exactly what I'm doing and exactly why I'm following this with much interest.

In this case, using the defaultPlatform as snapshot extension would help, but would not use the forEachPlatform solution. We would still need to fix the resolving for test paths and etc. (Basically an acceptable option)

If this solution solves that use-case, it's perfect for me.

Let me know if I can do anything to help out!

[sidferreira]

@barklund I'll probably start working on it today night (+- 23:00 GMT). It will be great if people join me to test it out.

So far, what I planned, using cpojer's ideas (not mentioning him to avoid spam) is to add in https://github.com/facebook/jest/blob/master/packages/jest-runtime/src/index.js

1. at Runtime a resetPlatform method, that will change the config.haste.defaultPlatform. After that will call resetModules()

2. at jestObject add forEachPlatform, which will have a loop for each config.haste.platforms calling describe(platformName, forEachPlatformSpecs) and adding to it a beforeAll that will call the resetPlatform.

3. Thinking about use currentPlatform instead of defaultPlatform, but still just an idea.

[arichiardi]

Will be able to test it. Lumo use case being that we want to have one snapshot folder per node version we are testing against.

[sidferreira]

@arichiardi Is there any specific reason you want different folders? As far as I found in the source code, it can be challenging. And the jest team mentioned they don't like the (at least for now).
Of course, it's a great moment to change it in this PR and we will work on related places. @cpojer may have some contribution for that.

Also, how exactly you set the different platforms?

[arichiardi]

@sidferreira so the platforms are set on CI with nvm $BUILD_NODE_VERSION and the env will have that env var set (I know, redundant because nvm does it already).

The sub-folder is not really necessary, I just need a way to test against snapshots like:

src/js/__tests__/__snapshots__/cli-test-v9.10.1.js.snap
src/js/__tests__/__snapshots__/cli-test-v8.11.1.js.snap
src/js/__tests__/__snapshots__/cli-test-v8.10.0.js.snap

Maybe this is already doable and I apologize if that's the case.

I am trying to use the string to toMatchSnapshot to work around that but it seems it was designed for a different use case.

[sidferreira]

@arichiardi Ok, I was in this step also but didn't work for me.
Jest creates a new SnapshotState for each test file.
I tried to make a workaround to have different file names, but I felt it would require a lot of large changes.

Is there a way for you to reset the node in use using before all or something? If so, then the work here will help you out. If not, we need to discuss a bit more.

[arichiardi]

Not clear on this sentence:

Is there a way for you to reset the node in use using before all or something? If so, then the work here will help you out. If not, we need to discuss a bit more.

Can you expand? Do you mean change node version while the test is running?

[sidferreira]

@arichiardi Ok, now that you asked like this I got the hint. Well, this is a purpose a bit different than the one planned, but still will keep in mind to try to achieve it anyway.

[arichiardi]

The solution in #5838 (comment) and #5838 (comment) feels better TBH, that is why I was kind of wondering if I can set the snapshotPath dynamically instead of in the conf options.

[sidferreira]

@arichiardi Not at this moment. And making this an option, as far as I checked, would be a big change.

[arichiardi]

Oh this is cool exactly what I was looking for 😄

[sidferreira]

@arichiardi yeah... the other solution didn't work (soon I'll describe the problems) so I tried this option. But kept in mind the point you mentioned (the snapshotTag may have dots on it)
Buuuut still fixing the many tests it broke 😞 😁

[sidferreira]

Hey @cpojer ! Well, after some long tests, debugs and stuff, I found out that the changes to make forEachPlatform to work would way larger than I can handle.

The issue I found is that the imports/requires are parsed, included and cached before the forEachPlatform, and the resetModules wouldn't clear this part of the cache, as resetModules doesn't handle what is cached inside of hasteFS, that is created way before the forEachPlatform be called.

Anyway, if you want I can start another PR, focused on that subject, so we can deal with it.
But based on your suggestions, my needs, and @arichiardi comments, I changed the initial snapshotPath (which I agree it's a delicate change) to be just snapshotTag, not linked directly with haste.defaultPlatform.

[thymikee]

ugh, I don't like splice at all, can we not mutate this array and use yet another variable?
Also, I feel like there's a nice opportunity to extract a function for finding snapshot associated with a file (some of the logic in line 37 and 44 is quite similar).

[sidferreira]

Hey @thymikee! well, first of all, I'm all open to suggestions. The idea of the splice is the following:

I want to run my tests on two projects. One for iOS, one for Android. So my snapshotTags will be "ios" and "android".
What happens in this function is that it removes the "snapshot extension" to try to find out if there's a test file related to this snapshot or not. Now, let's imagine this:

./__tests__/RoundButton.js
./__tests__/__snapshots__/RoundButton.js.ios.snap -> created when running the project with ios tag
./__tests__/__snapshots__/RoundButton.js.android.snap -> created when running the project with android tag
./__tests__/__snapshots__/RoundButton.js.snap -> older test, when I didn't have tags yet.

for each time I run the tests, I know only the current snaptshotTag, so if I have a snapshotTag, I need to extract it from the name the snapshot (which in this case I tried to allow to have dots) and the extension. As I have no idea about how many dots will have, in the path and in the snapshotTag, I used "splice" and "split" to remove the needed elements.

As I wrote the text above, I noticed I could it make a bit simpler, and improved, dropping support to tags with "." in it.

I'm looking for your feedback on that.

[viddo]

I just found this PR as I was reading up on #1650. I was curious if there's any reason for why the original suggestion of @cpojer (#1650 (comment) wouldn't work for the use-cases described here?

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.

I believe such an implementation would be preferred, as it would keep the internals simpler but still allow people to configure how the snapshot resolves at own discretion. I'd be happy to help out to implement it, if such PR would have a chance of being accepted? It was asked on the parent issue already but couldn't see any official answer there.

[sidferreira]

Hey @viddo ! I simply wasn't aware of that one. Shame on me.
I'll take a look to see how doable it is because I believe we'll have to send all configs over to the module. And I'm not sure if it will be accepted too, I'm working on it at my own risks :)

Thinking a bit more on that... Maybe we can just make the whole jest-snapshot the default value for a configuration?

Any words on that @cpojer @SimenB @ArtemGovorov @rickhanlonii

[viddo]

@sidferreira Took a stab at that, see PR #6143. Re-reading your PR I still think that would only solve part of what you want, you'd still need something like the forEachPlatform to be able to have multiple snapshot files.

[sidferreira]

@viddo In this case, I'm using jest's projects to run tests for each platform. As far as I tested the forEachPlatform may require way too many changes to achieve the results I expect. The main problem is how the require and import works and how they are cached. forEachPlatform would work on a per file/test level, and the projects work in a higher level, cleaning all imports properly.

[SimenB]

#6143 should cover this use case I think, so closing this. Thanks for the PR and discussion @sidferreira!

[github-actions]

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.