-
Notifications
You must be signed in to change notification settings - Fork 24.4k
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
Unify test-e2e
command in rn-tester-e2e
pacakge
#38701
Unify test-e2e
command in rn-tester-e2e
pacakge
#38701
Conversation
Base commit: 5eaf28b |
@cortinico has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
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.
Hi @szymonrybczak, thank you a lot for this PR.
I left a couple of comments to improve code organization and relieve maintenance burden. Let me know what do you think!
@@ -0,0 +1,13 @@ | |||
#!/bin/bash |
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.
given that this is a new script. Can we have it in JS instead of in bash?
This will make it more portable and JS is easier to read and reason about. Also, we have linters and formatters for it.
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.
Also... can we create a folder into scripts/e2e
where we put these scripts? This will keep the folder more organized.
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.
good idea, changed.
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.
Also... can we create a folder into scripts/e2e where we put these scripts? This will keep the folder more organized.
Would you like me to move all scripts related to running e2e to this folder?
1fa4d5c
to
a358592
Compare
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.
Thanks for updating the PR!
@cortinico 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 @szymonrybczak in 378aa72. When will my fix make it into a release? | Upcoming Releases |
Summary:
This PR is small cleanup in scripts in
rn-tester-e2e
package, it creates unified script so that we can easily pass platform as an argument. Context: #36267 (comment)Changelog:
[INTERNAL] [CHANGED] - Unify
test-e2e
command inrn-tester-e2e
packageTest Plan:
CI Green ✅