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

Chore: Upgrade Detox to 16.7.2 #30084

Closed
wants to merge 2 commits into from
Closed

Chore: Upgrade Detox to 16.7.2 #30084

wants to merge 2 commits into from

Conversation

kelset
Copy link
Contributor

@kelset kelset commented Oct 1, 2020

Summary

With Xcode 12 being the latest, Detox 15.x has issues - in particular, it means that if you try to yarn install the dependencies for the repo, you'll be greeted by this error:

error /<stuff>/react-native/node_modules/detox: Command failed.
Exit code: 1
Command: node scripts/postinstall.js
Arguments:
Directory: /<stuff>/react-native/node_modules/detox
Output:
/<stuff>/Library/Detox/ios/5824c837515589f21c08f09b716a6eda088aa31f was found, but could not find Detox.framework inside it. This means that the Detox framework build process was interrupted.
         deleting /<stuff>/Library/Detox/ios/5824c837515589f21c08f09b716a6eda088aa31f and trying to rebuild.
Extracting Detox sources...
Building Detox.framework from /<stuff>/Developer/OSS/react-native/node_modules/detox/ios_src...
child_process.js:637
    throw err;
    ^

With the 👍 of @hramos & @alloy I've prep'd up a small defensive PR that can be quickly merged before cutting 0.64, that bumps the version of Detox from 15.4.4 to the highest version available within the reach of "no breaking changes" in changelog.

The main reason why with 16.x this error doesn't happen is that from 16.0.0:

Detox now comes as a prebuilt framework on iOS, thus lowering npm install times and saving some build issues that happen due to unexpected Xcode setups.

It would have been better to update directly to latest (at the time of writing 17.7.1) but there are at least two versions that had changelogs that seem to involve bigger changes:

Hopefully CI will will show that the bump doesn't break any test 🤞

Changelog

[Internal] [Changed] - Bumped Detox in the repo to 16.7.2 for Xcode 12 compatibility

Test Plan

Running yarn in the main repo with Node 14 & Xcode 12, without this change, will cause the error copy-pasted above. After upgrading to this version, the error disappear.

@kelset kelset added 🔩Test Infrastructure Test infrastructure and continuous integration. DX Issues concerning how the developer experience can be improved. labels Oct 1, 2020
@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. Contributor A React Native contributor. labels Oct 1, 2020
@kelset kelset requested review from cpojer, hramos and alloy October 1, 2020 09:00
@kelset
Copy link
Contributor Author

kelset commented Oct 1, 2020

Looks like some CI jobs are failing, but not at the tests step.

The task failing is "Report size of RNTester.app" (which is a task after the actual testing so per se the detox bump is fine), with the error:

Missing GITHUB_PR_NUMBER. Example: 4687. PR feedback cannot be provided on GitHub without a valid pull request number.

Not sure how to fix it 😅

@kelset kelset changed the title Chore: Upgrade Detox to 16.x Chore: Upgrade Detox to 16.7.2 Oct 1, 2020
@alloy
Copy link
Contributor

alloy commented Oct 1, 2020

It would be great to get some input on what versions work best from people that actually use Detox (and RN), which isn’t me; do you?

@kelset
Copy link
Contributor Author

kelset commented Oct 1, 2020

It would be great to get some input on what versions work best from people that actually use Detox (and RN), which isn’t me; do you?

I've used Detox extensively in the first half of the year, and tbh the mindset is always to stay as close as possible to latest release because of the amount of fixes they put into each version.

That said, I think it's slightly out of scope for the purpose of this PR which is literally to just get rid of the problem I detailed above.

We could probably open a "good first issue" for folks to go over the detox testing and improve it, in the spirit of Hacktoberfest.

Copy link
Contributor

@alloy alloy left a comment

Choose a reason for hiding this comment

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

Yeah that sounds good to me 👍

@janicduplessis
Copy link
Contributor

Just hit this issue trying to yarn install the repo. This shouldn't affect people actually using detox in their app since it is only the version used for our tests.

@janicduplessis
Copy link
Contributor

cc @hramos

@tido64
Copy link
Collaborator

tido64 commented Oct 10, 2020

Looks like some CI jobs are failing, but not at the tests step.

The task failing is "Report size of RNTester.app" (which is a task after the actual testing so per se the detox bump is fine), with the error:

Missing GITHUB_PR_NUMBER. Example: 4687. PR feedback cannot be provided on GitHub without a valid pull request number.

Not sure how to fix it 😅

Can you try re-running the builds? The other PRs seems to be passing this step. Tom and I hit this issue today. Would be great to get this in.

@kelset
Copy link
Contributor Author

kelset commented Oct 12, 2020

@tido64 rerunning 🤞

@analysis-bot
Copy link

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

Base commit: d8b0e9d

@analysis-bot
Copy link

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 7,385,801 -4,096
android hermes armeabi-v7a 7,009,021 -8,192
android hermes x86 7,823,917 -8,192
android hermes x86_64 7,717,499 -4,096
android jsc arm64-v8a 9,528,006 -8,192
android jsc armeabi-v7a 9,147,130 -4,096
android jsc x86 9,392,682 -8,192
android jsc x86_64 9,974,392 -8,192

Base commit: d8b0e9d

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.

@hramos has imported 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 @kelset in fb14fd4.

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 Oct 14, 2020
@kelset kelset deleted the chore/upgrade-detox branch October 15, 2020 12:44
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. Contributor A React Native contributor. DX Issues concerning how the developer experience can be improved. Merged This PR has been merged. 🔩Test Infrastructure Test infrastructure and continuous integration.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants