Improve, test, and fix a bug related to adb logcat filtering. (#51012)

1. Write tests for the `AdbLogLine` extension/parsing, and run it on CI.
  Fixes flutter/flutter#144164.

2. Improve the logging to include `androidemu`-related errors logs 
  Work towards flutter/flutter#144164.

3. Clarified logic/fixed a bug related to handling `filterProcessId:
...`

Author: matanlurey

testing/run_tests.py

@@ -909,6 +909,7 @@ def build_dart_host_test_list(build_dir):
       ),
       (os.path.join('flutter', 'testing', 'litetest'), []),
       (os.path.join('flutter', 'testing', 'skia_gold_client'), []),
+      (os.path.join('flutter', 'testing', 'scenario_app'), []),
       (
           os.path.join('flutter', 'tools', 'api_check'),
           [os.path.join(BUILDROOT_DIR, 'flutter')],

testing/scenario_app/README.md

@@ -18,7 +18,7 @@ See also:
 - [`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
+[file_issue]: https://github.com/flutter/flutter/issues/new?labels=e:%20scenario-app,engine,team-engine
 
 ## Running a smoke test on Firebase TestLab
 

testing/scenario_app/bin/README.md

@@ -5,6 +5,13 @@ This directory contains code specific to running Android integration tests.
 The tests are uploaded and run on the device using `adb`, and screenshots are
 captured and compared using Skia Gold (if available, for example on CI).
 
+See also:
+
+- [File an issue][file_issue] with the `e: scenario-app, platform-android`
+  labels.
+
+[file_issue]: https://github.com/flutter/flutter/issues/new?labels=e:%20scenario-app,engine,platform-android,fyi-android,team-engine
+
 ## Usage
 
 ```sh

testing/scenario_app/bin/utils/adb_logcat_filtering.dart

@@ -30,6 +30,8 @@
 /// See also: <https://developer.android.com/tools/logcat>.
 library;
 
+import 'package:meta/meta.dart';
+
 import 'logs.dart';
 
 /// Represents a line of `adb logcat` output parsed into a structured form.
@@ -74,6 +76,34 @@ extension type const AdbLogLine._(Match _match) {
     return null;
   }
 
+  @visibleForTesting
+  static const Set<String> knownNoiseTags = <String>{
+    'CCodec',
+    'CCodecBufferChannel',
+    'CCodecConfig',
+    'Codec2Client',
+    'ColorUtils',
+    'DMABUFHEAPS',
+    'Gralloc4',
+    'MediaCodec',
+    'MonitoringInstr',
+    'ResourceExtractor',
+    'UsageTrackerFacilitator',
+    'hw-BpHwBinder',
+    'ziparchive',
+  };
+
+  @visibleForTesting
+  static const Set<String> knownUsefulTags = <String>{
+    'ActivityManager',
+  };
+
+  @visibleForTesting
+  static const Set<String> knownUsefulErrorTags = <String>{
+    'androidemu',
+    'THREAD_STATE',
+  };
+
   /// Returns `true` if the log line is verbose.
   bool isVerbose({String? filterProcessId}) => !_isRelevant(filterProcessId: filterProcessId);
   bool _isRelevant({String? filterProcessId}) {
@@ -82,37 +112,31 @@ extension type const AdbLogLine._(Match _match) {
       return true;
     }
 
-    // Debug logs are rarely useful.
-    if (severity == 'D') {
+    // Verbose and debug logs are rarely useful.
+    if (severity == 'V' || severity == 'D') {
       return false;
     }
 
-    // These are "known" noise tags.
-    if (const <String>{
-      'MonitoringInstr',
-      'ResourceExtractor',
-      'THREAD_STATE',
-      'ziparchive',
-    }.contains(name)) {
+    if (knownNoiseTags.contains(name)) {
       return false;
     }
 
-    // These are "known" tags useful for debugging.
-    if (const <String>{
-      'utter.scenario',
-      'utter.scenarios',
-      'TestRunner',
-    }.contains(name)) {
+    if (knownUsefulTags.contains(name)) {
+      return true;
+    }
+
+    if (severity == 'E' && knownUsefulErrorTags.contains(name)) {
       return true;
     }
 
     // If a process ID is specified, exclude logs _not_ from that process.
-    if (filterProcessId != null && process != filterProcessId) {
-      return false;
+    if (filterProcessId == null) {
+      // YOLO, let's keep it anyway.
+      return name.toLowerCase().contains('flutter') ||
+          message.toLowerCase().contains('flutter');
     }
 
-    // And... whatever, include anything with the word "flutter".
-    return name.toLowerCase().contains('flutter') || message.toLowerCase().contains('flutter');
+    return process == filterProcessId;
   }
 
   /// Logs the line to the console.

testing/scenario_app/pubspec.yaml

@@ -25,9 +25,14 @@ dependencies:
   vector_math: any
   skia_gold_client: any
 
+dev_dependencies:
+  litetest: any
+
 dependency_overrides:
   args:
     path: ../../../third_party/dart/third_party/pkg/args
+  async_helper:
+    path: ../../../third_party/dart/pkg/async_helper
   collection:
     path: ../../../third_party/dart/third_party/pkg/collection
   crypto:
@@ -36,8 +41,12 @@ dependency_overrides:
     path: ../../tools/dir_contents_diff
   engine_repo_tools:
     path: ../../tools/pkg/engine_repo_tools
+  expect:
+    path: ../../../third_party/dart/pkg/expect
   file:
     path: ../../../third_party/dart/third_party/pkg/file/packages/file
+  litetest:
+    path: ../litetest
   meta:
     path: ../../../third_party/dart/pkg/meta
   path:
@@ -48,6 +57,8 @@ dependency_overrides:
     path: ../../third_party/pkg/process
   skia_gold_client:
     path: ../../testing/skia_gold_client
+  smith:
+    path: ../../../third_party/dart/pkg/smith
   sky_engine:
     path: ../../sky/packages/sky_engine
   typed_data:

testing/scenario_app/test/adb_log_filter_test.dart

@@ -0,0 +1,106 @@
+import 'package:litetest/litetest.dart';
+
+import '../bin/utils/adb_logcat_filtering.dart';
+import 'src/fake_adb_logcat.dart';
+
+void main() {
+  /// Simulates the filtering of logcat output [lines].
+  Iterable<String> filter(Iterable<String> lines, {int? filterProcessId}) {
+    if (lines.isEmpty) {
+      throw StateError('No log lines to filter. This is unexpected.');
+    }
+    return lines.where((String line) {
+      final AdbLogLine? logLine = AdbLogLine.tryParse(line);
+      if (logLine == null) {
+        throw StateError('Invalid log line: $line');
+      }
+      final bool isVerbose = logLine.isVerbose(filterProcessId: filterProcessId?.toString());
+      return !isVerbose;
+    });
+  }
+
+  test('should always retain fatal logs', () {
+    final FakeAdbLogcat logcat = FakeAdbLogcat();
+    final FakeAdbProcess process = logcat.withProcess();
+    process.fatal('Something', 'A bad thing happened');
+
+    final Iterable<String> filtered = filter(logcat.drain());
+    expect(filtered, hasLength(1));
+    expect(filtered.first, contains('Something: A bad thing happened'));
+  });
+
+  test('should never retain debug logs', () {
+    final FakeAdbLogcat logcat = FakeAdbLogcat();
+    final FakeAdbProcess process = logcat.withProcess();
+    final String tag = AdbLogLine.knownNoiseTags.first;
+    process.debug(tag, 'A debug message');
+
+    final Iterable<String> filtered = filter(logcat.drain());
+    expect(filtered, isEmpty);
+  });
+
+  test('should never retain logs from known "noise" tags', () {
+    final FakeAdbLogcat logcat = FakeAdbLogcat();
+    final FakeAdbProcess process = logcat.withProcess();
+    final String tag = AdbLogLine.knownNoiseTags.first;
+    process.info(tag, 'Flutter flutter flutter');
+
+    final Iterable<String> filtered = filter(logcat.drain());
+    expect(filtered, isEmpty);
+  });
+
+  test('should always retain logs from known "useful" tags', () {
+    final FakeAdbLogcat logcat = FakeAdbLogcat();
+    final FakeAdbProcess process = logcat.withProcess();
+    final String tag = AdbLogLine.knownUsefulTags.first;
+    process.info(tag, 'A useful message');
+
+    final Iterable<String> filtered = filter(logcat.drain());
+    expect(filtered, hasLength(1));
+    expect(filtered.first, contains('$tag: A useful message'));
+  });
+
+  test('if a process ID is passed, retain the log', () {
+    final FakeAdbLogcat logcat = FakeAdbLogcat();
+    final FakeAdbProcess process = logcat.withProcess();
+    process.info('SomeTag', 'A message');
+
+    final Iterable<String> filtered = filter(logcat.drain(), filterProcessId: process.processId);
+    expect(filtered, hasLength(1));
+    expect(filtered.first, contains('SomeTag: A message'));
+  });
+
+  test('even if a process ID passed, retain logs containing "flutter"', () {
+    final FakeAdbLogcat logcat = FakeAdbLogcat();
+    final FakeAdbProcess process = logcat.withProcess();
+    process.info('SomeTag', 'A message with flutter');
+
+    final Iterable<String> filtered = filter(logcat.drain(), filterProcessId: process.processId);
+    expect(filtered, hasLength(1));
+    expect(filtered.first, contains('SomeTag: A message with flutter'));
+  });
+
+  test('should retain E-level flags from known "useful" error tags', () {
+    final FakeAdbLogcat logcat = FakeAdbLogcat();
+    final FakeAdbProcess process = logcat.withProcess();
+    final String tag = AdbLogLine.knownUsefulErrorTags.first;
+    process.error(tag, 'An error message');
+    process.info(tag, 'An info message');
+
+    final Iterable<String> filtered = filter(logcat.drain());
+    expect(filtered, hasLength(1));
+    expect(filtered.first, contains('$tag: An error message'));
+  });
+
+  test('should filter out error logs from unimportant processes', () {
+    final FakeAdbLogcat logcat = FakeAdbLogcat();
+    final FakeAdbProcess process = logcat.withProcess();
+
+    // I hate this one.
+    const String tag = 'gs.intelligence';
+    process.error(tag, 'No package ID ff found for resource ID 0xffffffff.');
+
+    final Iterable<String> filtered = filter(logcat.drain());
+    expect(filtered, isEmpty);
+  });
+}