-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[RNMobile] Cache the Web Driver Agent in iOS e2e tests workflow #29831
Merged
Merged
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I think having another code path just for local CI could further complicate things. WDYT about adding
test:e2e:build-wda
intest:e2e:ios:local
as an additional step and removing the check for CI so both local and CI runs stay the same?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.
The difference between local and CI regarding the command
test:e2e:build-wda
is that for CI the WDA cache gets invalidated with the build cache key so if there's any changes in the native side, the WDA is rebuilt but this won't happen for local. This could produce an error if you change for example the Xcode version without deleting the derived data folderios/build/WDA
.For local, my idea was to let Appium decide when the WDA should be rebuilt, that's why we have another code path but I'd be happy to figure out a different approach.
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.
When we put
test:e2e:build-wda
insidetest:e2e:ios:local
it should also always rebuild WDA when ran locally, right? We can add arm -rf ios/build/WDA
intest:e2e:build-wda
to be safe, but it seems to me that it's the same case for building withtest:e2e:build-app:ios
locally and I don't think it produces an error, so maybe even therm -rf
isn't necessary.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.
Just a note here if we go the
rm -rf ios/build/WDA
to utilize https://www.npmjs.com/package/rimraf for cross-platform compatibility. If it's not needed here then please do ignore 🙇🏾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.
Yeah you're right, we can run the command
test:e2e:build-wda
when runningtest:e2e:ios:local
. I thought that the workflow was runningtest:e2e:ios:local
but it's not, sorry for the misleading. Since this command is only run locally, it makes sense to also build the WDA. This would also require to pass the WDA derived data folder as an environment variable when running it from Gutenberg-mobile, as we do with the app’s path but nothing else.Regarding removing the derived data folder, I'm going to check it but most likely it won't be necessary.
Thanks for the feedback @ceyhun and @jd-alexander 🙇 !
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.
I applied the changes from these comments in this commit. I'm going to open a Gutenberg-mobile PR with the required changes to run the e2e tests there too.
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.
Here is the PR in Gutenberg-mobile related to these changes, @ceyhun and @jd-alexander I've assigned you as reviewers there too.