Skip to content
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

fix: image mock in jest setup #41957

Conversation

delphinebugner
Copy link
Contributor

@delphinebugner delphinebugner commented Dec 15, 2023

Summary:

resolveAssetSource should not return nothing by default in jest as some librairies (ex: react native fast image) rely on it!
They use it to compute the image source ; without this mock implementation, all sources disappear from images in test environnement - and we do test them a lot in test env (may be the only part of Images we actually look at in snapshots! )

Relates to issue #41907 -> not sure it fixes though, as in test the "require" source resolves to an object with assetFileTransformer - still having no width and height in output even with the real resolveAssetSource

It was introduced in version 0.73, where Image mock was changed - see PR #36996

Changelog:

[GENERAL][FIXED] - fix jest setup for Image.resolveAssetSource

Test Plan:

Here is a simple test rendering a FastImage with source = { uri : "some-string"} :

Without the fix : source is "undefined" in test :
image

With the fix :
image

the console.log comes from react native fast image and indicates that source is correctly retrieved

I also tested passing a require(../path.png) as source :
Without the fix :
image

With the fix :
image

resolveAssetSource should not return nothing by default as some librairies (ex: react native fast image) rely on it to compute the image source
without this mock implementation, all sources disappear from images in test environnement
see issue facebook#41907

 the input source - leaving it empty breaks
@facebook-github-bot facebook-github-bot added CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Dec 15, 2023
@ryancat
Copy link
Contributor

ryancat commented Dec 16, 2023

If this works before PR #36996, is it possible to just remove the line for resolveAssetSource and use the original defaults?

@idrissakhi
Copy link

@delphinebugner agree with @ryancat we should remove the line so it's require the actual ImageResolver to get the real image data such as width ... etc

@delphinebugner
Copy link
Contributor Author

delphinebugner commented Dec 18, 2023

Yes you're right, I was going too quicly, we should absolutely keep what's ImageResolver normally do... Additional question is, should we also check and eventually remove other methods mocked in PR #36996?

  .mock('../Libraries/Image/Image', () => {
    const Image = mockComponent('../Libraries/Image/Image');
    Image.getSize = jest.fn();
    Image.getSizeWithHeaders = jest.fn();
    Image.prefetch = jest.fn();
    Image.prefetchWithMetadata = jest.fn();
    Image.queryCache = jest.fn();
    Image.resolveAssetSource = jest.fn();

    return Image;
  })

I checked for iOS and all of those methods just call native module ImageLoader ; resolveAssetSource is the only exception!

The original issue was about Image.getSize getting undefined in test after jest setup : #35518
I was unable to reproduce the error, even by removing the mocked methods - maybe something else fixed it? I checked the native mock of ImageLoader, and it does have the getSize function ; even if something we can do could be to add getSizeWithHeaders and queryCache to it!

@ifero
Copy link

ifero commented Feb 28, 2024

Is there an ETA to merge this bit?

delphinebugner pushed a commit to delphinebugner/react-native that referenced this pull request Mar 14, 2024
It's purely a JS function, we can not mock it - I would say more, we should not mock it in test and ensure something is returned (for example react native fast image relies on this function to compute the URL of its component, without this fix, every URLs disappear in snapshots - see facebook#41957)
@delphinebugner
Copy link
Contributor Author

Closed because I opened a newer + more exhaustive PR on the topic: #43497

facebook-github-bot pushed a commit that referenced this pull request Mar 18, 2024
Summary:
### Context

- Since RN 0.73, in the jest.setup file, methods of the Image module (Image.getSize, Image.resolveAssetSource...) are mocked on the **JS side** (introduced in #36996)
- It causes issues like #41907 : `Image.resolveAssetSource` returns nothing in test env with the new JS mock, when some test relies on it.
- On my project, it broke the snapshots : the URL of images disappeared. I use `react-native-fast-image` which uses `Image.resolveAssetSource` to compute URLs.
- I first opened a PR to fix exclusively Image.resolveAssetSource: #41957. I will close it to focus on this new one.
- As suggested by ryancat and idrissakhi, it should be better to return to the previous mock, where no method is mocked on the JS side, and we can trust the actual JS to work in test.

This is what this PR intends to do.

### Content

Along fixing the Image module mock in jest.setup, this PR :

- adds unit test on each one of the methods, ensuring they have a consistent behavior even when the module is mocked.
- adds 3 missing native mocks for `NativeImageLoader`: `prefetchImageWithMetadata`, `getSizeWithHeaders` & `queryCache`. After this PR, no method from NativeImageLoader remains unmocked.

## Changelog:

[GENERAL][FIXED] - fix jest setup for Image methods (resolveAssetSource, getSize, prefetch, queryCache)

Pull Request resolved: #43497

Test Plan:
See exhaustive unit tests in PR.

You can re-use the mock with all the methods mocked and see how the new unit tests fail.

I also patched those changes on my project: my snapshot did have their URL back (see demonstrative screenshots in my original PR: #41957 - NB; fixed mock was different but result was the same -> those screenshots cover only two cases, but anyway they illustrate well the case!)

Reviewed By: ryancat

Differential Revision: D54959063

Pulled By: tdn120

fbshipit-source-id: 837266bd6991eb8292d9f6af1774e897ac7a8890
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants