ActivityWatch / aw-android

ActivityWatch for Android, using aw-server-rust as backend.
Mozilla Public License 2.0
155 stars 26 forks source link

test: E2E instrumented testing with basic screenshot test #88

Closed ErikBjare closed 1 year ago

ErikBjare commented 1 year ago

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.

ErikBjare commented 1 year ago

Rebased on master

ErikBjare commented 1 year ago

@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 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

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

Remaining:

ShootingKing-AM commented 1 year ago

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 https://github.com/ActivityWatch/aw-server-rust/pull/255 or https://github.com/ActivityWatch/aw-server-rust/pull/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 commented 1 year ago

@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! :)