Skip to content

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

Closed
kelset wants to merge 4 commits into
mainfrom
kelset/fix-logic-version-script
Closed

fix(scripts): add logic for version scripts to account for local E2E test versioning#35846
kelset wants to merge 4 commits into
mainfrom
kelset/fix-logic-version-script

Conversation

@kelset

@kelsetkelset commented Jan 16, 2023

Copy link
Copy Markdown
Contributor

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-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 Jan 16, 2023

@cipolleschicipolleschi 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.

I left a suggestion for better error messaging plus a question.

The rest of the scripts looks good to me!

Comment threadscripts/set-rn-version.js
Comment threadscripts/test-e2e-local.js Outdated
Co-authored-by: Riccardo <cipolleschi@fb.com>
@analysis-bot

analysis-bot commented Jan 16, 2023

Copy link
Copy Markdown
PlatformEngineArchSize (bytes)Diff
androidhermesarm64-v8a8,464,873-660
androidhermesarmeabi-v7a7,785,604-676
androidhermesx868,938,342-671
androidhermesx86_648,796,410-673
androidjscarm64-v8a9,651,262+608
androidjscarmeabi-v7a8,385,729+600
androidjscx869,713,458+611
androidjscx86_6410,190,641+610

Base commit: e6d1ab9
Branch: main

@cipolleschi

Copy link
Copy Markdown
Contributor

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

@facebook-github-bot

Copy link
Copy Markdown
Contributor

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

@kelset

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
ContributorAuthor

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?

Comment threadscripts/set-rn-version.js
@facebook-github-botfacebook-github-bot added the Merged This PR has been merged. label Jan 18, 2023
@facebook-github-bot

Copy link
Copy Markdown
Contributor

@cipolleschi merged this pull request in f238f15.

@kelset
kelset deleted the kelset/fix-logic-version-script branch January 18, 2023 16:41
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: MicrosoftPartner

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants

@kelset@analysis-bot@cipolleschi@facebook-github-bot