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

Update Appium dependency version to 1.20.2 #3121

Merged
merged 5 commits into from
Feb 15, 2021
Merged

Conversation

fluiddot
Copy link
Contributor

@fluiddot fluiddot commented Feb 9, 2021

gutenberg PR: WordPress/gutenberg#28890

Description

Appium dependency has recently included a fix for preventing an error when using NPM version 7 so it would be nice to update it.

To test:
Appium is only used for running the end-to-end tests so we need to verify that they work as expected. It should be enough by verifying that the PR checks pass and by running them locally with the following commands:

  1. npm run test:e2e:ios:local for iOS
  2. npm run test:e2e:android:local for Android

PR submission checklist:

  • I have considered adding unit tests where possible.
  • I have considered if this change warrants user-facing release notes more info and have added them to RELEASE-NOTES.txt if necessary.

@fluiddot fluiddot added the dependencies Pull requests that update a dependency file label Feb 9, 2021
@fluiddot fluiddot self-assigned this Feb 9, 2021
@gziolo gziolo marked this pull request as ready for review February 12, 2021 13:03
@fluiddot fluiddot requested review from ceyhun and dcalhoun February 12, 2021 14:10
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Feb 12, 2021

Wanna run full suite of Android and iOS UI tests? Click here and 'Approve' CI job!

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

🟢 Changes look good to me.
🟢 iOS e2e tests passed locally.
🟡 Android e2e tests had oddities locally.

The gutenberg-editor-audio tests hang indefinitely for me. I waited several minutes for them to fail, but they never did. I ultimately canceled the run. I asked @jd-alexander and he had not encountered this before. I suppose since this passes on the CI, this should not be a blocker. May just be a local environment issue.

Audio block hang
audio-block-test-hang.mov

The gutenberg-editor-paste passes on my emulator device, but fails on my physical Samsung S20 5G SE device. I'm wondering if this is something specific to (A) my local setup, (B) Samsung devices, or (C) using a physical device in general. I suppose since this passes on the CI, this should not be a blocker.

Paste test failure android-paste-failures
android-paste-samsung-failure.mp4

@fluiddot did you encounter anything like this when running the Android e2e tests locally? If not, then it's likely just my local environment.

@ockham
Copy link
Contributor

ockham commented Feb 15, 2021

👋 @fluiddot @dcalhoun It seems that this is blocking WordPress/gutenberg#28890 in the Gutenberg repo, which in turn is required for npm 7 to work with GB. Any chance we'll be able to unblock things anytime soon? 😊 🙏

(edit, wrong PR)

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

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

As we discussed in Slack, I think the e2e test oddities I encountered locally are likely not of consequence. The CI suite is succeeding and that should give us confidence.

@fluiddot
Copy link
Contributor Author

As we discussed in Slack, I think the e2e test oddities I encountered locally are likely not of consequence. The CI suite is succeeding and that should give us confidence.

Yeah, I'm going to run a last test on Android just to verify there's no issue locally and then merge it, thanks!

@fluiddot
Copy link
Contributor Author

Android e2e tests had oddities locally.

The gutenberg-editor-audio tests hang indefinitely for me. I waited several minutes for them to fail, but they never did. I ultimately canceled the run. I asked @jd-alexander and he had not encountered this before. I suppose since this passes on the CI, this should not be a blocker. May just be a local environment issue.

I also experience this issue the first time I ran them, in my case the audio block was not included in the inserter menu and it was failing. I managed to fix it by uninstall the Gutenberg app from the emulator and run the command again, I think in some cases the command doesn't install a fresh build.

The gutenberg-editor-paste passes on my emulator device, but fails on my physical Samsung S20 5G SE device. I'm wondering if this is something specific to (A) my local setup, (B) Samsung devices, or (C) using a physical device in general. I suppose since this passes on the CI, this should not be a blocker.

I haven't checked e2e tests with a real device locally, I'm wondering if the issue is related to these changes, have you run e2e tests on a device previously?

@dcalhoun
Copy link
Member

I haven't checked e2e tests with a real device locally, I'm wondering if the issue is related to these changes, have you run e2e tests on a device previously?

@fluiddot no, I have not run the e2e tests on a physical device prior to this PR. I can attempt to do so on develop and see what occurs. I'll report back.

@dcalhoun
Copy link
Member

@fluiddot after running the develop branch e2e tests on a physical device, I can confirm that the paste tests fail in develop as well. This should provide additional confidence in merging this PR.

@fluiddot
Copy link
Contributor Author

As we discussed, since the CI suite is succeeding, it should be safe to merge it, thanks for the review @dcalhoun !

If we spot new issues regarding the e2e tests we'll address them in new PRs (like the one when testing with physical device).

@fluiddot fluiddot added this to the 1.47 (16.8) milestone Feb 15, 2021
@fluiddot fluiddot merged commit 950ad3f into develop Feb 15, 2021
@fluiddot fluiddot deleted the update/appium-1.20.2 branch February 15, 2021 15:34
@ockham
Copy link
Contributor

ockham commented Feb 15, 2021

Thanks a lot, folks! ❤️

@dcalhoun dcalhoun mentioned this pull request Feb 16, 2021
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants