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

[screengrab] general improvements, removed deprecated and un-needed code #18003

Merged
merged 17 commits into from Jun 28, 2021

Conversation

penn5
Copy link
Contributor

@penn5 penn5 commented Jan 19, 2021

Checklist

  • I've run bundle exec rspec from the root directory to see all new and existing tests pass
  • I've followed the fastlane code style and run bundle exec rubocop -a to ensure the code style is valid
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary.

Motivation and Context

Currently, screengrab does not work on many devices, including Android 11 (due to scoped storage). Plus, many dependencies are outdated, and internal API hacks are (ab)used.
This PR fixes all the issues I could find.

Description

  1. Updated docs - removed unneeded permissions, fixed grammar, added simpler idioms
  2. Removed use of ActivityTestRule which is deprecated
  3. Updated gradle wrapper and removed bintray-release plugin which is outdated
  4. Changed target sdk to 30
  5. Updated dependencies
  6. Attempted (but failed) to fix issue where UiAutomatorScreengrabStrategy randomly captures too early/late. The only workaround I know is to put a Thread.sleep(1000) before each screenshot
  7. Removed need for READ/WRITE_EXTERNAL_STORAGE permission on API > 18
  8. Removed dependency on AAPT (it was only used to validate APKs, and the error is clear from stack traces generated by ADB if the user made a mistake anyway)
  9. Deprecated use_adb_root as it should be useless now (screenshots should always be readable by ADB)
  10. Use $ANDROID_DATA and $EXTERNAL_STORAGE when fetching screenshots (the path changes)
  11. Don't error when a permission grant fails (just warn)
  12. Kill app between tests to make sure the clean status bar is properly displayed
  13. Disable hidden API checks during tests, as LocaleUtils violates them
  14. Add support for restoring the previous locale (for people who don't actually use en-US as their language)
  15. Deprecated and ignored ending_locale, since it is automatically set to what the tests started at.
  16. Added theoretical suppport for testing a mix of locales at once (e.g. fr-FR, fallback to fr-CA). I didn't implement any way to actually use this, but it is used when restoring the original locale.
  17. Add shellescape the correct number of times on all variables passed to ADB

I wrote unit tests and tested on my app, as well as the sample app

Testing Steps

cd screengrab/example && bundler exec fastlane build_and_screengrab

@google-cla

This comment has been minimized.

@google-cla

This comment has been minimized.

@google-cla

This comment has been minimized.

@google-cla

This comment has been minimized.

@google-cla

This comment has been minimized.

@penn5
Copy link
Contributor Author

penn5 commented Apr 20, 2021

@joshdholtz could you review this please? I will resolve merge conflicts right now.

@joshdholtz
Copy link
Member

@penn5 Reviewing this today and working on getting screengrab in maven!

@penn5
Copy link
Contributor Author

penn5 commented May 5, 2021

@penn5 Reviewing this today and working on getting screengrab in maven!

IIRC (I haven't looked for ages) I added an alternative bintray plugin in this PR. That is obviously useless so feel free to nuke it from my branch (you should have write access) (or you can just deal with maven afterwards)

Thanks for the review!

@joshdholtz
Copy link
Member

@penn5 Just wanted to say I'm still working on this one! I wanted to get screengrab added to Maven Central which... took me an embarrassing amount of time to actually figure out 🤦‍♂️But... I have a PR that I'm merging in that makes it easy for me to upload new screengrab versions - #18672

So... once I fully review and test this PR, I will be able to upload it to Maven Central right away 🥳

@penn5
Copy link
Contributor Author

penn5 commented May 22, 2021

@penn5 Reviewing this today and working on getting screengrab in maven!

🤔

@penn5
Copy link
Contributor Author

penn5 commented Jun 16, 2021

There are a few issues to iron out (I don't know who introduced them, but I didn't spot them last time):
[x] Signing is mandatory (again?)
[x] Dependencies are out of date again
[x] Deprecation warnings are back (were they ever gone?)
[x] Spamming /usr/bin/adb -s emulator-5554 shell echo \$EXTERNAL_STORAGE to determine whether something needs run-as - this should be cached
I'll have a go at fixing them now. I fixed them.

@penn5
Copy link
Contributor Author

penn5 commented Jun 16, 2021

Aaaand I just got a deadlock in the example test suite. It's pretty rare, so I'll ignore it :P

Don't recheck the external storage paths all the time
@penn5
Copy link
Contributor Author

penn5 commented Jun 21, 2021

@joshdholtz ping (in case requesting review doesn't ping, idk how github works)

@joshdholtz
Copy link
Member

@penn5 Ah, thank you for ping! I should get a notification of review request but did not see it if it did come through 😬 Will look at this today!

@penn5
Copy link
Contributor Author

penn5 commented Jun 27, 2021

@joshdholtz if you have no more issues with this, your changes LGTM, barring potential syntax errors

Copy link
Member

@joshdholtz joshdholtz left a comment

Choose a reason for hiding this comment

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

Okay, I think this is good now! Thank you soooo much for working on this and bearing with me while I was testing it 😊 I'll get the new version of screen grab pushed to maven central right away!

@joshdholtz joshdholtz merged commit ba8be11 into fastlane:master Jun 28, 2021
@fastlane-bot
Copy link

Hey @penn5 👋

Thank you for your contribution to fastlane and congrats on getting this pull request merged 🎉
The code change now lives in the master branch, however it wasn't released to RubyGems yet.
We usually ship about once a week, and your PR will be included in the next one.

Please let us know if this change requires an immediate release by adding a comment here 👍
We'll notify you once we shipped a new release with your changes 🚀

@joshdholtz
Copy link
Member

@penn5 screengrab 2.1.0 has been pushed to maven central!

Copy link

@fastlane-bot fastlane-bot left a comment

Choose a reason for hiding this comment

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

Congratulations! 🎉 This was released as part of fastlane 2.187.0 🚀

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants