react-native icon indicating copy to clipboard operation
react-native copied to clipboard

Extend the local e2e test case to provide the REACT_NATIVE_MAVEN_LOCAL_REPO property

Open kelset opened this issue 3 years ago • 2 comments

Summary

This PR is a follow up of https://github.com/facebook/react-native/pull/35075/files to ensure that even in the local e2e testing scenario the new maven approach is followed - without this, RNTestProject on Android won't work, like so: Screenshot 2022-10-27 at 12 15 38

Changelog

[Internal] [Fixed] - Extend the local e2e test case to provide the REACT_NATIVE_MAVEN_LOCAL_REPO property

Test Plan

Run yarn test-e2e-local -t RNTestProject -p Android successfully. (which is not what's happening now, reason why PR is still in draft)

kelset avatar Oct 27 '22 13:10 kelset

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

Base commit: 64c3906280bbb643f758fa86f92a6d179007e36b Branch: main

analysis-bot avatar Oct 27 '22 13:10 analysis-bot

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 6,992,247 +0
android hermes armeabi-v7a 6,368,778 +0
android hermes x86 7,404,941 +0
android hermes x86_64 7,268,948 +0
android jsc arm64-v8a 8,855,839 +0
android jsc armeabi-v7a 7,594,355 +0
android jsc x86 8,913,589 +0
android jsc x86_64 9,396,951 +0

Base commit: 64c3906280bbb643f758fa86f92a6d179007e36b Branch: main

analysis-bot avatar Oct 27 '22 13:10 analysis-bot

@kelset I am planning to rename the tarball methods in hermes-utils.js in order to make a clear distinction between the different tarballs that we deal with:

  • Hermes source code tarball, generated by GitHub and downloaded from github.com/facebook/hermes
  • Hermes prebuilt artifact tarball, generated by Circle CI and downloaded from Maven
  • Hermes debug symbols tarball, generated by Circle CI and downloaded from Maven (WIP)

Here's the PR: https://github.com/facebook/react-native/pull/35156 - I have updated all the internal references to these methods.

I realize this would conflict with your PR. What do you think? Would renaming these have an adverse impact on other scripts from the community? Would you be okay rebasing your work on top of https://github.com/facebook/react-native/pull/35156 if it is merged?

hramos avatar Oct 31 '22 16:10 hramos

@hramos go ahead and merge yours, yeah I'll have conflicts but I'm still in the testing phase for this one so it's likely that your PR will be quicker to land

kelset avatar Oct 31 '22 17:10 kelset

Question/idea for a later PR: do we want to setup a circleci job that runs this script? In that way, if someone commits something that could potentially break it, the check will ask him to also fix the script.

WDYT?

cipolleschi avatar Nov 01 '22 09:11 cipolleschi

update: now the PR should be ready, I've run a few more tests and done more tweaks. Every flow seems to be working as expected.

re: @cipolleschi

Question/idea for a later PR: do we want to setup a circleci job that runs this script? In that way, if someone commits something that could potentially break it, the check will ask him to also fix the script.

No, I don't think there's value in running this script specifically - on CI it'd basically just build RNTester and RNTestProject, take a lot of time and not verify much that is not already verified by other CI jobs. The main advantage of this script is in the local scenario, where you want to have 1 handy command to get easily to have a local build working so that you can play around with it in the emulator/simulator.

This topic overlaps with the E2E conversation that is been happening in parallel, so I'm sure that we'll be able to do some refactoring so that both the E2E flows and this script will have less custom code and we can generalize certain sections

kelset avatar Nov 01 '22 12:11 kelset

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Nov 01 '22 12:11 facebook-github-bot

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Nov 01 '22 14:11 facebook-github-bot

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Nov 02 '22 11:11 facebook-github-bot

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Nov 02 '22 12:11 facebook-github-bot

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Nov 02 '22 13:11 facebook-github-bot

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Nov 02 '22 14:11 facebook-github-bot

quick headsup, I've updated this PR to account for some of the commits that landed in the last 12/24 hrs, such as:

  • https://github.com/facebook/react-native/commit/d71d0db51dec825ed9151ea02533279411381bb2
  • https://github.com/facebook/react-native/commit/b5bb227be88fc89538279cfd84fbec810e2b8aa2

And especially https://github.com/facebook/react-native/commit/6b8e13f53c5fc9c5e794bb7491648e4b25ad5811 which for some reason did add some new scripts but not as executable, so took care of it.

Before: Screenshot 2022-11-02 at 12 43 43

After: Screenshot 2022-11-02 at 12 50 32

kelset avatar Nov 02 '22 14:11 kelset

final update (I hope 🤣): everything working as expected: Screenshot 2022-11-02 at 14 27 15

kelset avatar Nov 02 '22 14:11 kelset

@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

facebook-github-bot avatar Nov 02 '22 14:11 facebook-github-bot

This pull request was successfully merged by @kelset in c540ff7bd156a3b1636161a42128dfc2b575e031.

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

react-native-bot avatar Nov 02 '22 15:11 react-native-bot