[flutter_tools] abstract logger construction (why can't I hold all these loggers) (#61201)

We have too many loggers, and the logger construction rules are too complicated to be untested. Capture these in a LoggerFactory and test that construction is correct.

packages/flutter_tools/lib/executable.dart

@@ -4,13 +4,17 @@
 
 import 'dart:async';
 
+import 'package:meta/meta.dart';
+
 import 'runner.dart' as runner;
 import 'src/base/context.dart';
+import 'src/base/io.dart';
 import 'src/base/logger.dart';
 import 'src/base/template.dart';
 // The build_runner code generation is provided here to make it easier to
 // avoid introducing the dependency into google3. Not all build* packages
 // are synced internally.
+import 'src/base/terminal.dart';
 import 'src/build_runner/build_runner.dart';
 import 'src/build_runner/mustache_template.dart';
 import 'src/build_runner/resident_web_runner.dart';
@@ -135,36 +139,80 @@ Future<void> main(List<String> args) async {
        WebRunnerFactory: () => DwdsWebRunnerFactory(),
        // The mustache dependency is different in google3
        TemplateRenderer: () => const MustacheTemplateRenderer(),
-       if (daemon)
-        Logger: () => NotifyingLogger(
-          verbose: verbose,
-          parent: VerboseLogger(StdoutLogger(
-            timeoutConfiguration: timeoutConfiguration,
-            stdio: globals.stdio,
-            terminal: globals.terminal,
+       Logger: () {
+        final LoggerFactory loggerFactory = LoggerFactory(
           outputPreferences: globals.outputPreferences,
-          ),
-        ))
-       else if (runMachine && !verbose)
-        Logger: () => AppRunLogger(parent: StdoutLogger(
-          timeoutConfiguration: timeoutConfiguration,
-          stdio: globals.stdio,
           terminal: globals.terminal,
-          outputPreferences: globals.outputPreferences,
-        ))
-       else if (runMachine && verbose)
-        Logger: () => AppRunLogger(parent: VerboseLogger(StdoutLogger(
-          timeoutConfiguration: timeoutConfiguration,
           stdio: globals.stdio,
-          terminal: globals.terminal,
-          outputPreferences: globals.outputPreferences,
-        )))
-       else if (verbose && !muteCommandLogging)
-        Logger: () => VerboseLogger(StdoutLogger(
           timeoutConfiguration: timeoutConfiguration,
-          stdio: globals.stdio,
-          terminal: globals.terminal,
-          outputPreferences: globals.outputPreferences,
-        ))
+        );
+        return loggerFactory.createLogger(
+          daemon: daemon,
+          machine: runMachine,
+          verbose: verbose && !muteCommandLogging,
+          windows: globals.platform.isWindows,
+        );
+       }
      });
 }
+
+
+/// An abstraction for instantiation of the correct logger type.
+///
+/// Our logger class hierarchy and runtime requirements are overly complicated.
+class LoggerFactory {
+  LoggerFactory({
+    @required Terminal terminal,
+    @required Stdio stdio,
+    @required OutputPreferences outputPreferences,
+    @required TimeoutConfiguration timeoutConfiguration,
+    StopwatchFactory stopwatchFactory = const StopwatchFactory(),
+  }) : _terminal = terminal,
+       _stdio = stdio,
+       _timeoutConfiguration = timeoutConfiguration,
+       _stopwatchFactory = stopwatchFactory,
+       _outputPreferences = outputPreferences;
+
+  final Terminal _terminal;
+  final Stdio _stdio;
+  final TimeoutConfiguration _timeoutConfiguration;
+  final StopwatchFactory _stopwatchFactory;
+  final OutputPreferences _outputPreferences;
+
+  /// Create the appropriate logger for the current platform and configuration.
+  Logger createLogger({
+    @required bool verbose,
+    @required bool machine,
+    @required bool daemon,
+    @required bool windows,
+  }) {
+    Logger logger;
+    if (windows) {
+      logger = WindowsStdoutLogger(
+        terminal: _terminal,
+        stdio: _stdio,
+        outputPreferences: _outputPreferences,
+        timeoutConfiguration: _timeoutConfiguration,
+        stopwatchFactory: _stopwatchFactory,
+      );
+    } else {
+      logger = StdoutLogger(
+        terminal: _terminal,
+        stdio: _stdio,
+        outputPreferences: _outputPreferences,
+        timeoutConfiguration: _timeoutConfiguration,
+        stopwatchFactory: _stopwatchFactory
+      );
+    }
+    if (verbose) {
+      logger = VerboseLogger(logger, stopwatchFactory: _stopwatchFactory);
+    }
+    if (daemon) {
+      return NotifyingLogger(verbose: verbose, parent: logger);
+    }
+    if (machine) {
+      return AppRunLogger(parent: logger);
+    }
+    return logger;
+  }
+}

packages/flutter_tools/lib/src/base/logger.dart

@@ -175,7 +175,7 @@ abstract class Logger {
 
 class StdoutLogger extends Logger {
   StdoutLogger({
-    @required AnsiTerminal terminal,
+    @required Terminal terminal,
     @required Stdio stdio,
     @required OutputPreferences outputPreferences,
     @required TimeoutConfiguration timeoutConfiguration,
@@ -188,7 +188,7 @@ class StdoutLogger extends Logger {
       _stopwatchFactory = stopwatchFactory;
 
   @override
-  final AnsiTerminal _terminal;
+  final Terminal _terminal;
   @override
   final OutputPreferences _outputPreferences;
   @override
@@ -345,7 +345,7 @@ class StdoutLogger extends Logger {
 /// they will show up as the unrepresentable character symbol '�'.
 class WindowsStdoutLogger extends StdoutLogger {
   WindowsStdoutLogger({
-    @required AnsiTerminal terminal,
+    @required Terminal terminal,
     @required Stdio stdio,
     @required OutputPreferences outputPreferences,
     @required TimeoutConfiguration timeoutConfiguration,
@@ -1006,7 +1006,7 @@ class AnsiStatus extends AnsiSpinner {
     this.padding = kDefaultStatusPadding,
     @required Duration timeout,
     @required Stopwatch stopwatch,
-    @required AnsiTerminal terminal,
+    @required Terminal terminal,
     VoidCallback onFinish,
     Stdio stdio,
     TimeoutConfiguration timeoutConfiguration,

packages/flutter_tools/test/general.shard/base/logger_test.dart

@@ -5,10 +5,12 @@
 import 'dart:async';
 import 'dart:convert' show jsonEncode;
 
+import 'package:flutter_tools/executable.dart';
 import 'package:flutter_tools/src/base/io.dart';
 import 'package:flutter_tools/src/base/logger.dart';
 import 'package:flutter_tools/src/base/platform.dart';
 import 'package:flutter_tools/src/base/terminal.dart';
+import 'package:flutter_tools/src/commands/daemon.dart';
 import 'package:mockito/mockito.dart';
 import 'package:quiver/testing/async.dart';
 
@@ -24,6 +26,52 @@ final String resetColor = RegExp.escape(AnsiTerminal.resetColor);
 class MockStdout extends Mock implements Stdout {}
 
 void main() {
+  testWithoutContext('correct logger instance is created', () {
+    final LoggerFactory loggerFactory = LoggerFactory(
+      terminal: Terminal.test(),
+      stdio: mocks.MockStdio(),
+      outputPreferences: OutputPreferences.test(),
+      timeoutConfiguration: const TimeoutConfiguration(),
+    );
+
+    expect(loggerFactory.createLogger(
+      verbose: false,
+      machine: false,
+      daemon: false,
+      windows: false,
+    ), isA<StdoutLogger>());
+    expect(loggerFactory.createLogger(
+      verbose: false,
+      machine: false,
+      daemon: false,
+      windows: true,
+    ), isA<WindowsStdoutLogger>());
+    expect(loggerFactory.createLogger(
+      verbose: true,
+      machine: false,
+      daemon: false,
+      windows: true,
+    ), isA<VerboseLogger>());
+    expect(loggerFactory.createLogger(
+      verbose: true,
+      machine: false,
+      daemon: false,
+      windows: false,
+    ), isA<VerboseLogger>());
+    expect(loggerFactory.createLogger(
+      verbose: false,
+      machine: false,
+      daemon: true,
+      windows: false,
+    ), isA<NotifyingLogger>());
+    expect(loggerFactory.createLogger(
+      verbose: false,
+      machine: true,
+      daemon: false,
+      windows: false,
+    ), isA<AppRunLogger>());
+  });
+
   group('AppContext', () {
     FakeStopwatch fakeStopWatch;