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

fix(scripts): add logic for version scripts to account for local E2E test versioning #35846

Closed
wants to merge 4 commits into from

Conversation

kelset
Copy link
Contributor

@kelset kelset commented Jan 16, 2023

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

@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. p: Microsoft Partner: Microsoft Partner labels Jan 16, 2023
Copy link
Contributor

@cipolleschi cipolleschi left a 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!

scripts/set-rn-version.js Show resolved Hide resolved
scripts/test-e2e-local.js Outdated Show resolved Hide resolved
Co-authored-by: Riccardo <cipolleschi@fb.com>
@analysis-bot
Copy link

analysis-bot commented Jan 16, 2023

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,464,873 -660
android hermes armeabi-v7a 7,785,604 -676
android hermes x86 8,938,342 -671
android hermes x86_64 8,796,410 -673
android jsc arm64-v8a 9,651,262 +608
android jsc armeabi-v7a 8,385,729 +600
android jsc x86 9,713,458 +611
android jsc x86_64 10,190,641 +610

Base commit: e6d1ab9
Branch: main

@cipolleschi
Copy link
Contributor

There are a couple of linting issues, could you take care of them, please? 🙏

@facebook-github-bot
Copy link
Contributor

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

@kelset
Copy link
Contributor Author

kelset commented Jan 17, 2023

There are a couple of linting issues, could you take care of them, please? 🙏

we really need to get this folder properly typed 😥

@facebook-github-bot
Copy link
Contributor

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

Comment on lines +42 to +46
try {
validateBuildType(buildType);
} catch (e) {
throw e;
}
Copy link
Contributor

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?

Copy link
Contributor Author

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?

scripts/set-rn-version.js Show resolved Hide resolved
@facebook-github-bot facebook-github-bot added the Merged This PR has been merged. label Jan 18, 2023
@facebook-github-bot
Copy link
Contributor

@cipolleschi merged this pull request in f238f15.

@kelset kelset deleted the kelset/fix-logic-version-script branch January 18, 2023 16:41
OlimpiaZurek pushed a commit to OlimpiaZurek/react-native that referenced this pull request May 22, 2023
…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
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. Merged This PR has been merged. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants