Refactor how SkiaGoldClient is used

[matanlurey]

This is some of the most confusing/scariest code in the Android scenario_app runner:

// Start a TCP socket in the host, and forward it to the device that runs the tests.
// This allows the test process to start a connection with the host, and write the bytes
// for the screenshots.
// On LUCI, the host uploads the screenshots to Skia Gold.
SkiaGoldClient? skiaGoldClient;
late  ServerSocket server;
final List<Future<void>> pendingComparisons = <Future<void>>[];
await step('Starting server...', () async {
  server = await ServerSocket.bind(InternetAddress.anyIPv4, _tcpPort);
  if (verbose) {
    stdout.writeln('listening on host ${server.address.address}:${server.port}');
  }
  server.listen((Socket client) {
    if (verbose) {
      stdout.writeln('client connected ${client.remoteAddress.address}:${client.remotePort}');
    }
    client.transform(const ScreenshotBlobTransformer()).listen((Screenshot screenshot) {
      final String fileName = screenshot.filename;
      final Uint8List fileContent = screenshot.fileContent;
      if (verbose) {
        log('host received ${fileContent.lengthInBytes} bytes for screenshot `$fileName`');
      }
      assert(skiaGoldClient != null, 'expected Skia Gold client');
      late File goldenFile;
      try {
        goldenFile = File(join(screenshotPath, fileName))..writeAsBytesSync(fileContent, flush: true);
      } on FileSystemException catch (err) {
        panic(<String>['failed to create screenshot $fileName: $err']);
      }
      if (verbose) {
        log('wrote ${goldenFile.absolute.path}');
      }
      if (isSkiaGoldClientAvailable) {
        final Future<void> comparison = skiaGoldClient!
          .addImg(fileName, goldenFile,
                  screenshotSize: screenshot.pixelCount)
          .catchError((dynamic err) {
            panic(<String>['skia gold comparison failed: $err']);
          });
        pendingComparisons.add(comparison);
      }
    },
    onError: (dynamic err) {
      panic(<String>['error while receiving bytes: $err']);
    },
    cancelOnError: true);
  });
});

[jonahwilliams]

If we could save the files on device, then we could use something like adb pull. I'm not actually sure if that code ends up being simpler though, since we may need to request storage permission to do so.

[matanlurey]

Interesting. I'm down for whatever is simpler, let's chat (maybe include @gaaclarke) next week!

[reidbaker]

Do we need to be the ones saving the files though? Can we have adb do the screenshot logic? adb exec-out screencap -p

[matanlurey]

Do we need to be the ones saving the files though? Can we have adb do the screenshot logic? adb exec-out screencap -p

This is a good question. I had an informal chat with @jonahwilliams about this, /cc @gaaclarke.

If we only ever take screenshots at the end of a test, and it the tests were declarative in some way, we could entirely rely on taking a single adb exec-out screenpap -p at the end, and vastly simplify how we're doing things. However, in practice, we don't have the mechanisms to indicate that, and we have an imperative screenshot API.

Another option would be adding some sort of channel back and forth, so that tests could indicate to the runner to take a screenshot. In other words, ScreenshotUtil.capture() would send a message to the runner, which would use ADB on it's behalf, and so on.

That all being said, the current workflow (a) works and (b) seems to be working well enough.

I'll close this as unplanned for now - any of you feel free to re-open if we should re-consider.

[github-actions]

This thread has been automatically locked since there has not been any recent activity after it was closed. If you are still experiencing a similar issue, please open a new bug, including the output of flutter doctor -v and a minimal reproduction of the issue.