Factor out use of "print" in flutter_goldens (#144846)

Hixie · web-flow · commit c7123c96cd04 · 2024-03-17T01:56:19.000Z
This is part 5 of a broken down version of the #140101 refactor.

This PR removes direct use of `print` and replaces it with a callback.
diff --git a/dev/automated_tests/flutter_test/flutter_gold_expectation.txt b/dev/automated_tests/flutter_test/flutter_gold_expectation.txt
@@ -1,7 +1 @@
-[0-9]+:[0-9]+ [+]0: Local passes non-existent baseline for new test, null expectation *
- *No expectations provided by Skia Gold for test: library.flutter.new_golden_test.1.png. This may be a new test. If this is an unexpected result, check https://flutter-gold.skia.org.
- *Validate image output found at flutter/test/library/
-[0-9]+:[0-9]+ [+]1: Local passes non-existent baseline for new test, empty expectation *
- *No expectations provided by Skia Gold for test: library.flutter.new_golden_test.2.png. This may be a new test. If this is an unexpected result, check https://flutter-gold.skia.org.
- *Validate image output found at flutter/test/library/
 [0-9]+:[0-9]+ [+]2: All tests passed! *
diff --git a/dev/automated_tests/flutter_test/flutter_gold_test.dart b/dev/automated_tests/flutter_test/flutter_gold_test.dart
@@ -21,6 +21,7 @@ const List<int> _kFailPngBytes = <int>[
 ];
 
 void main() {
+  final List<String> log = <String>[];
   final MemoryFileSystem fs = MemoryFileSystem();
   final Directory basedir = fs.directory('flutter/test/library/')
     ..createSync(recursive: true);
@@ -34,26 +35,39 @@ void main() {
       environment: <String, String>{'FLUTTER_ROOT': '/flutter'},
       operatingSystem: 'macos'
     ),
+    log: log.add,
   );
 
   test('Local passes non-existent baseline for new test, null expectation', () async {
+    log.clear();
     expect(
       await comparator.compare(
         Uint8List.fromList(_kFailPngBytes),
         Uri.parse('flutter.new_golden_test.1.png'),
       ),
       isTrue,
     );
+    const String expectation =
+      'No expectations provided by Skia Gold for test: library.flutter.new_golden_test.1.png. '
+      'This may be a new test. If this is an unexpected result, check https://flutter-gold.skia.org.\n'
+      'Validate image output found at flutter/test/library/';
+    expect(log, const <String>[expectation]);
   });
 
   test('Local passes non-existent baseline for new test, empty expectation', () async {
+    log.clear();
     expect(
       await comparator.compare(
         Uint8List.fromList(_kFailPngBytes),
         Uri.parse('flutter.new_golden_test.2.png'),
       ),
       isTrue,
     );
+    const String expectation =
+      'No expectations provided by Skia Gold for test: library.flutter.new_golden_test.2.png. '
+      'This may be a new test. If this is an unexpected result, check https://flutter-gold.skia.org.\n'
+      'Validate image output found at flutter/test/library/';
+    expect(log, const <String>[expectation]);
   });
 }
 
diff --git a/packages/flutter_goldens/lib/flutter_goldens.dart b/packages/flutter_goldens/lib/flutter_goldens.dart
@@ -39,16 +39,17 @@ bool _isMainBranch(String? branch) {
 Future<void> testExecutable(FutureOr<void> Function() testMain, {String? namePrefix}) async {
   const Platform platform = LocalPlatform();
   if (FlutterPostSubmitFileComparator.isForEnvironment(platform)) {
-    goldenFileComparator = await FlutterPostSubmitFileComparator.fromDefaultComparator(platform, namePrefix: namePrefix);
+    goldenFileComparator = await FlutterPostSubmitFileComparator.fromDefaultComparator(platform, namePrefix: namePrefix, log: print);
   } else if (FlutterPreSubmitFileComparator.isForEnvironment(platform)) {
-    goldenFileComparator = await FlutterPreSubmitFileComparator.fromDefaultComparator(platform, namePrefix: namePrefix);
+    goldenFileComparator = await FlutterPreSubmitFileComparator.fromDefaultComparator(platform, namePrefix: namePrefix, log: print);
   } else if (FlutterSkippingFileComparator.isForEnvironment(platform)) {
     goldenFileComparator = FlutterSkippingFileComparator.fromDefaultComparator(
       'Golden file testing is not executed on Cirrus, or LUCI environments outside of flutter/flutter.',
-        namePrefix: namePrefix
+      namePrefix: namePrefix,
+      log: print
     );
   } else {
-    goldenFileComparator = await FlutterLocalFileComparator.fromDefaultComparator(platform);
+    goldenFileComparator = await FlutterLocalFileComparator.fromDefaultComparator(platform, log: print);
   }
   await testMain();
 }
@@ -103,6 +104,7 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator {
     this.fs = const LocalFileSystem(),
     this.platform = const LocalPlatform(),
     this.namePrefix,
+    required this.log,
   });
 
   /// The directory to which golden file URIs will be resolved in [compare] and
@@ -124,6 +126,9 @@ abstract class FlutterGoldenFileComparator extends GoldenFileComparator {
   /// The prefix that is added to all golden names.
   final String? namePrefix;
 
+  /// The logging function to use when reporting messages to the console.
+  final LogCallback log;
+
   @override
   Future<void> update(Uri golden, Uint8List imageBytes) async {
     final File goldenFile = getGoldenFile(golden);
@@ -225,6 +230,7 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
     super.fs,
     super.platform,
     super.namePrefix,
+    required super.log,
   });
 
   /// Creates a new [FlutterPostSubmitFileComparator] that mirrors the relative
@@ -237,6 +243,7 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
     SkiaGoldClient? goldens,
     LocalFileComparator? defaultComparator,
     String? namePrefix,
+    required LogCallback log,
   }) async {
 
     defaultComparator ??= goldenFileComparator as LocalFileComparator;
@@ -247,9 +254,9 @@ class FlutterPostSubmitFileComparator extends FlutterGoldenFileComparator {
     );
     baseDirectory.createSync(recursive: true);
 
-    goldens ??= SkiaGoldClient(baseDirectory);
+    goldens ??= SkiaGoldClient(baseDirectory, log: log);
     await goldens.auth();
-    return FlutterPostSubmitFileComparator(baseDirectory.uri, goldens, namePrefix: namePrefix);
+    return FlutterPostSubmitFileComparator(baseDirectory.uri, goldens, namePrefix: namePrefix, log: log);
   }
 
   @override
@@ -302,6 +309,7 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
     super.fs,
     super.platform,
     super.namePrefix,
+    required super.log,
   });
 
   /// Creates a new [FlutterPreSubmitFileComparator] that mirrors the
@@ -315,6 +323,7 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
     LocalFileComparator? defaultComparator,
     Directory? testBasedir,
     String? namePrefix,
+    required LogCallback log,
   }) async {
 
     defaultComparator ??= goldenFileComparator as LocalFileComparator;
@@ -328,13 +337,14 @@ class FlutterPreSubmitFileComparator extends FlutterGoldenFileComparator {
       baseDirectory.createSync(recursive: true);
     }
 
-    goldens ??= SkiaGoldClient(baseDirectory);
+    goldens ??= SkiaGoldClient(baseDirectory, log: log);
 
     await goldens.auth();
     return FlutterPreSubmitFileComparator(
       baseDirectory.uri,
       goldens, platform: platform,
       namePrefix: namePrefix,
+      log: log,
     );
   }
 
@@ -388,6 +398,7 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
     super.skiaClient,
     this.reason, {
     super.namePrefix,
+    required super.log,
   });
 
   /// Describes the reason for using the [FlutterSkippingFileComparator].
@@ -399,21 +410,18 @@ class FlutterSkippingFileComparator extends FlutterGoldenFileComparator {
     String reason, {
     LocalFileComparator? defaultComparator,
     String? namePrefix,
+    required LogCallback log,
   }) {
     defaultComparator ??= goldenFileComparator as LocalFileComparator;
     const FileSystem fs = LocalFileSystem();
     final Uri basedir = defaultComparator.basedir;
-    final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir));
-    return FlutterSkippingFileComparator(basedir, skiaClient, reason, namePrefix: namePrefix);
+    final SkiaGoldClient skiaClient = SkiaGoldClient(fs.directory(basedir), log: log);
+    return FlutterSkippingFileComparator(basedir, skiaClient, reason, namePrefix: namePrefix, log: log);
   }
 
   @override
   Future<bool> compare(Uint8List imageBytes, Uri golden) async {
-    // Ideally we would use markTestSkipped here but in some situations,
-    // comparators are called outside of tests.
-    // See also: https://github.com/flutter/flutter/issues/91285
-    // ignore: avoid_print
-    print('Skipping "$golden" test: $reason');
+    log('Skipping "$golden" test: $reason');
     return true;
   }
 
@@ -471,6 +479,7 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
     super.skiaClient, {
     super.fs,
     super.platform,
+    required super.log,
   });
 
   /// Creates a new [FlutterLocalFileComparator] that mirrors the
@@ -483,6 +492,7 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
     SkiaGoldClient? goldens,
     LocalFileComparator? defaultComparator,
     Directory? baseDirectory,
+    required LogCallback log,
   }) async {
     defaultComparator ??= goldenFileComparator as LocalFileComparator;
     baseDirectory ??= FlutterGoldenFileComparator.getBaseDirectory(
@@ -494,7 +504,7 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
       baseDirectory.createSync(recursive: true);
     }
 
-    goldens ??= SkiaGoldClient(baseDirectory);
+    goldens ??= SkiaGoldClient(baseDirectory, log: log);
     try {
       // Check if we can reach Gold.
       await goldens.getExpectationForTest('');
@@ -503,25 +513,28 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
         baseDirectory.uri,
         goldens,
         'OSError occurred, could not reach Gold. '
-          'Switching to FlutterSkippingGoldenFileComparator.',
+        'Switching to FlutterSkippingGoldenFileComparator.',
+        log: log,
       );
     } on io.SocketException catch (_) {
       return FlutterSkippingFileComparator(
         baseDirectory.uri,
         goldens,
         'SocketException occurred, could not reach Gold. '
-          'Switching to FlutterSkippingGoldenFileComparator.',
+        'Switching to FlutterSkippingGoldenFileComparator.',
+        log: log,
       );
     } on FormatException catch (_) {
       return FlutterSkippingFileComparator(
         baseDirectory.uri,
         goldens,
         'FormatException occurred, could not reach Gold. '
-          'Switching to FlutterSkippingGoldenFileComparator.',
+        'Switching to FlutterSkippingGoldenFileComparator.',
+        log: log,
       );
     }
 
-    return FlutterLocalFileComparator(baseDirectory.uri, goldens);
+    return FlutterLocalFileComparator(baseDirectory.uri, goldens, log: log);
   }
 
   @override
@@ -532,12 +545,7 @@ class FlutterLocalFileComparator extends FlutterGoldenFileComparator with LocalC
     testExpectation = await skiaClient.getExpectationForTest(testName);
 
     if (testExpectation == null || testExpectation.isEmpty) {
-      // There is no baseline for this test.
-      // Ideally we would use markTestSkipped here but in some situations,
-      // comparators are called outside of tests.
-      // See also: https://github.com/flutter/flutter/issues/91285
-      // ignore: avoid_print
-      print(
+      log(
         'No expectations provided by Skia Gold for test: $golden. '
         'This may be a new test. If this is an unexpected result, check '
         'https://flutter-gold.skia.org.\n'
diff --git a/packages/flutter_goldens/lib/skia_client.dart b/packages/flutter_goldens/lib/skia_client.dart
@@ -23,6 +23,9 @@ const String _kTestBrowserKey = 'FLUTTER_TEST_BROWSER';
 const String _kWebRendererKey = 'FLUTTER_WEB_RENDERER';
 const String _kImpellerKey = 'FLUTTER_TEST_IMPELLER';
 
+/// Signature of callbacks used to inject [print] replacements.
+typedef LogCallback = void Function(String);
+
 /// Exception thrown when an error is returned from the [SkiaClient].
 class SkiaException implements Exception {
   /// Creates a new `SkiaException` with a required error [message].
@@ -52,6 +55,7 @@ class SkiaGoldClient {
     this.platform = const LocalPlatform(),
     Abi? abi,
     io.HttpClient? httpClient,
+    required this.log,
   }) : httpClient = httpClient ?? io.HttpClient(),
        abi = abi ?? Abi.current();
 
@@ -90,6 +94,9 @@ class SkiaGoldClient {
   /// be null.
   final Directory workDirectory;
 
+  /// The logging function to use when reporting messages to the console.
+  final LogCallback log;
+
   /// The local [Directory] where the Flutter repository is hosted.
   ///
   /// Uses the [fs] file system.
@@ -436,10 +443,7 @@ class SkiaGoldClient {
         }
         expectation = jsonResponse['digest'] as String?;
       } on FormatException catch (error) {
-        // Ideally we'd use something like package:test's printOnError, but best reliability
-        // in getting logs on CI for now we're just using print.
-        // See also: https://github.com/flutter/flutter/issues/91285
-        print( // ignore: avoid_print
+        log(
           'Formatting error detected requesting expectations from Flutter Gold.\n'
           'error: $error\n'
           'url: $requestForExpectations\n'
diff --git a/packages/flutter_goldens/test/flutter_goldens_test.dart b/packages/flutter_goldens/test/flutter_goldens_test.dart
