-
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
fix(scripts): add logic for version scripts to account for local E2E test versioning #35846
Conversation
# Conflicts: # scripts/version-utils.js
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.
I left a suggestion for better error messaging plus a question.
The rest of the scripts looks good to me!
Co-authored-by: Riccardo <cipolleschi@fb.com>
Base commit: e6d1ab9 |
There are a couple of linting issues, could you take care of them, please? 🙏 |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
we really need to get this folder properly typed 😥 |
@cipolleschi has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
try { | ||
validateBuildType(buildType); | ||
} catch (e) { | ||
throw e; | ||
} |
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.
Effectively, @cortinico noticed that this should be the default behavior... @kelset do you think we can remove this try-catch?
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.
during my testing, adding this seemed to have an effect so that exec() would catch the error correctly. I can remove but honestly, what's the problem with having it?
@cipolleschi merged this pull request in f238f15. |
…test versioning (facebook#35846) Summary: While working on 0.71 branch I encountered a problem in testing locally. Basically, I was getting hit by a silent error caused by recent work facebook#35296 that didn't account for the shape of E2E local script for the release, `0.71.0-20230116-1649`. This scripts fixes both aspects: the error now gets thrown "better" and the logic accounts for the E2E shape. ## Changelog <!-- Help reviewers and the release process by writing your own changelog entry. Pick one each for the category and type tags: For more details, see: https://reactnative.dev/contributing/changelogs-in-pull-requests --> [INTERNAL] [FIXED] - add logic for version scripts to account for local E2E test versioning Pull Request resolved: facebook#35846 Test Plan: Tested via the other PR: facebook#35847 Reviewed By: cortinico Differential Revision: D42543200 Pulled By: cipolleschi fbshipit-source-id: 727eb887fcbd183ec56d8a9b7e98241eaacb1d98
Summary
While working on 0.71 branch I encountered a problem in testing locally. Basically, I was getting hit by a silent error caused by recent work #35296 that didn't account for the shape of E2E local script for the release,
0.71.0-20230116-1649
.This scripts fixes both aspects: the error now gets thrown "better" and the logic accounts for the E2E shape.
Changelog
[INTERNAL] [FIXED] - add logic for version scripts to account for local E2E test versioning
Test Plan
Tested via the other PR: #35847