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.

Sign up for GitHub

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

Jump to bottom

Fix Detox Tests #30312

Closed
wants to merge 4 commits into from
Closed

Fix Detox Tests #30312

wants to merge 4 commits into from

Conversation

utkarsh-dixit
Copy link
Contributor

@utkarsh-dixit utkarsh-dixit commented Nov 3, 2020 •
edited
Loading

Summary

Detox tests were failing because of DisplayIfVisible component and YellowBox. This PR is meant to fix these issues

Changelog

[General] [Removed]: Remove DisplayIfVisible component and use simple if else conditions to show which component to show.
[General] [Fixed]: Disable Yellow box so that it doesn't block detox to perform any action on api's tab at bottom.
[General] [Changed]: Update detox from 16.7.2 to 17.10.0
[General][Removed]: Exclude arm64 for rn-tester iOS project
[General][Changed]: Updated pod file for cocoapods dependencies

Test Plan

You can test this PR by,

yarn build-ios-e2e && yarn test-ios-e2e

All Test Passing

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 3, 2020
@analysis-bot
Copy link

analysis-bot commented Nov 3, 2020 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: fc0b3fa

@analysis-bot
Copy link

analysis-bot commented Nov 3, 2020 •
edited
Loading

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,896,785 +133
android hermes armeabi-v7a 8,394,513 +126
android hermes x86 9,385,080 +134
android hermes x86_64 9,330,215 +129
android jsc arm64-v8a 10,348,457 +0
android jsc armeabi-v7a 9,829,278 +0
android jsc x86 10,398,251 +0
android jsc x86_64 10,983,646 +0

Base commit: fc0b3fa

@hramos
Copy link
Contributor

hramos commented Nov 4, 2020

Thanks for the fix! Can you re-enable the detox tests on Circle CI as part of this PR? I think you'd only need to uncomment some lines in .circleci/config.yml, and then we'll be able to confirm these are fixed.

@utkarsh-dixit utkarsh-dixit requested a review from hramos as a code owner November 6, 2020 15:03
@utkarsh-dixit
Copy link
Contributor Author

utkarsh-dixit commented Nov 10, 2020 •
edited
Loading

@hramos I enabled detox tests for iOS in circleci but they are failing because react native fails to build for iOS (mainly for arm64 and i386). I faced the same issue locally when I was building rn-tester for iOS, which I fixed by following the solution in this issue #29984, which was to exclude arm64 and i386 architectures when building rn tester app.

ld: in /Users/distiller/react-native/packages/rn-tester/Pods/OpenSSL-Universal/ios/lib/libcrypto.a(cryptlib.o), building for iOS Simulator, but linking in object file built for iOS, file '/Users/distiller/react-native/packages/rn-tester/Pods/OpenSSL-Universal/ios/lib/libcrypto.a' for architecture arm64

Do you have any ideas about what is causing this issue?

@mkcode
Copy link
Contributor

mkcode commented Nov 10, 2020

There are a few issues here:

@mkcode mkcode mentioned this pull request Nov 10, 2020
Closed
facebook-github-bot pushed a commit that referenced this pull request Nov 12, 2020
@mkcode @facebook-github-bot
Update yarn.lock (#30361)
0bbab02 
Summary:
This yarn.lock file was regenerated by deleting the existing yarn.lock file and rerunning `yarn install`.

The current `yarn.lock` file is fairly out of date and is affecting the CI servers.

More specifically, fsevents@1.2.7, [referenced in yarn.lock here](https://github.com/facebook/react-native/blob/46be292f671c70aac4ecc178c96e3a2a6a3d16da/yarn.lock#L3215), [fails to compile](fsevents/fsevents#272) on node 12 or later.

This causes the [yarn install step on CI to error](https://app.circleci.com/pipelines/github/facebook/react-native/7065/workflows/de2bebff-33a5-4d2e-bc73-f7f380641452/jobs/176266/parallel-runs/0/steps/0-104).

This PR is being made as 1 step in getting [the RNTester detox tests running again](#30312).

We can alternatively manually update the `yarn.lock` file with [the minimum needed fsevent changes](https://github.com/facebook/react-native/compare/master...MLH-Fellowship:update-yarn-lock-fsevents?expand=1#diff-51e4f558fae534656963876761c95b83b6ef5da5103c4adef6768219ed76c2deL3215-L3221) if this PR changes too much.

## Changelog

<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://github.com/facebook/react-native/wiki/Changelog
-->

[Internal] [Changed] - Bump fsevents from 1.2.7 to 1.2.13.

Pull Request resolved: #30361

Test Plan: Verify yarn install step on CI is fixed.

Reviewed By: hramos

Differential Revision: D24910611

Pulled By: cpojer

fbshipit-source-id: 45a2f25a14d368d2f464e489924b1a2befbdf784
@utkarsh-dixit
Copy link
Contributor Author

@hramos Can you review this PR again, the tests are successfully passing on circle CI now

@mkcode
Copy link
Contributor

mkcode commented Dec 8, 2020

@utkarsh-dixit - Let's fix the analyze code step first.

mkcode
mkcode suggested changes Dec 15, 2020
View reviewed changes
packages/rn-tester/js/RNTesterAppShared.js Outdated
);
// Disable yellowbox so that they don't cover the API's tab and make it
// unaccessible for detox.
LogBox.ignoreAllLogs();
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's test for jest presence here and only disable Logs in detox where jest is defined.

@kelset
Copy link
Contributor

kelset commented Sep 22, 2022

closing this since Detox is not in the repo anymore: #32907

@kelset kelset closed this Sep 22, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mkcode mkcode mkcode requested changes

@hramos hramos Awaiting requested review from hramos

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@utkarsh-dixit @analysis-bot @hramos @mkcode @kelset @facebook-github-bot