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

Improve test manual e2e script #28382

Closed
wants to merge 5 commits into from

Conversation

alloy
Copy link
Contributor

@alloy alloy commented Mar 24, 2020

Summary

Cleans up and improves the manual e2e test script:

  • Use yarn instead of npm to install dependencies that have previously been recorded in yarn.lock
  • Fix incorrectly opening the Xcode project, rather than workspace, of the iOS RNTestProject
  • Use RN CLI to build/run both Android and iOS test apps
  • Launch and prepare Android emulator and iOS simulators instead of requiring manual intervention:
    • Delete RNTester app
    • Delete RNTestProject app
  • Launch packager for RNTester automatically in background instead of requiring manual intervention
  • Refactor and organise differently to allow for easier future maintenance

Changelog

[Internal] [Fixed] - Fix and smoothen usage of test-manual-e2e.sh script

Test Plan

Ran it manually many times.

@alloy alloy requested review from grabbou and kelset March 24, 2020 14:44
@alloy alloy self-assigned this Mar 24, 2020
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 24, 2020
@alloy
Copy link
Contributor Author

alloy commented Mar 24, 2020

Actually still need to make the iOS side of launching/cleaning work, but need to do some work in the CLI package for that first.

Once this change is released I can wrap up these changes react-native-community/cli#1068

/cc @grabbou @thymikee

@analysis-bot
Copy link

analysis-bot commented Mar 24, 2020

Platform Engine Arch Size (bytes) Diff
ios - universal 10,800,224 0

Base commit: cf02bd9

Copy link
Contributor

@kelset kelset left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, thanks for working on this 👏

It will make everyone's life easier and better 👍

I've left a few comments but nothing blocking 👍

./gradlew :RNTester:android:app:installJscDebug || error "Couldn't build RNTester Android"

info "Deleting previously installed Android RNTester app"
adb uninstall com.facebook.react.uiapp
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice

info ""
read -n 1
adb shell am start -n com.facebook.react.uiapp/.RNTesterActivity
# Prepare
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would probably change a bit this section to 'clean up the house' more. For ex. I'd extract (similar to what happens to kill_packagers) all the cleanup part to its own method and in it, something along the lines of:

clean_folders() {
    rm -rf android
	rm react-native-*.tgz
    rm -rf /tmp/RNTestProject
	rm -rf node_modules 
}

and call it after info "Preparing version $PACKAGE_VERSION"

What do you think?

PS: maybe we could even add some more scripts to that like
rm -rf ~/Library/Developer/Xcode/DerivedData/*

# iOS
pushd RNTester
{
info "Press any key to open the workspace in Xcode, then build and test manually."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure if it should live here or not, but I was wondering if it was useful to run rm -rf ios/build...

info ""
read -n 1
success "Installing CocoaPods dependencies"
rm -rf Pods && pod install
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

...and pod cache clean --all here.
Too much?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, yeah that seems a bit much. What is your experience with this?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh just that I am a bad person and I do tabula rasa on my projects when I need to ensure full clean 😂 and you know way better than me how Pods work so I'm fully ok with not including this command 👍

cd "$repo_root"
# Test new app from template
{
npm pack
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
npm pack
yarn pack

(or is it a npm specific command?)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not really, but it also doesn’t do anything with version resolving, so I’m less worried about it. I can take a look, though, and make the whole script uniform 👍

}
}
popd
}

info "Next steps:"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tbh I think that these last 2 lines could be even be removed, I don't think they provide any actual value. Or maybe we could change the link to point to https://github.com/react-native-community/releases/blob/master/README.md or even https://github.com/react-native-community/releases/blob/master/docs/release-process.md?

@hramos hramos added p: Microsoft Partner: Microsoft Partner and removed Bug labels Mar 31, 2020
@kelset
Copy link
Contributor

kelset commented Nov 18, 2022

closing this as it's stale + we moved to a new script #34513

@kelset kelset closed this Nov 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. p: Microsoft Partner: Microsoft Partner
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants