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

mock Animated components with versions that skipSetNativeProps #25108

Closed
wants to merge 1 commit into from

Conversation

rdy
Copy link
Contributor

@rdy rdy commented May 31, 2019

Summary

Using Animated components like View and Image do not get created with __skipSetNativeProps_FOR_TESTS_ONLY = true since they get created before the jest mock can be applied to createAnimatedComponent. For these components mock getters to create versions that properly skip set native props.

Jest tests that render these components get warnings the setNativeProps gets called even though using a testRenderer this should not happen.

Changelog

[Javascript] [Fixed] - Define Animated Components for react-native/jest/setup that properly skip setNativeProps

Test Plan

Run tests

@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. p: Sony Partner: Sony labels May 31, 2019
@pull-bot
Copy link

Warnings
⚠️

❔ Base Branch - The base branch for this PR is something other than master. Are you sure you want to merge these changes into a stable release? If you are interested in backporting updates to an older release, the suggested approach is to land those changes on master first and then cherry-pick the commits into the branch for that release. The Releases Guide has more information.

Messages
📖

📋 Changelog Format - Did you include a Changelog? A changelog entry has the following format: [CATEGORY] [TYPE] - Message.

Generated by 🚫 dangerJS against 18a89ea

@rdy
Copy link
Contributor Author

rdy commented May 31, 2019

@grabbou @matthargett this is a change i needed to suppress warnings i was seeing from animated components being rendered in jest in 0.59, it looks like createAnimatedComponent gets mocked too late so setNativeProps gets called on these components and throws a bunch of warnings.

@RubenSandwich
Copy link
Contributor

RubenSandwich commented Jun 3, 2019

@rdy @cpojer

I ran into this problem myself and I solved it a bit differently. I used jest.spyOn to change the params at runtime:

import { Animated } from 'react-native';
const mockedAnimatedTiming = Animated.timing;
jest.spyOn(Animated, 'timing').mockImplementation((value, options) => {
  return mockedAnimatedTiming(value, {
    ...options,
    useNativeDriver: false,
  });
});

I'd suggest we adopt this for the RN jest/setup.js because it removes the whole need for __skipSetNativeProps_FOR_TESTS_ONLY. However, I can imagine jest.spyOn has a higher overhead cost then your approach.

Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you rebase this on master instead? We don't merge things like this into stable branches.

Please check out https://github.com/facebook/react-native/pull/25108/files#diff-606adbd6a8c97d177b17baee5a69cdd9R101 where we are mocking AnimatedImplementation. I think we should make the changes there.

@rdy rdy changed the base branch from 0.59-stable to master June 3, 2019 20:46
Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

jest/setup.js Outdated Show resolved Hide resolved
jest/setup.js Outdated Show resolved Hide resolved
jest/setup.js Outdated Show resolved Hide resolved
@rdy
Copy link
Contributor Author

rdy commented Jun 3, 2019

@cpojer i rebased against master as you requested.

I wasn't sure about your comment on applying this to the AnimatedImplementation. The problem with the way the current mock worked was that Animated.View, Animated.Image, .etc are constructed using a version prior to the mock applied by jest/setup.js. I couldn't think of a better way to intercept those components except by directly mocking them with versions that were not called setNativeProps in test, I duped the code because of how jest doesn't like accessing stuff through closure scope.

If there is a better way to apply the AnimatedImplementation mock at the right time I'd be happy to apply that to this commit.

@rdy
Copy link
Contributor Author

rdy commented Jun 3, 2019

@RubenSandwich I tried your snippet with my tests cases but I still saw the warning.

Maybe I didn't apply it correctly?

In my case I have Animated.View that end up getting constructed with the original implementation of createAnimatedComponent. In the componentDidMount it eventually calls _animatedPropsCalback but that version doesn't have the flag that tells it to skip the setNativeProps calls which spits out a bunch of those warnings.

I'd still like the animation to run in my test as I have something that tracks the animation frames so I can prove that those values are getting interpolated the way i expect over time.

@RubenSandwich
Copy link
Contributor

RubenSandwich commented Jun 4, 2019

@rdy

So I put that code at the start of my own app's native module mocks. To use it in jest/setup.js .mock('AnimatedImplementation', () => { It has to be modified slightly:

...
  .mock('AnimatedImplementation', () => {
    const AnimatedImplementation = require.requireActual('AnimatedImplementation');

    const oldTiming = AnimatedImplementation.timing;
    AnimatedImplementation.timing = function(value, options) {
      return oldTiming(value, {
        ...options,
        useNativeDriver: false,
      });
    };

    return AnimatedImplementation;
  })
...

I ran my own apps tests and that change worked just fine without any changes to the snapshots or console warnings. No idea if it would pass the current tests in the RN repo though. It also still seems to remove the need for __skipSetNativeProps_FOR_TESTS_ONLY in my tests.

Another important point is that I'm on RN 52, so the above code would need to be modified for removing the haste style imports. (So full file paths.)

@cpojer
Copy link
Contributor

cpojer commented Jun 4, 2019

It looks like this is breaking all of the Animated tests in React Native. Can you fix them up?

@matthargett
Copy link
Contributor

Note that the underlying issue appears to be a regression from sometime between 0.55 and 0.59, and was discovered during an upgrade. cc @kelset

@cpojer
Copy link
Contributor

cpojer commented Jun 8, 2019

@rdy could you take a look at the test failures? This version is not mergeable and breaks all the JS tests.

@rdy
Copy link
Contributor Author

rdy commented Jun 8, 2019

@cpojer yes, sorry was wrapped up in a few other tasks this week but I’ll look into the failures.

Copy link

@analysis-bot analysis-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code analysis results:

  • eslint found some issues. Run yarn lint --fix to automatically fix problems.

jest/setup.js Outdated Show resolved Hide resolved
Copy link
Contributor

@cpojer cpojer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice, that looks good!

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cpojer is landing this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@react-native-bot
Copy link
Collaborator

This pull request was successfully merged by @rdy in a4acd88.

When will my fix make it into a release? | Upcoming Releases

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jun 12, 2019
M-i-k-e-l pushed a commit to M-i-k-e-l/react-native that referenced this pull request Mar 10, 2020
…ook#25108)

Summary:
Using Animated components like View and Image do not get created with __skipSetNativeProps_FOR_TESTS_ONLY = true since they get created before the jest mock can be applied to createAnimatedComponent. For these components mock getters to create versions that properly skip set native props.

Jest tests that render these components get warnings the setNativeProps gets called even though using a testRenderer this should not happen.

## Changelog

[Javascript] [Fixed] - Define Animated Components for react-native/jest/setup that properly skip setNativeProps
Pull Request resolved: facebook#25108

Differential Revision: D15779434

Pulled By: cpojer

fbshipit-source-id: f39e21ed8e71c2c155297c791d3bf573909896d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API: Animated Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged This PR has been merged. p: Sony Partner: Sony
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants