Skip to content

test: E2E instrumented testing with basic screenshot test #88

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

Merged
merged 11 commits into from
Nov 25, 2022

Conversation

ErikBjare
Copy link
Member

@ErikBjare ErikBjare commented Nov 24, 2022

Continuing work by @ShootingKing-AM in #85

Playing around, just making a new PR to not mess up changes in #85.

Made it reuse apks from previous build step, along with a bunch of other changes/fixes.

- specify jdk 1.8
- make jni debuggable
- Min targetSDK must be >=30 for playstore now
* Test Screenshot ci (#2)
- grant test app permissions in advance
- fix SettingsWatcher to properly get Premissions if allowed
@ErikBjare ErikBjare force-pushed the dev/instrumentation-e2e branch from 6a61d59 to a3b55db Compare November 24, 2022 18:40
@ErikBjare
Copy link
Member Author

Rebased on master

@ErikBjare ErikBjare force-pushed the dev/instrumentation-e2e branch from cf95b49 to 780db09 Compare November 24, 2022 19:05
@ErikBjare ErikBjare force-pushed the dev/instrumentation-e2e branch 14 times, most recently from d76ecc0 to 3ddf622 Compare November 24, 2022 23:16
@ErikBjare ErikBjare force-pushed the dev/instrumentation-e2e branch from 3ddf622 to 1ac31df Compare November 24, 2022 23:32
@ErikBjare ErikBjare force-pushed the dev/instrumentation-e2e branch from b284558 to e5fa4d2 Compare November 25, 2022 00:47
@ErikBjare ErikBjare changed the title e2e instrumented testing test: E2E instrumented testing with basic screenshot test Nov 25, 2022
@ErikBjare
Copy link
Member Author

@ShootingKing-AM What do you think about this? Almost there!

I can't seem to get reports to work with adb shell am instrument, apparently not possible: https://stackoverflow.com/questions/50765454/how-to-generate-html-report-while-running-am-instrument-command-uiautomator

Couldn't figure out how to use Gradle in the e2e job without having to setup Java+SDK+NDK + rebuild the whole thing, but maybe possible somehow.

How does it look to you otherwise? Any thoughts? :)

@ShootingKing-AM
Copy link
Contributor

ShootingKing-AM commented Nov 25, 2022

Reports are not necessary for now i guess. Agree that #85 is re-doing many things for test which needs to be optimized (wrote as todo)

I feel that it's better to use gradle to run instrumentation,

  1. pretty sure it abstracts adb to ease testing for devs. If we intend to use adb again, we will be writing gradle's internal code again in a different language :(
  2. And i am not sure where the screenshot would be saved in case of adb. (Now it's being handled by uiautomator, espresso and gradle in backend)
  3. Gradle will have more features in future like Gradle managed devices for scaling up testing. Google devs will take care of our CI emu matrix (i hope)
  4. Wish to have a basic e2e CI friendly test, let google devs work on ci-gradle integrations and easing up overhead of instrumentation testing, we can spend our time better by writing actual tests for aw :)
  5. Regarding the time CI is taking, anyways we are committing to master, weekly or bi-weekly so, 30mins per CI run per PR is should be fine

@ErikBjare ErikBjare force-pushed the dev/instrumentation-e2e branch from 190dc70 to 9c76704 Compare November 25, 2022 12:32
@ErikBjare
Copy link
Member Author

@ShootingKing-AM Using gradle is fine, but as I said, we need a way to run it without rebuilding the entire thing and duplicating tons of steps. Do you know a way?

pretty sure it abstracts adb to ease testing for devs. If we intend to use adb again, we will be writing gradle's internal code again in a different language

Not sure what you mean, testing using adb+apk directly seems pretty straight-forward the way I do it here?

And i am not sure where the screenshot would be saved in case of adb.

Easy to fix, can be retrieved with adb pull ... from somewhere (looking it up now).

Gradle will have more features in future like Gradle managed devices for scaling up testing.

Feels excessive at this point. Let's get it working first.

Regarding the time CI is taking, anyways we are committing to master, weekly or bi-weekly so, 30mins per CI run per PR is should be fine

It's fine as long as the tests pass every time (unlikely), it's not fine when they fail! (waiting 30min for CI is not acceptable imo)

It's also a mess and very bug-prone to have to maintain two almost identical build-jobs, we should not have to build the artifacts twice.

@ErikBjare
Copy link
Member Author

ErikBjare commented Nov 25, 2022

@ShootingKing-AM I think this is good enough to merge soon. Still some work left to do, but at least it runs successfully!

Remaining:

  • Screenshot instrumented test doesn't seem to run? (no log output)
  • adb pull screenshots from instrumented test

@ShootingKing-AM
Copy link
Contributor

ShootingKing-AM commented Nov 25, 2022

Using gradle is fine, but as I said, we need a way to run it without rebuilding the entire thing and duplicating tons of steps. Do you know a way?

No I don't know a way to use gradle to install and test already built apk

Not sure what you mean, testing using adb+apk directly seems pretty straight-forward the way I do it here?

Those are for basic tests, Screenshot test needs some services/helpers.
Anyways, fyi code is majorly used from https://github.com/android/testing-samples/tree/main/ui/espresso/ScreenshotSample and they suggested using ./gradle -cAT
By abstraction I mean, gradle internally uses adb devices/pull/push/am etc.. to run tests, and get some useful data from them to ease developer workload whilst testing. Can assure that adb+apk will break very soon given android development esp. wrt to ci-instrumentation testing is very rapid.

It's fine as long as the tests pass every time (unlikely), it's not fine when they fail! (waiting 30min for CI is not acceptable imo)

#85 is not intended to replace local emulator-based or device-based instrumentation testing. Its just to catch and alert devs that aw-android fails to run for eg. in cases like ActivityWatch/aw-server-rust#255 or ActivityWatch/aw-server-rust#324.
Intended workflow is aw-server-rust/aw-android CI test fails -> Simulate and test locally -> fix -> test locally -> commit. We need to at least test for 3SDK levels - minSdkLevel, targetSdkLevel, playstoreMinSdkLevel (31 iirc)

Easy to fix, can be retrieved with adb pull ... from somewhere (looking it up now).

I feel that we are doing unnecessary work :( Why to even look up and maintain it for future, if there are some changes in espressor/uiautomator as to where the screenshot() output will go, that would be compensated by simultaneous gradle version updates and we will still be getting the screenshots into /additionalTestOutputs/ by gradle connectedAndroidTest

It's also a mess and very bug-prone to have to maintain two almost identical build-jobs, we should not have to build the artifacts twice.

True, but its just a compile and test process, also felt that this is much easier and maintainable, since, gradle has to build and emu has macos-requirements(for now)

Overall, at the end of day, need a e2e ci-friendly test which would have minimum maintenance work. I still strongly feel that adb based instrumentation is not the way-forward, rest of course is your wish :)

@ErikBjare
Copy link
Member Author

@ShootingKing-AM Ah fair enough! Those are some very valid points, I'm still learning a lot about all of this.

That being said, I'm probably going to go ahead and merge this for now, since it's definitely an improvement over nothing at all and a minimal almost-working setup that is easier to continue working on.

Please feel free to keep working on this in a new PR. Thank you so much for getting this much-needed work started in #85! :)

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

Successfully merging this pull request may close these issues.

2 participants