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

Decouple isTesting Platform flag from disabling/enabling animations when testing #38490

Closed
wants to merge 1 commit into from

Conversation

rshest
Copy link
Contributor

@rshest rshest commented Jul 18, 2023

Summary:

Changelog

[Internal] -

The internal Platform.isTesting is tightly coupled to animations being disabled, which in turn can lead to subtle problems in some of the corner cases:

  • Since isTesting is force override to false on JS side in non-dev builds, it means that e2e tests, that would like to use release builds, are out of luck when animation disabling is desired
  • Conversely, some of the e2e tests may actually rely on animations being enabled in order to work, which means they are also out of luck if trying to test a dev build
  • Finally, we have cases of hybrid builds, which are build in release on native side, but also have __DEV__=true on the native side

To both cover the above scenarios, but also to be backwards compatible to all the existing once, this change introduces another flag, Platform.isDisableAnimations.

The way it works is:

  • If it's not specified, the e2e tests behaviour will be exactly the same as before, since by default isDisableAnimations will be true when isTesting is true
  • If it's specified and is equal to false, it means that animations will be still enabled, even if isTesting is true (for those e2e tests that rely on animations being enabled)
  • If it's specified and is equal to 'true', it means that animations will be force disabled, no matter whether we test a release or a dev build

Note that this only specifies the JS side of things, defaulting Platform.isDisableAnimations to "not specified" (i.e. all the tests will behave as before).

Pulling it through for different platforms is done as a separate follow-up.

Differential Revision: D47516800

@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: Facebook Partner: Facebook Partner fb-exported labels Jul 18, 2023
@github-actions
Copy link

github-actions bot commented Jul 18, 2023

Warnings
⚠️

packages/react-native/Libraries/LayoutAnimation/LayoutAnimation.js#L13 - packages/react-native/Libraries/LayoutAnimation/LayoutAnimation.js line 13 – 'FabricUIManagerSpec' is defined but never used. (no-unused-vars)

Generated by 🚫 dangerJS against 36fd627

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D47516800

…hen testing (facebook#38490)

Summary:
Pull Request resolved: facebook#38490

## Changelog
[Internal] -

The internal `Platform.isTesting` is tightly coupled to animations being disabled, which in turn can lead to subtle problems in some of the corner cases:
* Since `isTesting` is force override to `false` on JS side in non-dev builds, it means that e2e tests, that would like to use release builds, are out of luck when animation disabling is desired
* Conversely, some of the e2e tests may actually rely on animations being enabled in order to work, which means they are also out of luck if trying to test a dev build
* Finally, we have cases of hybrid builds, which are build in release on native side, but also have `__DEV__=true` on the native side

To both cover the above scenarios, but also to be backwards compatible to all the existing once, this change introduces another flag, `Platform.isDisableAnimations`.

The way it works is:
* If it's not specified, the e2e tests behaviour will be exactly the same as before, since by default `isDisableAnimations` will be true when `isTesting` is true
* If it's specified and is equal to `false`, it means that animations will be still enabled, even if `isTesting` is true (for those e2e tests that rely on animations being enabled)
* If it's specified and is equal to 'true', it means that animations will be force disabled, no matter whether we test a release or a dev build

Note that this only specifies the JS side of things, defaulting `Platform.isDisableAnimations` to "not specified" (i.e. all the tests will behave as before).

Pulling it through for different platforms is done as a separate follow-up.

Differential Revision: D47516800

fbshipit-source-id: ba338e931ff51d4420feda6c32e7400656beb842
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D47516800

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,836,246 +31
android hermes armeabi-v7a 8,146,137 +24
android hermes x86 9,341,623 +22
android hermes x86_64 9,184,362 +27
android jsc arm64-v8a 9,448,107 +58
android jsc armeabi-v7a 8,629,863 +40
android jsc x86 9,530,780 +36
android jsc x86_64 9,773,988 +36

Base commit: 777934e
Branch: main

@github-actions
Copy link

This pull request was successfully merged by @rshest in 04ad34d.

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

@github-actions github-actions bot added the Merged This PR has been merged. label Jul 18, 2023
rshest added a commit to rshest/react-native that referenced this pull request Jul 19, 2023
Summary:
## Changelog
[Internal] -

This allows to override `Platform.isDisableAnimations` for the Android platform, in the same way as it's done with `IS_TESTING` (but similarly optionally doing `IS_DISABLE_ANIMATIONS` in addition to/instead).

See for more context: facebook#38490

Differential Revision: D47530731

fbshipit-source-id: 80c63787bca303cbedddb05a64eee99d98a51ff6
rshest added a commit to rshest/react-native that referenced this pull request Jul 19, 2023
…facebook#38511)

Summary:
Pull Request resolved: facebook#38511

## Changelog
[Internal] -

This allows to override `Platform.isDisableAnimations` for the Android platform, in the same way as it's done with `IS_TESTING` (but similarly optionally doing `IS_DISABLE_ANIMATIONS` in addition to/instead).

See for more context: facebook#38490

Reviewed By: cortinico

Differential Revision: D47530731

fbshipit-source-id: 1f08f1ec0ec64c880975dfef27a202fba6d26bd7
facebook-github-bot pushed a commit that referenced this pull request Jul 19, 2023
…#38511)

Summary:
Pull Request resolved: #38511

## Changelog
[Internal] -

This allows to override `Platform.isDisableAnimations` for the Android platform, in the same way as it's done with `IS_TESTING` (but similarly optionally doing `IS_DISABLE_ANIMATIONS` in addition to/instead).

See for more context: #38490

Reviewed By: cortinico

Differential Revision: D47530731

fbshipit-source-id: b90300124b2a8bac97fae78a94e8a2cc9d7fd5bc
juniorklawa pushed a commit to juniorklawa/react-native that referenced this pull request Jul 20, 2023
…hen testing (facebook#38490)

Summary:
Pull Request resolved: facebook#38490

## Changelog
[Internal] -

The internal `Platform.isTesting` is tightly coupled to animations being disabled, which in turn can lead to subtle problems in some of the corner cases:
* Since `isTesting` is force override to `false` on JS side in non-dev builds, it means that e2e tests, that would like to use release builds, are out of luck when animation disabling is desired
* Conversely, some of the e2e tests may actually rely on animations being enabled in order to work, which means they are also out of luck if trying to test a dev build
* Finally, we have cases of hybrid builds, which are build in release on native side, but also have `__DEV__=true` on the native side

To both cover the above scenarios, but also to be backwards compatible to all the existing once, this change introduces another flag, `Platform.isDisableAnimations`.

The way it works is:
* If it's not specified, the e2e tests behaviour will be exactly the same as before, since by default `isDisableAnimations` will be true when `isTesting` is true
* If it's specified and is equal to `false`, it means that animations will be still enabled, even if `isTesting` is true (for those e2e tests that rely on animations being enabled)
* If it's specified and is equal to 'true', it means that animations will be force disabled, no matter whether we test a release or a dev build

Note that this only specifies the JS side of things, defaulting `Platform.isDisableAnimations` to "not specified" (i.e. all the tests will behave as before).

Pulling it through for different platforms is done as a separate follow-up.

Differential Revision: D47516800

fbshipit-source-id: efcb78f68f9102fec025ecce9a475c9c0bc1ecca
juniorklawa pushed a commit to juniorklawa/react-native that referenced this pull request Jul 20, 2023
…facebook#38511)

Summary:
Pull Request resolved: facebook#38511

## Changelog
[Internal] -

This allows to override `Platform.isDisableAnimations` for the Android platform, in the same way as it's done with `IS_TESTING` (but similarly optionally doing `IS_DISABLE_ANIMATIONS` in addition to/instead).

See for more context: facebook#38490

Reviewed By: cortinico

Differential Revision: D47530731

fbshipit-source-id: b90300124b2a8bac97fae78a94e8a2cc9d7fd5bc
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. fb-exported Merged This PR has been merged. p: Facebook Partner: Facebook Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants