From 9cc132d4c20d60f5541a8f693f81d78d67d6d746 Mon Sep 17 00:00:00 2001 From: Matan Lurey Date: Fri, 24 Jan 2025 17:55:28 -0800 Subject: [PATCH] Force Impeller backend for `android_engine_test`, and test both OpenGLES and Vulkan (#162089) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A few changes in this PR (could have been split up, but that would take an extra 4 hours - 2 days in CI time): - Removed the old `flutter_driver_android_test` shards and references - Override `AndroidManifest.xml` per backend, forcing that backend to be used (no fallbacks) or it fails - Bumps the Android emulator to 35 due to the use of magic strings, I think The check for the Impeller backend actually being used is the honor system right now, as a bug in `flutter drive` (https://github.com/flutter/flutter/issues/162087) prevents me from seeing error output, meaning that it's impossible to be sure a particular Impeller backend is used (or understand why a crash happens), so I guess I'll work on that next. Due to https://github.com/flutter/flutter/issues/162088, we were accidentally running our Vulkan tests as OpenGLES 🤦🏼 . --- .ci.yaml | 30 +------ .../run_android_engine_tests.dart | 85 +++++++++++++++---- dev/bots/test.dart | 3 - .../android_engine_test/README.md | 3 +- .../android/app/src/main/AndroidManifest.xml | 6 +- 5 files changed, 75 insertions(+), 52 deletions(-) diff --git a/.ci.yaml b/.ci.yaml index 06c47342c8c..b04bc89c3ae 100644 --- a/.ci.yaml +++ b/.ci.yaml @@ -1526,7 +1526,7 @@ targets: - engine/** - DEPS - - name: Linux_android_emu_34 android_engine_vulkan_tests + - name: Linux_android_emu android_engine_vulkan_tests recipe: flutter/flutter_drone bringup: true timeout: 60 @@ -1539,7 +1539,7 @@ targets: {"dependency": "goldctl", "version": "git_revision:2387d6fff449587eecbb7e45b2692ca0710b63b9"} ] - - name: Linux_android_emu_34 android_engine_opengles_tests + - name: Linux_android_emu android_engine_opengles_tests recipe: flutter/flutter_drone bringup: true timeout: 60 @@ -1552,32 +1552,6 @@ targets: {"dependency": "goldctl", "version": "git_revision:2387d6fff449587eecbb7e45b2692ca0710b63b9"} ] - - name: Linux_android_emu flutter_driver_android_test - recipe: flutter/flutter_drone - bringup: true - timeout: 60 - properties: - shard: android_engine_tests - tags: > - ["framework", "hostonly", "shard", "linux"] - dependencies: >- - [ - {"dependency": "goldctl", "version": "git_revision:2387d6fff449587eecbb7e45b2692ca0710b63b9"} - ] - presubmit_max_attempts: "2" - - - name: Linux_android_emu_34 flutter_driver_android_test - recipe: flutter/flutter_drone - timeout: 60 - properties: - shard: android_engine_tests - tags: > - ["framework", "hostonly", "shard", "linux"] - dependencies: >- - [ - {"dependency": "goldctl", "version": "git_revision:2387d6fff449587eecbb7e45b2692ca0710b63b9"} - ] - - name: Linux web_benchmarks_canvaskit recipe: devicelab/devicelab_drone presubmit: false diff --git a/dev/bots/suite_runners/run_android_engine_tests.dart b/dev/bots/suite_runners/run_android_engine_tests.dart index 770160c90aa..6ad399b9a3e 100644 --- a/dev/bots/suite_runners/run_android_engine_tests.dart +++ b/dev/bots/suite_runners/run_android_engine_tests.dart @@ -3,6 +3,7 @@ // found in the LICENSE file. import 'package:file/file.dart'; +import 'package:file/local.dart'; import 'package:glob/glob.dart'; import 'package:glob/list_local_fs.dart'; import 'package:path/path.dart' as path; @@ -32,28 +33,78 @@ import '../utils.dart'; /// to determine flakiness in the *same* state, or want better debugging, see /// `dev/integration_tests/android_engine_test/README.md`. Future runAndroidEngineTests({required ImpellerBackend impellerBackend}) async { - print('Running Flutter Driver Android tests...'); + print('Running Flutter Driver Android tests (backend=$impellerBackend)'); final String androidEngineTestPath = path.join('dev', 'integration_tests', 'android_engine_test'); final List mains = Glob('$androidEngineTestPath/lib/**_main.dart').listSync(); - for (final FileSystemEntity file in mains) { - await runCommand( - 'flutter', - [ - 'drive', - path.relative(file.path, from: androidEngineTestPath), - // There are no reason to enable development flags for this test. - // Disable them to work around flakiness issues, and in general just - // make less things start up unnecessarily. - '--no-dds', - '--no-enable-dart-profiling', - '--test-arguments=test', - '--test-arguments=--reporter=expanded', - ], - workingDirectory: androidEngineTestPath, - environment: {'ANDROID_ENGINE_TEST_GOLDEN_VARIANT': impellerBackend.name}, + + final File androidManifestXml = const LocalFileSystem().file( + path.join(androidEngineTestPath, 'android', 'app', 'src', 'main', 'AndroidManifest.xml'), + ); + final String androidManifestContents = androidManifestXml.readAsStringSync(); + + try { + // Replace whatever the current backend is with the specified backend. + final RegExp impellerBackendMetadata = RegExp(_impellerBackendMetadata(value: '.*')); + androidManifestXml.writeAsStringSync( + androidManifestContents.replaceFirst( + impellerBackendMetadata, + _impellerBackendMetadata(value: impellerBackend.name), + ), ); + + // Stdout will produce: "Using the Impeller rendering backend (.*)" + // TODO(matanlurey): Enable once `flutter drive` retains error logs. + // final RegExp impellerStdoutPattern = RegExp('Using the Imepller rendering backend (.*)'); + + for (final FileSystemEntity file in mains) { + final CommandResult result = await runCommand( + 'flutter', + [ + 'drive', + path.relative(file.path, from: androidEngineTestPath), + // There are no reason to enable development flags for this test. + // Disable them to work around flakiness issues, and in general just + // make less things start up unnecessarily. + '--no-dds', + '--no-enable-dart-profiling', + '--test-arguments=test', + '--test-arguments=--reporter=expanded', + ], + workingDirectory: androidEngineTestPath, + environment: {'ANDROID_ENGINE_TEST_GOLDEN_VARIANT': impellerBackend.name}, + ); + final String? stdout = result.flattenedStdout; + if (stdout == null) { + foundError(['No stdout produced.']); + continue; + } + + // TODO(matanlurey): Enable once `flutter drive` retains error logs. + // https://github.com/flutter/flutter/issues/162087. + // + // final Match? stdoutMatch = impellerStdoutPattern.firstMatch(stdout); + // if (stdoutMatch == null) { + // foundError(['Could not find pattern ${impellerStdoutPattern.pattern}.', stdout]); + // continue; + // } + + // final String reportedBackend = stdoutMatch.group(1)!.toLowerCase(); + // if (reportedBackend != impellerBackend.name) { + // foundError([ + // 'Reported Imepller backend was $reportedBackend, expected ${impellerBackend.name}', + // ]); + // continue; + // } + } + } finally { + // Restore original contents. + androidManifestXml.writeAsStringSync(androidManifestContents); } } +String _impellerBackendMetadata({required String value}) { + return ''; +} + enum ImpellerBackend { vulkan, opengles } diff --git a/dev/bots/test.dart b/dev/bots/test.dart index 0845740085e..fbfe7ac8b9e 100644 --- a/dev/bots/test.dart +++ b/dev/bots/test.dart @@ -137,9 +137,6 @@ Future main(List args) async { 'web_skwasm_tests': webTestsSuite.runWebSkwasmUnitTests, // All web integration tests 'web_long_running_tests': webTestsSuite.webLongRunningTestsRunner, - // TODO(matanlurey): Remove once a post-submit runs with the new shards. - // (Part of https://github.com/flutter/flutter/issues/161333) - 'android_engine_tests': () => runAndroidEngineTests(impellerBackend: ImpellerBackend.vulkan), 'android_engine_vulkan_tests': () => runAndroidEngineTests(impellerBackend: ImpellerBackend.vulkan), 'android_engine_opengles_tests': diff --git a/dev/integration_tests/android_engine_test/README.md b/dev/integration_tests/android_engine_test/README.md index 70793738ba4..f2c8b09cd68 100644 --- a/dev/integration_tests/android_engine_test/README.md +++ b/dev/integration_tests/android_engine_test/README.md @@ -19,7 +19,8 @@ See [`dev/bots/suite_runners/run_android_engine_tests.dart`](../../bots/suite_ru ```sh # TIP: If golden-files do not exist locally, this command will fail locally. -SHARD=android_engine_tests bin/cache/dart-sdk/bin/dart dev/bots/test.dart +SHARD=android_engine_vulkan_tests bin/cache/dart-sdk/bin/dart dev/bots/test.dart +SHARD=android_engine_opengles_tests bin/cache/dart-sdk/bin/dart dev/bots/test.dart ``` ## Running the apps and tests diff --git a/dev/integration_tests/android_engine_test/android/app/src/main/AndroidManifest.xml b/dev/integration_tests/android_engine_test/android/app/src/main/AndroidManifest.xml index 9439ec6c91f..882488091fe 100644 --- a/dev/integration_tests/android_engine_test/android/app/src/main/AndroidManifest.xml +++ b/dev/integration_tests/android_engine_test/android/app/src/main/AndroidManifest.xml @@ -31,9 +31,9 @@ found in the LICENSE file. --> - + + +