Skip to content

infra(e2e): rework local E2E script - #34513

Closed
kelset wants to merge 1 commit into
mainfrom
kelset/rework-local-e2e-script
Closed

infra(e2e): rework local E2E script#34513
kelset wants to merge 1 commit into
mainfrom
kelset/rework-local-e2e-script

Conversation

@kelset

@kelsetkelset commented Aug 26, 2022

Copy link
Copy Markdown
Contributor

Summary

This is a long time coming effort to improve the situation around the local e2e script that in the release crew: the current bash-based script is quirky at best, and what you end up generating as a sample project is not really a true sample project. This is where this PR comes in: it migrates the flow from ./scripts/test-manual-e2e.sh to yarn test-e2e-local <options>.

Here's the current shape of the options:

Options:
--help Show help [boolean]
--version Show version number [boolean]
-t, --target [choices: "RNTester", "RNTestProject"] [default: "RNTester"]
-p, --platform [choices: "iOS", "Android"] [default: "iOS"]
-h, --hermes [boolean] [default: true]

The idea is to change it so that you can just run the script, and it will do that one specific thing "well", without the tester needing to do anything aside from actually testing the app once it's open.

Some of the key changes:

  • tries to stick to the patterns of the other established *.js based scripts, in terms of tooling and approach (and even refactor parts that can be shared with other scripts) - like the android artifacts generation
  • no need to start the android emulator on the side
  • no need to start Metro on the side
  • RNTester iOS will open up on the simulator (no Xcode open that then you need to press)

Things that still need work:

  • see the #fixme and #todo in comments
  • because we rely on exec, the output sent back is not formatted/shaped correctly so it's a bit more noisy/chaotic - but can't handle it right now because the package we use doesn't allow it - see Preserve colors in output of exec() shelljs/shelljs#86

Changelog

[Internal] [Changed] - Migrate bash E2E local testing script to new JS based command

Test Plan

To test the script, you can run it passing the options showed above; this is the current situation:

  • RNTester iOS Hermes ✅
  • RNTester Android Hermes ✅
  • RNTester iOS JSC ✅
  • RNTester Android JSC ✅
  • RNTestProject Android Hermes ✅
  • RNTestProject iOS Hermes ✅
  • RNTestProject Android JSC ✅
  • RNTestProject iOS JSC ✅

@kelset
kelset requested a review from hramos as a code ownerAugust 26, 2022 17:56
@facebook-github-botfacebook-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. p: Microsoft Partner: Microsoft Partner labels Aug 26, 2022
@kelset
kelset marked this pull request as draft August 26, 2022 17:57
@github-actions

github-actionsBot commented Aug 26, 2022

Copy link
Copy Markdown
Warnings
⚠️🔒 package.json - Changes were made to package.json. This will require a manual import by a Facebook employee.

Generated by 🚫 dangerJS against bbd4d2d

@analysis-bot

analysis-bot commented Aug 26, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
ios-universaln/a--

Base commit: cc13b02
Branch: main

@kelset
kelsetforce-pushed the kelset/rework-local-e2e-script branch from cfc606e to ae06068CompareSeptember 12, 2022 13:36
@analysis-bot

analysis-bot commented Sep 12, 2022

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a7,739,292-70
androidhermesarmeabi-v7a7,141,935-93
androidhermesx868,050,381-91
androidhermesx86_648,022,197-104
androidjscarm64-v8a9,607,479-70
androidjscarmeabi-v7a8,372,968-103
androidjscx869,555,141-77
androidjscx86_6410,147,725-96

Base commit: 97f5ef0
Branch: main

@kelset
kelsetforce-pushed the kelset/rework-local-e2e-script branch 2 times, most recently from fe916e0 to 4b9c857CompareSeptember 15, 2022 10:32
@kelset

Copy link
Copy Markdown
ContributorAuthor

update on solving the Hermes on iOS for RNTestProject: I managed to figure out the logic to make it build from source on the new project for when testing in main branch and from release branch.

BUT this caused this to happen:
Screenshot_2022-09-14_at_18 24 41

Basically, on the Android side everything gets build with the correct configuration, while on iOS not. This took a lot of digging, and a bit of fortune and "lucky timing", but basically now we know what's going on (it's the same issue @Kudo is trying to address here). Long story short, how hermes gets build on iOS has 3 different variations:

  • there's build-apple-framework.sh in the Hermes repo, that runs this Cmake command
  • there's build-apple-framework.sh in the React Native repo, that runs this variation of the Cmake command that in particular has this extra option -DHERMES_RELEASE_VERSION="for RN $(get_release_version)" \ that makes the right version of Hermes be generated (and displayed in a fresh new project)
  • there's a variation of the above that is run in the RN CI, but it's all "manual" while the ones above are run through cocoapods. See here the scripts.

The core problem is that, within the hermes-engine.podspec contained in the RN repo (at react-native/main/sdks/hermes-engine/), there's a bit of logic that triggers that script here (through ./utils/build-ios-framework.sh). The issue is that we all thought that that ./utils/build-ios-framework.sh would invoke the React native version of the scripts (since the podspec file lives right next to the utils folder) but, in reality, it doesn't. It just so happens that the Hermes repo has a root level utils folder which is (you guessed it) where the Hermes variation of those build scripts live.
So, when running the pod install command in a react-native project (build from source), it will go and download the hermes source code (since the source[:git] gets set) but then it will use the hermes variation of the build-*.sh scripts.

So we need to figure out how to get the pod process to build using the React Native variation of those scripts. @Kudo has proposed a patch here that would have some more explicit pathing - which seems a good workaround, but it doesn't seem to do the trick:
Screenshot 2022-09-15 at 16 31 27

So currently I'm freezing working on this until this build hermes issue is addressed.

@kelset
kelsetforce-pushed the kelset/rework-local-e2e-script branch from 9f30ff2 to 58d5a2bCompareSeptember 16, 2022 13:23
@kelset

kelset commented Sep 16, 2022

Copy link
Copy Markdown
ContributorAuthor

update, with the latest updated patch proposed by Kudo, this works correctly now 🎉

Screenshot 2022-09-16 at 15 24 19


the fix now is live here #34710, once that's merged I'll rebase this on top - and everything should be taken care of.

@cortinicocortinico left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Amazing stuff @kelset! Left minor comments around

Comment threadscripts/testing-utils.js Outdated
Comment threadscripts/test-e2e-local-clean.js
Comment threadscripts/test-e2e-local.js Outdated
Comment threadscripts/test-e2e-local.js
Comment threadscripts/test-e2e-local.js
Comment threadscripts/test-e2e-local.js Outdated
facebook-github-bot pushed a commit that referenced this pull request Sep 26, 2022
… version (#34710)
Summary:
Within the `hermes-engine.podspec` contained in the RN repo (at `react-native/main/sdks/hermes-engine/`), there's a bit of logic that triggers `./utils/build-ios-framework.sh` and `./utils/build-mac-framework.sh` .
The issue is that we all thought that that `./utils/build-ios-framework.sh` would invoke the React native version of the scripts (since the podspec file lives right next to the `utils` folder) but, in reality, it doesn't. It just so happens that the Hermes repo has a root level `utils` folder which is (you guessed it) where the Hermes variation of those build scripts live.
So, when running the pod install command in a react-native project (build from source), it will go and download the hermes source code (since the `source[:git]` gets set) but then it will use the **hermes** variation of the `build-*.sh` scripts.
[Read more here](#34513 (comment)).
This PR is taking kudo's proposed [patch here](reactwg/react-native-new-architecture#68 (reply in thread)) - props for the fix go to him.
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[iOS] [Fixed] - Change hermes logic in build scripts for Apple to use the correct files
Pull Request resolved: #34710
Test Plan: Tested by kudo in his work, and in my PR locally - [see here](#34513 (comment)).
Reviewed By: cortinico
Differential Revision: D39647057
Pulled By: cipolleschi
fbshipit-source-id: 6520e248801a307ca2f8886a3853dd1ff4af193d
@kelset
kelsetforce-pushed the kelset/rework-local-e2e-script branch 2 times, most recently from eb7cde2 to d0d3f5aCompareSeptember 26, 2022 10:57
@kelset
kelset marked this pull request as ready for review September 26, 2022 13:24
@facebook-github-botfacebook-github-bot added the Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. label Sep 26, 2022
@kelset
kelsetforce-pushed the kelset/rework-local-e2e-script branch 2 times, most recently from e8ab62b to 95ffd40CompareSeptember 26, 2022 13:39
@kelsetkelset changed the title infra(e2e): rework local E2E script (WIP)infra(e2e): rework local E2E scriptSep 26, 2022
@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@kelset

Copy link
Copy Markdown
ContributorAuthor

@dmytrorykun just an headsup, I'm testing on a release branch and I've hit an issue... I'll let you know once I've fixed it

@kelset

Copy link
Copy Markdown
ContributorAuthor

@dmytrorykun / @cortinico ok nvm the issue I was hitting was caused by me not having cherry-picked all the necessary changes back on top of 0.70 to test my script correctly; after a bit of investigation it's clear that it's tooo complicated to backport this script so we'll have to face potential/eventual issues during the 0.71 RC phase - but on paper everything should work fine 🤞😇

@kelset

Copy link
Copy Markdown
ContributorAuthor

(note to self: this page of the docs https://reactnative.dev/contributing/how-to-contribute-code#development-workflow needs to be updated once this is merged)

@cortinico

Copy link
Copy Markdown
Contributor

Can we solve the merge conflict?

@kelsetkelset mentioned this pull request Oct 3, 2022
@kelset
kelsetforce-pushed the kelset/rework-local-e2e-script branch from ace4995 to bbd4d2dCompareOctober 3, 2022 13:59
@kelset

Copy link
Copy Markdown
ContributorAuthor

@cortinico done 👍

@cortinico

Copy link
Copy Markdown
Contributor

Are we fine merging this?

@kelset

kelset commented Oct 3, 2022

Copy link
Copy Markdown
ContributorAuthor

from my perspective we should be good to go; we won't cherry pick it back into 0.70 or older but at least starting from 0.71 we'll have a better DX for testing, and we can tell people to use it too.

It also brings along a couple of small improvements so it'd be good to merge :3

Maybe you want to checkout the branch locally and run it once just to verify it works on other setups other than my machine?

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@react-native-bot

Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @kelset in 97f5ef0.

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

@react-native-botreact-native-bot added the Merged This PR has been merged. label Oct 4, 2022
@kelset
kelset deleted the kelset/rework-local-e2e-script branch October 4, 2022 12:00
@kelset
kelset restored the kelset/rework-local-e2e-script branch October 4, 2022 12:00
@kelset
kelset deleted the kelset/rework-local-e2e-script branch October 4, 2022 12:11
facebook-github-bot pushed a commit that referenced this pull request Oct 4, 2022
Summary:
While working on #34513 I noticed that on main branch the versioning is not really consistent everywhere. So this PR is an attempt at realigning so that on the main branch, RN is 1000.0.0 everywhere - in a way, it's cleaning up the room for the monorepo work to go flawlessly).
It's just a pass of `node scripts/set-rn-version.js --to-version 1000.0.0`.
There's the small chance that some versions where kept to 0.0.0 on purpose (build tools are weird), so we might just have to close this off. No big deal :)
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[Internal] [Changed] - re-align version to be 1000.0.0 on main everywhere
Pull Request resolved: #34817
Test Plan: CI is green and when imported, nothing breaks.
Reviewed By: cortinico
Differential Revision: D39926953
Pulled By: cortinico
fbshipit-source-id: ff66530382f891e17c00b35edf97c03591b6a9a8
facebook-github-bot pushed a commit that referenced this pull request Oct 7, 2022
…34882)
Summary:
Quick follow up to #34513 to fix an issue that has been bothering the release crew for a while: the iOS new arch component not working! Turns out, we're silly billies �
## Changelog
<!-- Help reviewers and the release process by writing your own changelog entry. For an example, see:
https://reactnative.dev/contributing/changelogs-in-pull-requests
-->
[Internal] [Changed] - Add new arch flag to iOS pod install command in E2E script
Pull Request resolved: #34882
Test Plan:
Everything works correctly now:
<img width="987" alt="Screenshot 2022-10-06 at 14 20 09" src="https://user-images.githubusercontent.com/16104054/194327768-4da7d607-879b-46ad-a453-504983980831.png">
Reviewed By: dmytrorykun
Differential Revision: D40143251
Pulled By: dmytrorykun
fbshipit-source-id: 91ba6e22c25770efe6a839d6728d7052bc17a8f2
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA SignedThis label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.ContributorA React Native contributor.MergedThis PR has been merged.p: MicrosoftPartner: MicrosoftPartnerShared with MetaApplied via automation to indicate that an Issue or Pull Request has been shared with the team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants

@kelset@analysis-bot@facebook-github-bot@cortinico@react-native-bot@vincenzovitale