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

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.

dev/automated_tests/flutter_test/flutter_gold_expectation.txt

-[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! *

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,9 +35,11 @@ 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),
@@ -44,9 +47,15 @@ void main() {
       ),
       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),
@@ -54,6 +63,11 @@ void main() {
       ),
       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]);
   });
 }
 

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'

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'

packages/flutter_goldens/test/flutter_goldens_test.dart

@@ -58,6 +58,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
     });
 
@@ -77,6 +78,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       final File goldenFile = fs.file('/workDirectory/temp/golden_file_test.png')
@@ -120,6 +122,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       final File goldenFile = fs.file('/workDirectory/temp/golden_file_test.png')
@@ -180,6 +183,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       process.fallbackProcessResult = ProcessResult(123, 1, 'Fallback failure', 'Fallback failure');
@@ -205,6 +209,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       const RunInvocation gitInvocation = RunInvocation(
@@ -249,6 +254,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       const RunInvocation gitInvocation = RunInvocation(
@@ -301,6 +307,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       const RunInvocation gitInvocation = RunInvocation(
@@ -357,6 +364,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       const RunInvocation goldctlInvocation = RunInvocation(
@@ -397,6 +405,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       final List<String> ciArguments = skiaClient.getCIArguments();
@@ -433,6 +442,7 @@ void main() {
         platform: platform,
         httpClient: fakeHttpClient,
         abi: Abi.linuxX64,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       traceID = skiaClient.getTraceID('flutter.golden.1');
@@ -461,6 +471,7 @@ void main() {
         platform: platform,
         httpClient: fakeHttpClient,
         abi: Abi.linuxX64,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       traceID = skiaClient.getTraceID('flutter.golden.1');
@@ -484,6 +495,7 @@ void main() {
         platform: platform,
         httpClient: fakeHttpClient,
         abi: Abi.linuxX64,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       traceID = skiaClient.getTraceID('flutter.golden.1');
@@ -510,6 +522,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       const RunInvocation goldctlInvocation = RunInvocation(
@@ -554,6 +567,7 @@ void main() {
         process: process,
         platform: platform,
         httpClient: fakeHttpClient,
+        log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
       );
 
       const RunInvocation goldctlInvocation = RunInvocation(
@@ -605,9 +619,11 @@ void main() {
   });
 
   group('FlutterGoldenFileComparator', () {
+    final List<String> log = <String>[];
     late FlutterGoldenFileComparator comparator;
 
     setUp(() {
+      log.clear(); // TODO(ianh): inline the setup into the tests to avoid risk of accidental bleed-through
       final Directory basedir = fs.directory('flutter/test/library/')
         ..createSync(recursive: true);
       comparator = FlutterPostSubmitFileComparator(
@@ -615,6 +631,7 @@ void main() {
         FakeSkiaGoldClient(),
         fs: fs,
         platform: platform,
+        log: log.add,
       );
     });
 
@@ -637,9 +654,11 @@ void main() {
     test('ignores version number', () {
       final Uri key = comparator.getTestUri(Uri.parse('foo.png'), 1);
       expect(key, Uri.parse('foo.png'));
+      expect(log, isEmpty);
     });
 
     test('adds namePrefix', () async {
+      final List<String> log = <String>[];
       const String libraryName = 'sidedishes';
       const String namePrefix = 'tomatosalad';
       const String fileName = 'lettuce.png';
@@ -652,18 +671,22 @@ void main() {
         fs: fs,
         platform: platform,
         namePrefix: namePrefix,
+        log: log.add,
       );
       await comparator.compare(
         Uint8List.fromList(_kTestPngBytes),
         Uri.parse(fileName),
       );
       expect(fakeSkiaClient.testNames.single, '$namePrefix.$libraryName.$fileName');
+      expect(log, isEmpty);
     });
 
     group('Post-Submit', () {
+      final List<String> log = <String>[];
       late FakeSkiaGoldClient fakeSkiaClient;
 
       setUp(() {
+        log.clear(); // TODO(ianh): inline the setup into the tests to avoid risk of accidental bleed-through
         fakeSkiaClient = FakeSkiaGoldClient();
         final Directory basedir = fs.directory('flutter/test/library/')
           ..createSync(recursive: true);
@@ -672,6 +695,7 @@ void main() {
           fakeSkiaClient,
           fs: fs,
           platform: platform,
+          log: log.add,
         );
       });
 
@@ -693,6 +717,7 @@ void main() {
             ),
           ),
         );
+        expect(log, isEmpty);
       });
 
       test('calls init during compare', () {
@@ -702,6 +727,7 @@ void main() {
           Uri.parse('flutter.golden_test.1.png'),
         );
         expect(fakeSkiaClient.initCalls, 1);
+        expect(log, isEmpty);
       });
 
       test('does not call init in during construction', () {
@@ -709,6 +735,7 @@ void main() {
         FlutterPostSubmitFileComparator.fromDefaultComparator(
           platform,
           goldens: fakeSkiaClient,
+          log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
         );
         expect(fakeSkiaClient.initCalls, 0);
       });
@@ -828,9 +855,11 @@ void main() {
     });
 
     group('Pre-Submit', () {
+      final List<String> log = <String>[];
       late FakeSkiaGoldClient fakeSkiaClient;
 
       setUp(() {
+        log.clear(); // TODO(ianh): inline the setup into the tests to avoid risk of accidental bleed-through
         fakeSkiaClient = FakeSkiaGoldClient();
         final Directory basedir = fs.directory('flutter/test/library/')
           ..createSync(recursive: true);
@@ -839,6 +868,7 @@ void main() {
           fakeSkiaClient,
           fs: fs,
           platform: platform,
+          log: log.add,
         );
       });
 
@@ -860,6 +890,7 @@ void main() {
             ),
           ),
         );
+        expect(log, isEmpty);
       });
 
       test('calls init during compare', () {
@@ -869,6 +900,7 @@ void main() {
           Uri.parse('flutter.golden_test.1.png'),
         );
         expect(fakeSkiaClient.tryInitCalls, 1);
+        expect(log, isEmpty);
       });
 
       test('does not call init in during construction', () {
@@ -876,6 +908,7 @@ void main() {
         FlutterPostSubmitFileComparator.fromDefaultComparator(
           platform,
           goldens: fakeSkiaClient,
+          log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
         );
         expect(fakeSkiaClient.tryInitCalls, 0);
       });
@@ -1090,10 +1123,12 @@ void main() {
     });
 
     group('Local', () {
+      final List<String> log = <String>[];
       late FlutterLocalFileComparator comparator;
       final FakeSkiaGoldClient fakeSkiaClient = FakeSkiaGoldClient();
 
       setUp(() async {
+        log.clear(); // TODO(ianh): inline the setup into the tests to avoid risk of accidental bleed-through
         final Directory basedir = fs.directory('flutter/test/library/')
           ..createSync(recursive: true);
         comparator = FlutterLocalFileComparator(
@@ -1104,6 +1139,7 @@ void main() {
             environment: <String, String>{'FLUTTER_ROOT': _kFlutterRoot},
             operatingSystem: 'macos',
           ),
+          log: log.add,
         );
 
         const String hash = '55109a4bed52acc780530f7a9aeff6c0';
@@ -1130,6 +1166,7 @@ void main() {
             ),
           ),
         );
+        expect(log, isEmpty);
       });
 
       test('passes when bytes match', () async {
@@ -1140,6 +1177,7 @@ void main() {
           ),
           isTrue,
         );
+        expect(log, isEmpty);
       });
 
       test('returns FlutterSkippingGoldenFileComparator when network connection is unavailable', () async {
@@ -1153,6 +1191,7 @@ void main() {
           platform,
           goldens: fakeSkiaClient,
           baseDirectory: fakeDirectory,
+          log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
         );
         expect(comparator.runtimeType, FlutterSkippingFileComparator);
 
@@ -1162,6 +1201,7 @@ void main() {
           platform,
           goldens: fakeSkiaClient,
           baseDirectory: fakeDirectory,
+          log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
         );
         expect(comparator.runtimeType, FlutterSkippingFileComparator);
 
@@ -1171,6 +1211,7 @@ void main() {
           platform,
           goldens: fakeSkiaClient,
           baseDirectory: fakeDirectory,
+          log: (String message) => fail('skia gold client printed unexpected output: "$message"'),
         );
         expect(comparator.runtimeType, FlutterSkippingFileComparator);