Asset basenames in Jest snapshots #13319

myrjola · 2017-04-05T11:31:45Z

This change only affects tests run with Jest. require('/images/image1.png') will be replaced with

Object {
  "testUri": "relative/path/to/images/image1.png",
}

in the Jest snapshot instead of always being 1 returned by RelativeImageStub. This change makes it possible to test conditional asset loading in components.

The problem with this change is that it will probably break a lot of existing snapshots, but that should be easily fixed when a project updates to a new version of React Native by running jest -u to update all snapshots.

Motivation:

A component can have conditional asset loading based on its props, this logic would be nice to test with Jest snapshots. This problem has been discussed in jestjs/jest#2838.

Test Plan:

I have added a snapshot test assetRelativePathInSnapshot.

$ npm test -- assetRelativePathInSnapshot

> react-native@1000.0.0 test /Users/martin/git/react-native
> jest "assetRelativePathInSnapshot"

 PASS  Libraries/Image/__tests__/assetBaseNameInSnapshot.js
  ✓ renders assets based on basename (132ms)

Test Suites: 1 passed, 1 total
Tests:       1 passed, 1 total
Snapshots:   1 passed, 1 total
Time:        1.674s
Ran all test suites matching "assetBasenameInSnapshot".

It is easy to verify from the snapshot that the change works.

Libraries/Image/tests/snapshots/assetBaseNameInSnapshot.js.snap:

// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders assets based on relative path 1`] = `
<View>
  <Image
    source={
      Object {
        "testUri": "Libraries/Image/__tests__/img/img1.png",
      }
    }
  />
  <Image
    source={
      Object {
        "testUri": "Libraries/Image/__tests__/img/img2.png",
      }
    }
  />
</View>
`;

Without the change the output would look like this:

// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`renders assets based on basename 1`] = `
<View>
  <Image
    source={1}
  />
  <Image
    source={1}
  />
</View>
`;

Breaking changes:

Jest snapshots will need to be updated

Who does this affect: Everyone using Image in Jest snapshots
How to migrate: Running jest -u will update the snapshots, the snapshots should be reviewed that they are correct.
Why make this breaking change: It enables testing of conditional asset loading.
Severity (number of people affected x effort): Low.

facebook-github-bot · 2017-08-21T18:50:13Z

@myrjola I tried to find reviewers for this pull request and wanted to ping them to take another look. However, based on the blame information for the files in this pull request I couldn't find any reviewers. This sometimes happens when the files in the pull request are new or don't exist on master anymore. Is this pull request still relevant? If yes could you please rebase? In case you know who has context on this code feel free to mention them in a comment (one person is fine). Thanks for reading and hope you will continue contributing to the project.

ide

Make the mock for snapshots shaped more similar to the actual asset objects.

ide · 2017-08-21T21:52:48Z

jest/assetFileTransformer.js

+  // the correct images are loaded for components. So require('img1.png')
+  // becomes 'img1.png' in the Jest snapshot.
+  process: (_, filename) =>
+    `module.exports = ${JSON.stringify(path.basename(filename))};`


Better to mimic what the actual transformer does and return an object.

require('/images/image1.png') will be replaced with 'image1.png' in the snapshot instead of 1 returned by RelativeImageStub. This makes it possible to test conditional asset loading in components.

myrjola · 2017-08-22T16:20:51Z

Rebased the code and addressed the requested changes. I also chose to return relative path instead of basename. Updated the PR description with the latest changes and also added a breaking changes section.

myrjola · 2017-08-22T16:23:32Z

jest/assetFileTransformer.js

-    `module.exports = ${JSON.stringify(path.basename(filename))};`
+    `module.exports = {uri: ${JSON.stringify(path.relative('.', filename))}};`,
+
+  getCacheKey: createCacheKeyFunction([__filename]),


This function was needed after rebasing on latest master. I took some inspiration from https://github.com/facebook/react-native/blob/master/jest/preprocessor.js.

Without it running Jest will give the following error:

FAIL Libraries/Image/__tests__/assetBaseNameInSnapshot.js ● renders assets based on basename /Users/martin/git/react-native/Libraries/Image/__tests__/img/img1.png:1 ({"Object.<anonymous>":function(module,exports,require,__dirname,__filename,global,jest){module.exports = [object Object]; ^^^^^^ SyntaxError: Unexpected identifier at ScriptTransformer._transformAndBuildScript (node_modules/jest-runtime/build/script_transformer.js:305:17) at Object.<anonymous> (Libraries/Image/__tests__/assetBaseNameInSnapshot.js:21:64) at tryCallTwo (node_modules/promise/lib/core.js:45:5) at doResolve (node_modules/promise/lib/core.js:200:13) ✕ renders assets based on basename (144ms)

pull-bot · 2017-08-22T19:25:06Z

	Warnings
⚠️	🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Attention: @facebook/react-native

Generated by 🚫 dangerJS

ide · 2017-08-22T20:54:02Z

jest/assetFileTransformer.js

+  // require('img1.png') becomes `Object { "uri": 'path/to/img1.png' }` in the
+  // Jest snapshot.
+  process: (_, filename) =>
+    `module.exports = {uri: ${JSON.stringify(path.relative('.', filename))}};`,


This "uri" field doesn't actually exist at runtime, maybe call it testUri so it's clear this field is defined only in tests?

ide · 2017-08-23T05:00:36Z

@cpojer how does this look to you?

myrjola · 2017-08-23T05:01:42Z

Renamed uri to testUri in the latest commit and updated description.

cpojer · 2017-08-23T10:34:47Z

@mjesun mind taking care of this one?

mjesun

LGTM, looks nice :)

One minor nit: could we use real images (a 1x1 PNG would be enough). I know it does not change anything from the test PoV, but GH has hard times trying to load the invalid image.

Thanks!

myrjola · 2017-08-23T17:13:14Z

Changed empty PNG files to real PNGs. Now GitHub is happy again.

facebook-github-bot · 2017-08-25T17:57:07Z

@mjesun has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

mjesun · 2017-08-29T13:49:34Z

Sorry for the late reply; we haven't forgot about that, but we had some issues importing the PR :( (unrelated to the PR itself).

mjesun · 2017-09-07T14:41:03Z

@myrjola Just a small heads up; we adjusted the test so it is able to run on our infra (and potentially anywhere regardless of the exec folder 🙂. Thanks again for the contribution! 😊

oblador · 2017-11-08T16:24:49Z

jest-preset.json

@@ -7,12 +7,15 @@
    ]
  },
  "moduleNameMapper": {
-    "^[./a-zA-Z0-9$_-]+\\.(bmp|gif|jpg|jpeg|png|psd|svg|webp)$": "RelativeImageStub",


Removing this broke importing files with multiple device resolution variations. Metro bundler will resolve require('./file.png') to ./file@2x.png if you're on a 2x screen.

Thanks for reporting! Could you give a bit more detail what the problem is? Apparently this change has broken your tests. Could you give some steps to reproduce?

I immediately notice that the regex ^[./a-zA-Z0-9$_-]+\\.(bmp|gif|jpg|jpeg|png|psd|svg|webp)$ does not match the @ character in ./file@2x.png. The regex has been moved from moduleNameMapper to the transform mapper on line 17 in the same file. Perhaps the change from ./file.png to ./file@2x.png happens between the mappers. @oblador, could you edit the regex on line 17 to ^[./a-zA-Z0-9@$_-]+\\.(bmp|gif|jpg|jpeg|png|psd|svg|webp)$, so that it also matches ./file@2x.png?

This also breaks tests completely on Windows for me. Please see #19370 for details

Replace regexp to match on Windows filesystems, replace backslashes with forward slashes in assetFileTransformer.js

facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Apr 5, 2017

myrjola mentioned this pull request Apr 5, 2017

Fix Circle CI build for Ubuntu 14.04 image #13321

Closed

myrjola force-pushed the asset-names-in-jest-snapshots branch 2 times, most recently from 04fd23e to cb67bc1 Compare April 5, 2017 15:59

shergin added the Tooling label Jun 2, 2017

ide suggested changes Aug 21, 2017

View reviewed changes

myrjola added 2 commits August 22, 2017 16:30

Asset basenames in Jest snapshots

9f102e4

require('/images/image1.png') will be replaced with 'image1.png' in the snapshot instead of 1 returned by RelativeImageStub. This makes it possible to test conditional asset loading in components.

Return assets conforming to ImageURISourcePropType and use relative path

bee962c

myrjola force-pushed the asset-names-in-jest-snapshots branch from cb67bc1 to bee962c Compare August 22, 2017 16:05

myrjola commented Aug 22, 2017

View reviewed changes

Rephrase basename to relative path according to new semantics

05f41f2

myrjola force-pushed the asset-names-in-jest-snapshots branch from 0895f74 to 05f41f2 Compare August 22, 2017 19:16

ide reviewed Aug 22, 2017

View reviewed changes

Rename uri to testUri

f2ee740

ide requested a review from cpojer August 23, 2017 05:00

cpojer requested a review from mjesun August 23, 2017 10:34

mjesun suggested changes Aug 23, 2017

View reviewed changes

Change empty PNG files to real PNGs

78e9906

facebook-github-bot added the Import Started This pull request has been imported. This does not imply the PR has been approved. label Aug 25, 2017

facebook-github-bot closed this in 2563917 Sep 6, 2017

oblador reviewed Nov 8, 2017

View reviewed changes

sam-vdp mentioned this pull request May 21, 2018

Jest require() image broken on Windows #19370

Closed

3 tasks

sam-vdp added a commit to sam-vdp/react-native that referenced this pull request May 29, 2018

Fix jest snapshot testing on Windows (broken after facebook#13319):

8f63c5e

Replace regexp to match on Windows filesystems, replace backslashes with forward slashes in assetFileTransformer.js

sam-vdp mentioned this pull request May 29, 2018

Fix jest snapshot testing on windows #19496

Closed

nathggns mentioned this pull request Oct 17, 2018

Provide a more useful mock result when requiring an image in React Native newsuk/times-components#1102

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Asset basenames in Jest snapshots #13319

Asset basenames in Jest snapshots #13319

myrjola commented Apr 5, 2017 •

edited by mjesun

Loading

facebook-github-bot commented Aug 21, 2017

ide left a comment

ide Aug 21, 2017

myrjola commented Aug 22, 2017

myrjola Aug 22, 2017

pull-bot commented Aug 22, 2017 •

edited

Loading

ide Aug 22, 2017

ide commented Aug 23, 2017

myrjola commented Aug 23, 2017

cpojer commented Aug 23, 2017

mjesun left a comment

myrjola commented Aug 23, 2017

facebook-github-bot commented Aug 25, 2017

mjesun commented Aug 29, 2017

mjesun commented Sep 7, 2017

oblador Nov 8, 2017

myrjola Nov 9, 2017

sam-vdp May 21, 2018

Asset basenames in Jest snapshots #13319

Asset basenames in Jest snapshots #13319

Conversation

myrjola commented Apr 5, 2017 • edited by mjesun Loading

Motivation:

Test Plan:

Breaking changes:

Jest snapshots will need to be updated

facebook-github-bot commented Aug 21, 2017

ide left a comment

Choose a reason for hiding this comment

ide Aug 21, 2017

Choose a reason for hiding this comment

myrjola commented Aug 22, 2017

myrjola Aug 22, 2017

Choose a reason for hiding this comment

pull-bot commented Aug 22, 2017 • edited Loading

ide Aug 22, 2017

Choose a reason for hiding this comment

ide commented Aug 23, 2017

myrjola commented Aug 23, 2017

cpojer commented Aug 23, 2017

mjesun left a comment

Choose a reason for hiding this comment

myrjola commented Aug 23, 2017

facebook-github-bot commented Aug 25, 2017

mjesun commented Aug 29, 2017

mjesun commented Sep 7, 2017

oblador Nov 8, 2017

Choose a reason for hiding this comment

myrjola Nov 9, 2017

Choose a reason for hiding this comment

sam-vdp May 21, 2018

Choose a reason for hiding this comment

myrjola commented Apr 5, 2017 •

edited by mjesun

Loading

pull-bot commented Aug 22, 2017 •

edited

Loading