Refactor Android scenario_app to remove shell entrypoint, simplify. (…

…#50922) Closes flutter/flutter#144045. There is still more work I want to do, like pulling args parsing into it's own class, and potentially cleanup the [golden file collection](flutter/flutter#144047), but those seem reasonable for future (lower priority) PRs.
flutter · Feb 23, 2024 · b6ffb98 · b6ffb98
1 parent 41c3372
commit b6ffb98
Show file tree

Hide file tree

Showing 10 changed files with 113 additions and 178 deletions.
diff --git a/ci/builders/linux_android_emulator.json b/ci/builders/linux_android_emulator.json
@@ -59,7 +59,7 @@
                     ]
                 },
                 {
-                    "language": "bash",
+                    "language": "dart",
                     "name": "Android Scenario App Integration Tests (Skia)",
                     "test_dependencies": [
                         {
@@ -74,14 +74,14 @@
                     "contexts": [
                         "android_virtual_device"
                     ],
-                    "script": "flutter/testing/scenario_app/run_android_tests.sh",
+                    "script": "flutter/testing/scenario_app/bin/run_android_tests.dart",
                     "parameters": [
-                        "android_emulator_debug_x64",
+                        "--out-dir=../out/android_emulator_debug_x64",
                         "--no-enable-impeller"
                     ]
                 },
                 {
-                    "language": "bash",
+                    "language": "dart",
                     "name": "Android Scenario App Integration Tests (Impeller/Vulkan)",
                     "test_dependencies": [
                         {
@@ -96,9 +96,9 @@
                     "contexts": [
                         "android_virtual_device"
                     ],
-                    "script": "flutter/testing/scenario_app/run_android_tests.sh",
+                    "script": "flutter/testing/scenario_app/bin/run_android_tests.dart",
                     "parameters": [
-                        "android_emulator_debug_x64",
+                        "--out-dir=../out/android_emulator_debug_x64",
                         "--enable-impeller",
                         "--impeller-backend=vulkan"
                     ]

diff --git a/ci/builders/linux_android_emulator_api_33.json b/ci/builders/linux_android_emulator_api_33.json
@@ -59,7 +59,7 @@
                     ]
                 },
                 {
-                    "language": "bash",
+                    "language": "dart",
                     "name": "Scenario App Integration Tests",
                     "test_dependencies": [
                         {
@@ -74,9 +74,9 @@
                     "contexts": [
                         "android_virtual_device"
                     ],
-                    "script": "flutter/testing/scenario_app/run_android_tests.sh",
+                    "script": "flutter/testing/scenario_app/bin/run_android_tests.dart",
                     "parameters": [
-                        "android_debug_api33_x64"
+                        "--out-dir=../out/android_debug_api33_x64"
                     ]
                 }
             ]

diff --git a/ci/builders/linux_android_emulator_opengles.json b/ci/builders/linux_android_emulator_opengles.json
@@ -34,7 +34,7 @@
             },
             "tests": [
                 {
-                    "language": "bash",
+                    "language": "dart",
                     "name": "Android Scenario App Integration Tests (Impeller/OpenGLES)",
                     "test_dependencies": [
                         {
@@ -49,9 +49,9 @@
                     "contexts": [
                         "android_virtual_device"
                     ],
-                    "script": "flutter/testing/scenario_app/run_android_tests.sh",
+                    "script": "flutter/testing/scenario_app/bin/run_android_tests.dart",
                     "parameters": [
-                        "android_emulator_opengles_debug_x64",
+                        "--out-dir=../out/android_emulator_opengles_debug_x64",
                         "--enable-impeller",
                         "--impeller-backend=opengles"
                     ]

diff --git a/testing/scenario_app/README.md b/testing/scenario_app/README.md
@@ -6,17 +6,20 @@ This package simulates a Flutter app that uses the engine (`dart:ui`) only,
 in conjunction with Android and iOS-specific embedding code that simulates the
 use of the engine in a real app (such as plugins and platform views).
 
-The [`run_android_tests.sh`](run_android_tests.sh) and
+The [`bin/run_android_tests.dart`](bin/run_android_tests.dart) and
 [`run_ios_tests.sh`](run_ios_tests.sh) are then used to run the tests on a
 connected device or emulator.
 
 See also:
 
+- [File an issue][file_issue] with the `e: scenario-app` label.
 - [`bin/`](bin/), the entry point for running Android integration tests.
 - [`lib/`](lib/), the Dart code and instrumentation for the scenario app.
 - [`ios/`](ios/), the iOS-side native code and tests.
 - [`android/`](android/), the Android-side native code and tests.
 
+[file_issue]: https://github.com/flutter/flutter/issues/new?labels=e:%20scenario-app,engine,platform-android,fyi-android,team-engine
+
 ## Running a smoke test on Firebase TestLab
 
 To run the smoke test on Firebase TestLab test, build `android_profile_arm64`,
@@ -41,10 +44,3 @@ viewport.
 
 Then set the scenario from the Android or iOS app by calling `set_scenario` on
 platform channel `driver`.
-
-## Output validation
-
-When using `//flutter/testing/scenario_app/run_android_tests.sh` the generated
-output will be checked against a golden file at
-`//flutter/testing/scenario_app_android_output.txt` to make sure all output was
-generated. A patch will be printed to stdout if they don't match.
diff --git a/testing/scenario_app/android/README.md b/testing/scenario_app/android/README.md
@@ -5,15 +5,16 @@ the Android-specific native code and tests for the [scenario app](../lib). To
 run the tests, you will need to build the engine with the appropriate
 configuration.
 
-For example, `android_debug_unopt` or `android_debug_unopt_arm64` was built,
-run:
+For example, for the latest `android` build you've made locally:
 
 ```sh
-# From the root of the engine repository
-$ ./testing/scenario_app/run_android_tests.sh android_debug_unopt
+dart ./testing/scenario_app/bin/run_android_tests.dart
+```
+
+Or for a specific, build, such as `android_debug_unopt_arm64`:
 
-# Or, for arm64
-$ ./testing/scenario_app/run_android_tests.sh android_debug_unopt_arm64
+```sh
+dart ./testing/scenario_app/bin/run_android_tests.dart --out-dir=../out/android_debug_unopt_arm64
 ```
 
 ## Debugging
@@ -28,8 +29,7 @@ Locally (or on a temporary PR for CI), you can run the tests with the
 to verify the setup:
 
 ```sh
-# From the root of the engine repository
-$ ./testing/scenario_app/run_android_tests.sh android_debug_unopt_arm64 --smoke-test dev.flutter.scenarios.EngineLaunchE2ETest
+dart ./testing/scenario_app/bin/run_android_tests.dart --smoke-test dev.flutter.scenarios.EngineLaunchE2ETest
 ```
 
 The result of `adb logcat` and screenshots taken during the test will be stored
@@ -43,7 +43,7 @@ You can then view the logs and screenshots on LUCI. [For example](https://ci.chr
 ## CI Configuration
 
 See [`ci/builders/linux_android_emulator.json`](../../../ci/builders/linux_android_emulator.json)
-, and grep for `run_android_tests.sh`.
+, and grep for `run_android_tests.dart`.
 
 The following matrix of configurations is tested on the CI:
 
@@ -67,3 +67,9 @@ The following matrix of configurations is tested on the CI:
 ## Updating Gradle dependencies
 
 See [Updating the Embedding Dependencies](../../../tools/cipd/android_embedding_bundle/README.md).
+
+## Output validation
+
+The generated output will be checked against a golden file
+([`expected_golden_output.txt`](./expected_golden_output.txt)) to make sure all
+output was generated. A patch will be printed to stdout if they don't match.
diff --git a/testing/scenario_app_android_output.txt → ...io_app/android/expected_golden_output.txt b/testing/scenario_app_android_output.txt → ...io_app/android/expected_golden_output.txt
diff --git a/...io_app/bin/android_integration_tests.dart → ...g/scenario_app/bin/run_android_tests.dart b/...io_app/bin/android_integration_tests.dart → ...g/scenario_app/bin/run_android_tests.dart
@@ -46,7 +46,7 @@ void main(List<String> args) async {
     )
     ..addOption(
       'adb',
-      help: 'Absolute path to the adb tool',
+      help: 'Path to the adb tool',
       defaultsTo: engine != null ? join(
         engine.srcDir.path,
         'third_party',
@@ -56,6 +56,30 @@ void main(List<String> args) async {
         'adb',
       ) : null,
     )
+    ..addOption(
+      'ndk-stack',
+      help: 'Path to the ndk-stack tool',
+      defaultsTo: engine != null ? join(
+        engine.srcDir.path,
+        'third_party',
+        'android_tools',
+        'ndk',
+        'prebuilt',
+        () {
+          if (Platform.isLinux) {
+            return 'linux-x86_64';
+          } else if (Platform.isMacOS) {
+            return 'darwin-x86_64';
+          } else if (Platform.isWindows) {
+            return 'windows-x86_64';
+          } else {
+            throw UnsupportedError('Unsupported platform: ${Platform.operatingSystem}');
+          }
+        }(),
+        'bin',
+        'ndk-stack',
+      ) : null,
+    )
     ..addOption(
       'out-dir',
       help: 'Out directory',
@@ -68,7 +92,7 @@ void main(List<String> args) async {
     )
     ..addOption(
       'smoke-test',
-      help: 'runs a single test to verify the setup',
+      help: 'Runs a single test to verify the setup',
     )
     ..addFlag(
       'use-skia-gold',
@@ -79,8 +103,17 @@ void main(List<String> args) async {
       'enable-impeller',
       help: 'Enable Impeller for the Android app.',
     )
-    ..addOption('output-contents-golden',
-      help: 'Path to a file that will be used to check the contents of the output to make sure everything was created.',
+    ..addOption(
+      'output-contents-golden',
+      help: 'Path to a file that contains the expected filenames of golden files.',
+      defaultsTo: engine != null ? join(
+        engine.srcDir.path,
+        'flutter',
+        'testing',
+        'scenario_app',
+        'android',
+        'expected_golden_output.txt',
+      ) : null,
     )
     ..addOption(
       'impeller-backend',
@@ -123,6 +156,10 @@ void main(List<String> args) async {
         panic(<String>['invalid graphics-backend', results['impeller-backend'] as String? ?? '<null>']);
       }
       final Directory logsDir = Directory(results['logs-dir'] as String? ?? join(outDir.path, 'scenario_app', 'logs'));
+      final String? ndkStack = results['ndk-stack'] as String?;
+      if (ndkStack == null) {
+        panic(<String>['--ndk-stack is required']);
+      }
       await _run(
         verbose: verbose,
         outDir: outDir,
@@ -133,6 +170,7 @@ void main(List<String> args) async {
         impellerBackend: impellerBackend,
         logsDir: logsDir,
         contentsGolden: contentsGolden,
+        ndkStack: ndkStack,
       );
       exit(0);
     },
@@ -172,6 +210,7 @@ Future<void> _run({
   required _ImpellerBackend? impellerBackend,
   required Directory logsDir,
   required String? contentsGolden,
+  required String ndkStack,
 }) async {
   const ProcessManager pm = LocalProcessManager();
 
@@ -187,6 +226,9 @@ Future<void> _run({
   final String logcatPath = join(logsDir.path, 'logcat.txt');
 
   // TODO(matanlurey): Use screenshots/ sub-directory (https://github.com/flutter/flutter/issues/143604).
+  if (!logsDir.existsSync()) {
+    logsDir.createSync(recursive: true);
+  }
   final String screenshotPath = logsDir.path;
   final String apkOutPath = join(scenarioAppPath, 'app', 'outputs', 'apk');
   final File testApk = File(join(apkOutPath, 'androidTest', 'debug', 'app-debug-androidTest.apk'));
@@ -279,6 +321,10 @@ Future<void> _run({
           case null:
             break;
           case 'ActivityManager':
+            // These are mostly noise, i.e. "D ActivityManager: freezing 24632 com.blah".
+            if (adbLogLine!.severity == 'D') {
+              break;
+            }
           // TODO(matanlurey): Figure out why this isn't 'flutter.scenario' or similar.
           // Also, why is there two different names?
           case 'utter.scenario':
@@ -388,6 +434,33 @@ Future<void> _run({
   } finally {
     await server.close();
 
+    await step('Killing logcat process...', () async {
+      final bool delivered = logcatProcess.kill(ProcessSignal.sigkill);
+      assert(delivered);
+      await logcatProcessExitCode;
+    });
+
+    await step('Flush logcat...', () async {
+      await logcat.flush();
+      await logcat.close();
+      log('wrote logcat to $logcatPath');
+    });
+
+    await step('Symbolize stack traces', () async {
+      final ProcessResult result = await pm.run(
+        <String>[
+          ndkStack,
+          '-sym',
+          outDir.path,
+          '-dump',
+          logcatPath,
+        ],
+      );
+      if (result.exitCode != 0) {
+        panic(<String>['Failed to symbolize stack traces']);
+      }
+    });
+
     await step('Remove reverse port...', () async {
       final int exitCode = await pm.runAndForward(<String>[
         adb.path,
@@ -413,19 +486,15 @@ Future<void> _run({
       }
     });
 
-    await step('Killing logcat process...', () async {
-      final bool delivered = logcatProcess.kill(ProcessSignal.sigkill);
-      assert(delivered);
-      await logcatProcessExitCode;
-    });
-
     await step('Wait for Skia gold comparisons...', () async {
       await Future.wait(pendingComparisons);
     });
 
     if (contentsGolden != null) {
       // Check the output here.
       await step('Check output files...', () async {
+        // TODO(matanlurey): Resolve this in a better way. On CI this file always exists.
+        File(join(screenshotPath, 'noop.txt')).writeAsStringSync('');
         // TODO(gaaclarke): We should move this into dir_contents_diff.
         _withTemporaryCwd(contentsGolden, () {
           final int exitCode = dirContentsDiff(basename(contentsGolden), screenshotPath);
@@ -435,11 +504,5 @@ Future<void> _run({
         });
       });
     }
-
-    await step('Flush logcat...', () async {
-      await logcat.flush();
-      await logcat.close();
-      log('wrote logcat to $logcatPath');
-    });
   }
 }
diff --git a/testing/scenario_app/bin/utils/adb_logcat_filtering.dart b/testing/scenario_app/bin/utils/adb_logcat_filtering.dart
@@ -60,6 +60,9 @@ extension type const AdbLogLine._(Match _match) {
   /// The full line of `adb logcat` output.
   String get line => _match.group(0)!;
 
+  /// The character representing the severity of the log message, such as `I`.
+  String get severity => _match.group(2)!;
+
   /// The process name, such as `ActivityManager`.
   String get process => _match.group(3)!;