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

A native Android unit-testing harness. #51479

Merged
merged 7 commits into from
Mar 18, 2024

Conversation

chinmaygarde
Copy link
Member

Sets up rules to create an APK that is comprised of solely native code. Existing executable targets (like GTests) can then use this to run on Android devices while having access to activities, windows, etc.. This allows for broader test coverage. Basically, anything that needed an ANativeWindow could only be tested in an integration test.

Executables that need access to the native activity must provide an implementation of NativeActivityMain that returns a custom subclass of flutter::NativeActivity. The native_activity_apk reads like an executable or shared_library target. Just one that packages that executable in an APK.

The APK is built using the Android Tools and does not use Gradle. Creating a new APK after invalidating some code takes ~200ms on my machine. The edit, compile, run cycle for only a tiny bit worse than testing on the host.

Builds on top of this new infrastructure to create a GTestActivity that runs an existing test suites. This works really well except the GTest suite logs to STDOUT whereas the engine logs to logcat. To quickly work around this, a custom test status listener has been wired up. This only displays the test results to logcat today but a similar mechanism can be used to talk to the test runner in the host. I will wire this up in an upcoming patch as there is no hooks into this from CI right now.

Creates an APK variant of the impeller_toolkit_android_unittests harness.

Sets up rules to create an APK that is comprised of solely native code. Existing
executable targets (like GTests) can then use this to run on Android devices
while having access to activities, windows, etc.. This allows for broader test
coverage. Basically, anything that needed an ANativeWindow could only be tested
in an integration test.

Executables that need access to the native activity must provide an
implementation of `NativeActivityMain` that returns a custom subclass of
`flutter::NativeActivity`. The `native_activity_apk` reads like an `executable`
or `shared_library` target. Just one that packages that executable in an APK.

The APK is built using the Android Tools and does not use Gradle. Creating a new
APK after invalidating some code takes ~200ms on my machine. The edit, compile,
run cycle for only a tiny bit worse than testing on the host.

Builds on top of this new infrastructure to create a `GTestActivity` that runs
an existing test suites. This works really well except the GTest suite logs to
`STDOUT` whereas the engine logs to `logcat`. To quickly work around this, a
custom test status listener has been wired up. This only displays the test
results to logcat today but a similar mechanism can be used to talk to the test
runner in the host. I will wire this up in an upcoming patch as there is no
hooks into this from CI right now.

Creates an APK variant of the `impeller_toolkit_android_unittests` harness.
@flutter-dashboard
Copy link

It looks like this pull request may not have tests. Please make sure to add tests before merging. If you need an exemption to this rule, contact "@test-exemption-reviewer" in the #hackers channel in Chat (don't just cc them here, they won't see it! Use Discord!).

If you are not sure if you need tests, consider this rule of thumb: the purpose of a test is to make sure someone doesn't accidentally revert the fix. Ask yourself, is there anything in your PR that you feel it is important we not accidentally revert back to how it was before your fix?

Reviewers: Read the Tree Hygiene page and make sure this patch meets those guidelines before LGTMing.

@chinmaygarde
Copy link
Member Author

This doesn't include the stuff on how CI is going launch and talk to such tests. A straightforward, if inelegant way would be to parse logcat. OTOH, we could write to socket and use adb port forwarding to listen for status updates.

gradle also seems to be generating and writing debug keystores to the home directory. I am not sure if it makes sense to write to stuff outside the source root out directory from a GN step. I included a debug keystore in this commit but perhaps we can make this a gclient step?

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

Just took a quick look so far. It would be nice to have a readme or examples of how to add native test cases and how to run locally. Unless you have plans to follow up with either of those?

@chinmaygarde
Copy link
Member Author

Done. I also migrated impeller_toolkit_android_unittests to this harness as an actual user.

@chinmaygarde
Copy link
Member Author

Wiring this up on CI, I do plan a followup for.

Copy link
Member

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jason-simmons jason-simmons left a comment

Choose a reason for hiding this comment

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

The overall design LGTM

testing/android/native_activity/native_activity.cc Outdated Show resolved Hide resolved
@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 18, 2024
FML_CHECK(copied != nullptr)
<< "Allocation failure while saving instance state.";
memcpy(copied, mapping->GetMapping(), mapping->GetSize());
return copied;
Copy link
Member

Choose a reason for hiding this comment

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

Also set *out_size to mapping->GetSize()

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

@jason-simmons jason-simmons removed the autosubmit Merge PR when tree becomes green via auto submit App label Mar 18, 2024
@chinmaygarde chinmaygarde added the autosubmit Merge PR when tree becomes green via auto submit App label Mar 18, 2024
@auto-submit auto-submit bot merged commit 0ee413e into flutter:main Mar 18, 2024
28 checks passed
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Mar 18, 2024
auto-submit bot pushed a commit to flutter/flutter that referenced this pull request Mar 18, 2024
…145365)

flutter/engine@89df726...0ee413e

2024-03-18 chinmaygarde@google.com A native Android unit-testing harness. (flutter/engine#51479)
2024-03-18 98614782+auto-submit[bot]@users.noreply.github.com Reverts "[Fuchsia] only download fuchsia deps when necessary (#51439)" (flutter/engine#51500)
2024-03-18 kustermann@google.com [web] Avoid using `js_util.{jsify,dartify}()` in dart2wasm for converting to JS wrappers (flutter/engine#51375)

If this roll has caused a breakage, revert this CL and stop the roller
using the controls here:
https://autoroll.skia.org/r/flutter-engine-flutter-autoroll
Please CC bdero@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human
is aware of the problem.

To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose

To report a problem with the AutoRoller itself, please file a bug:
https://issues.skia.org/issues/new?component=1389291&template=1850622

Documentation for the AutoRoller is here:
https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
@chinmaygarde chinmaygarde deleted the native_activity branch April 29, 2024 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
autosubmit Merge PR when tree becomes green via auto submit App e: impeller
Projects
None yet
3 participants