-
Notifications
You must be signed in to change notification settings - Fork 24.3k
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
infra(e2e): rework local E2E script #34513
Conversation
Base commit: cc13b02 |
cfc606e
to
ae06068
Compare
Base commit: 97f5ef0 |
fe916e0
to
4b9c857
Compare
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: 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:
The core problem is that, within the 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: So currently I'm freezing working on this until this build hermes issue is addressed. |
9f30ff2
to
58d5a2b
Compare
update, with the latest updated patch proposed by Kudo, this works correctly now 🎉 the fix now is live here #34710, once that's merged I'll rebase this on top - and everything should be taken care of. |
There was a problem hiding this comment.
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
… 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
eb7cde2
to
d0d3f5a
Compare
e8ab62b
to
95ffd40
Compare
@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
@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 |
@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 🤞😇 |
(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) |
Can we solve the merge conflict? |
ace4995
to
bbd4d2d
Compare
@cortinico done 👍 |
Are we fine merging this? |
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? |
@dmytrorykun has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
This pull request was successfully merged by @kelset in 97f5ef0. When will my fix make it into a release? | Upcoming Releases |
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
…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
… version (facebook#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](facebook#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: facebook#34710 Test Plan: Tested by kudo in his work, and in my PR locally - [see here](facebook#34513 (comment)). Reviewed By: cortinico Differential Revision: D39647057 Pulled By: cipolleschi fbshipit-source-id: 6520e248801a307ca2f8886a3853dd1ff4af193d
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: ```sh 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 shelljs/shelljs#86 ## 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] - Migrate bash E2E local testing script to new JS based command Pull Request resolved: facebook#34513 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 ✅ Reviewed By: cortinico Differential Revision: D39814692 Pulled By: cortinico fbshipit-source-id: d4791798aaad764c6a3757269b7636f847ccf2ca
…4817) Summary: While working on facebook#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: facebook#34817 Test Plan: CI is green and when imported, nothing breaks. Reviewed By: cortinico Differential Revision: D39926953 Pulled By: cortinico fbshipit-source-id: ff66530382f891e17c00b35edf97c03591b6a9a8
…acebook#34882) Summary: Quick follow up to facebook#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: facebook#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
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
toyarn test-e2e-local <options>
.Here's the current shape of the options:
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:
Things that still need work:
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: