added unit tests for the android embedder that run on android devices #28784

gaaclarke · 2021-09-21T22:34:38Z

Created a test runner that can execute Android embedder code on an Android device. This matches the capabilities that we have for iOS with iOS with IosUnitTests. Before this all c++ files in shell/platform/android were unable to be unit tested. Since scenario_app isn't working for Android there is was no test coverage for these files in the engine repository.

This will be used to create the unit tests to resolve flutter/flutter#90398. The AndroidShellHolder test is hitting the code that we were concerned about.

We previously discussed something that would execute on the host platform with SwiftShader but that would be more work to get android code compiling for the host platform and be less useful since this actually runs on the target device.

Pre-launch Checklist

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I read and followed the Flutter Style Guide and the C++, Objective-C, Java style guides.
I listed at least one issue that this PR fixes in the description above.
I added new tests to check the change I am making or feature I am adding, or Hixie said the PR is test-exempt. See testing the engine for instructions on
writing and running engine tests.
I updated/added relevant documentation (doc comments with ///).
I signed the CLA.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

jason-simmons · 2021-09-23T18:16:40Z

shell/platform/android/android_context_gl.cc

-      display, config_, reinterpret_cast<EGLNativeWindowType>(window->handle()),
-      attribs);
-  return std::make_unique<AndroidEGLSurface>(surface, display, context_);
+  if (window->IsOffscreen()) {


This is imprecise - an AndroidNativeWindow can be backed by a real window surface that is positioned offscreen.

Change this to something like IsFakeWindow and emphasize that this is only provided to support tests and will not happen on a real device.

Done, the docstring mentions it's for tests.

jason-simmons · 2021-09-23T18:17:22Z

shell/platform/android/android_context_gl.cc

+  if (window->IsOffscreen()) {
+    const EGLint attribs[] = {EGL_WIDTH, 1, EGL_HEIGHT, 1, EGL_NONE};
+
+    EGLSurface surface = eglCreatePbufferSurface(display, config_, attribs);


Call AndroidContextGL::CreatePbufferSurface which is identical to this code.

dnfield · 2021-09-23T20:19:38Z

testing/run_tests.py

+  remote_path = '/data/local/tmp'
+  remote_tests_path = os.path.join(remote_path, test_runner_name)
+  RunCmd(['adb', 'push', tests_path, remote_path], cwd=buildroot_dir)
+  RunCmd(['adb', 'shell', remote_tests_path])


How does this validate failure or pass?

adb shell forwards the exit code which RunCmd grabs.

dnfield · 2021-09-23T20:21:49Z

shell/platform/android/android_context_gl.cc

+  if (window->IsFakeWindow()) {
+    return CreatePbufferSurface();
+  } else {


Why do we need to create a PBuffer surface?

How confident can we be that we're testing what we even care about here?

Is there any possible way we could avoid this code in production code?

The tests in this PR are just very simple setup and teardown. This is more about setting up the infrastructure to make more interesting tests that do assert more things.

As far as the IsFakeWindow question, I looked into it. I didn't see a way to do it that wasn't a significant refactor. That's sometimes the problem when you add tests after the fact versus build them up with tests.

You could transfer creating the surface to the window, but that would leak egl calls outside of the AndroidContextGL. I'm open to any suggestion I might have overlooked.

Why do we need to create a PBuffer surface?

The code assumes a surface, it will crash if you don't have one. We don't have a window so our only option is to make a fake pbuffer surface.

dnfield

Have you considered creating an integration test in FTL that would then scan the logs for the specific error/warning that Android emits when the context is collected on the wrong thread?

dnfield · 2021-09-23T20:25:19Z

or - in addition to scanning the logs, you could add a FML_DCHECK and test with an unopt build.

gaaclarke · 2021-09-23T20:29:25Z

Have you considered creating an integration test in FTL that would then scan the logs for the specific error/warning that Android emits when the context is collected on the wrong thread?

I didn't really look into it because there is a broader need for unit tests for these files and it will be adequate for my purposes.

gaaclarke · 2021-09-24T17:07:59Z

@jason-simmons friendly ping

@dnfield One thing I forgot to mention was the need to run the unit tests in the debugger.

… devices (flutter/engine#28784)

…flutter#28784)

flutter-dashboard bot added the platform-android label Sep 21, 2021

google-cla bot added the cla: yes label Sep 21, 2021

gaaclarke force-pushed the android_unittests branch from 3431b5b to 15c9add Compare September 21, 2021 22:39

gaaclarke changed the title ~~added unit tests that run on android devices~~ added unit tests for the android embedder that run on android devices Sep 21, 2021

gaaclarke force-pushed the android_unittests branch 3 times, most recently from a882bdd to fcadf81 Compare September 21, 2021 22:59

gaaclarke marked this pull request as ready for review September 21, 2021 23:13

gaaclarke requested a review from chinmaygarde September 21, 2021 23:13

gaaclarke mentioned this pull request Sep 21, 2021

Share the GrDirectContext across engine instances on Android flutter/flutter#90398

Closed

gaaclarke force-pushed the android_unittests branch from fcadf81 to 8dc3e5f Compare September 21, 2021 23:57

zanderso requested a review from jason-simmons September 22, 2021 23:10

jason-simmons reviewed Sep 23, 2021

View reviewed changes

gaaclarke requested a review from jason-simmons September 23, 2021 18:53

dnfield reviewed Sep 23, 2021

View reviewed changes

jason-simmons approved these changes Sep 27, 2021

View reviewed changes

gaaclarke added 3 commits September 28, 2021 13:14

added unit tests that run on android devices

3f7b0d8

review feedback

c4eb82a

feedback2

34f4f86

gaaclarke force-pushed the android_unittests branch from d91fee4 to 34f4f86 Compare September 28, 2021 20:16

gaaclarke mentioned this pull request Sep 28, 2021

☂️ Add Android Embedder Unit Tests flutter/flutter#90903

Closed

gaaclarke merged commit 20ec34e into flutter:master Sep 28, 2021

engine-flutter-autoroll mentioned this pull request Sep 29, 2021

Roll Engine from bd250bdd8178 to e2775928ec64 (16 revisions) flutter/flutter#90980

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 29, 2021

20ec34e added unit tests for the android embedder that run on android…

0ab5b3f

… devices (flutter/engine#28784)

dnfield pushed a commit to dnfield/engine that referenced this pull request Oct 6, 2021

added unit tests for the android embedder that run on android devices (…

e58551f

…flutter#28784)

added unit tests for the android embedder that run on android devices #28784

added unit tests for the android embedder that run on android devices #28784

Uh oh!

Conversation

gaaclarke commented Sep 21, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pre-launch Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnfield left a comment

Choose a reason for hiding this comment

Uh oh!

dnfield commented Sep 23, 2021

Uh oh!

gaaclarke commented Sep 23, 2021

Uh oh!

gaaclarke commented Sep 24, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

gaaclarke commented Sep 21, 2021 •

edited

Loading