From fbca40add2c0bdba9f779fde57093314512fe43d Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Fri, 10 Jul 2020 11:51:47 -0700 Subject: [PATCH] [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 | 104 +++++++++++++----- .../flutter_tools/lib/src/base/logger.dart | 8 +- .../test/general.shard/base/logger_test.dart | 48 ++++++++ 3 files changed, 128 insertions(+), 32 deletions(-) diff --git a/packages/flutter_tools/lib/executable.dart b/packages/flutter_tools/lib/executable.dart index b0eb4adb378..e69ba78521e 100644 --- a/packages/flutter_tools/lib/executable.dart +++ b/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 main(List 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, - outputPreferences: globals.outputPreferences, - ), - )) - else if (runMachine && !verbose) - Logger: () => AppRunLogger(parent: StdoutLogger( - timeoutConfiguration: timeoutConfiguration, - stdio: globals.stdio, - terminal: globals.terminal, + Logger: () { + final LoggerFactory loggerFactory = LoggerFactory( 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, - )) + timeoutConfiguration: timeoutConfiguration, + ); + 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; + } +} diff --git a/packages/flutter_tools/lib/src/base/logger.dart b/packages/flutter_tools/lib/src/base/logger.dart index 075c234c180..20ab0e1b45c 100644 --- a/packages/flutter_tools/lib/src/base/logger.dart +++ b/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, diff --git a/packages/flutter_tools/test/general.shard/base/logger_test.dart b/packages/flutter_tools/test/general.shard/base/logger_test.dart index 6ca3251da3e..2130457e6d7 100644 --- a/packages/flutter_tools/test/general.shard/base/logger_test.dart +++ b/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()); + expect(loggerFactory.createLogger( + verbose: false, + machine: false, + daemon: false, + windows: true, + ), isA()); + expect(loggerFactory.createLogger( + verbose: true, + machine: false, + daemon: false, + windows: true, + ), isA()); + expect(loggerFactory.createLogger( + verbose: true, + machine: false, + daemon: false, + windows: false, + ), isA()); + expect(loggerFactory.createLogger( + verbose: false, + machine: false, + daemon: true, + windows: false, + ), isA()); + expect(loggerFactory.createLogger( + verbose: false, + machine: true, + daemon: false, + windows: false, + ), isA()); + }); + group('AppContext', () { FakeStopwatch fakeStopWatch;