Skip to content

chore: Add menu to test-manual-e2e script#32911

Closed
gabrieldonadel wants to merge 2 commits into
facebook:mainfrom
gabrieldonadel:update-test-manual-e2e-script
Closed

chore: Add menu to test-manual-e2e script#32911
gabrieldonadel wants to merge 2 commits into
facebook:mainfrom
gabrieldonadel:update-test-manual-e2e-script

Conversation

@gabrieldonadel
Copy link
Copy Markdown
Collaborator

Summary

Add a menu to the test-manual-e2e script in order to allow a user to select which scenario they want to test rather than having to run them all in order.

Closes #32679

Changelog

[Internal] [Added] - Add menu to test-manual-e2e.sh in order to accept configurations

Test Plan

Manually run ./scripts/test-manual-e2e.sh and test all menu options

Screen.Recording.2022-01-17.at.23.35.05.mov

@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. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. labels Jan 18, 2022
@react-native-bot react-native-bot added the Type: Enhancement A new feature or enhancement of an existing feature. label Jan 18, 2022
@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Jan 18, 2022

Platform Engine Arch Size (bytes) Diff
ios - universal n/a --

Base commit: 2151d11
Branch: main

@analysis-bot
Copy link
Copy Markdown

analysis-bot commented Jan 18, 2022

Platform Engine Arch Size (bytes) Diff
android hermes arm64-v8a 8,291,745 -275
android hermes armeabi-v7a 7,628,857 -280
android hermes x86 8,764,469 -278
android hermes x86_64 8,701,379 -279
android jsc arm64-v8a 9,678,790 -124
android jsc armeabi-v7a 8,672,604 -114
android jsc x86 9,637,026 -115
android jsc x86_64 10,231,535 -121

Base commit: 2151d11
Branch: main

@lunaleaps
Copy link
Copy Markdown
Contributor

Amazing! Let me take a closer look later today! Thanks so much for working on this!

@yungsters
Copy link
Copy Markdown
Contributor

Neat! Minor suggestion… when expecting input from the user, maybe prepend the line with > .

Which app do you want to test?
  1 - RNTester
  2 - A new app using the template
> …

@gabrieldonadel
Copy link
Copy Markdown
Collaborator Author

Neat! Minor suggestion… when expecting input from the user, maybe prepend the line with > .

Which app do you want to test?
  1 - RNTester
  2 - A new app using the template
> …

Sure @yungsters, I've just updated it to a prepend the line with >
image

@cortinico
Copy link
Copy Markdown
Contributor

Add a menu to the test-manual-e2e script in order to allow a user to select which scenario they want to test rather than having to run them all in order.

That's great stuff. Just a minor question: wouldn't supporting cli arguments a better approach here? Having something like --android-only, --hermes-only or so?

Asking as if this is happening inside a development loop, having to go through a menu selection will introduce a lot of friction.

@gabrieldonadel
Copy link
Copy Markdown
Collaborator Author

Add a menu to the test-manual-e2e script in order to allow a user to select which scenario they want to test rather than having to run them all in order.

That's great stuff. Just a minor question: wouldn't supporting cli arguments a better approach here? Having something like --android-only, --hermes-only or so?

Asking as if this is happening inside a development loop, having to go through a menu selection will introduce a lot of friction.

@lunaleaps @yungsters thoughts on this? I personally don't have much context of how/when this is used so my opinion wouldn't really add much value here

@lunaleaps
Copy link
Copy Markdown
Contributor

Yea that's a good suggestion, maybe we can do both? Currently this isn't part of a dev loop but it would be great to use this for our CI tests

@gabrieldonadel
Copy link
Copy Markdown
Collaborator Author

gabrieldonadel commented Jan 19, 2022

Yea that's a good suggestion, maybe we can do both? Currently this isn't part of a dev loop but it would be great to use this for our CI tests

How would this work? Would we bypass the menu if any arguments were provided? If that's the case what do you think about supporting the following arguments?

  • --android-only (would run all Android related tests)
  • --ios-only (would run all iOS related tests)
  • --hermes-only (would run both Android and iOS tests using hermes)
  • --jsc-only (would run both Android and iOS tests using JSC)
  • --all (would run all tests)

Combining arguments examples:

  • --android-only --hermes-only (would run Android tests using hermes)
  • --ios-only --jsc-only (would run iOS tests using JSC)

Also, should I tackle this in a follow-up PR or should I add these changes to this one?

cc: @lunaleaps @cortinico

@cortinico
Copy link
Copy Markdown
Contributor

How would this work? Would we bypass the menu if any arguments were provided?

My 2 cents: I think the menu could be ditched as it probably adds a bit of complexity to the script. It's a nice to have though so as it's already coded, I think we can eventually keep it.

Anyway I believe you should just skip the menu if any positional argument is passed 👍

I would suggest to:

  • --android (would run all Android related tests)
  • --ios (would run all iOS related tests)
  • --hermes (would run both Android and iOS tests using hermes)
  • --jsc (would run both Android and iOS tests using JSC)

Also remove the --all as that's the default.

Combining arguments should intersect them:

  • --android --hermes (would run Android tests using hermes)
  • --ios --jsc (would run iOS tests using JSC)

@lunaleaps
Copy link
Copy Markdown
Contributor

Yup all sound good, re: separate PR or this one, maybe let's merge this one and do a follow-up?

@facebook-github-bot
Copy link
Copy Markdown
Contributor

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

@gabrieldonadel
Copy link
Copy Markdown
Collaborator Author

Yup all sound good, re: separate PR or this one, maybe let's merge this one and do a follow-up?

Cool, sounds good to me

@react-native-bot
Copy link
Copy Markdown
Collaborator

This pull request was successfully merged by @gabrieldonadel in 1ec3997.

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

@react-native-bot react-native-bot added the Merged This PR has been merged. label Jan 20, 2022
@gabrieldonadel gabrieldonadel deleted the update-test-manual-e2e-script branch January 20, 2022 20:13
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. Merged This PR has been merged. Shared with Meta Applied via automation to indicate that an Issue or Pull Request has been shared with the team. Type: Enhancement A new feature or enhancement of an existing feature.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Releases] Update test-manual-e2e.sh to accept configurations for ios, android, hermes, jsc, etc.

7 participants