[flutter_tools] remove globals from FlutterValidator, add documentation and...

[flutter_tools] remove globals from FlutterValidator, add documentation and move tests to new file (#67234)

Remove globals from the flutter validator class, and refactor the tests into a separate file. Applies some other cleanup like adding doc comments, and making the doctor validator work like it is documented to work - removing the gen_snapshot check if the artifact is not downloaded instead of downloading all android artifacts.

#47161

packages/flutter_tools/lib/src/commands/doctor.dart

@@ -28,16 +28,6 @@ class DoctorCommand extends FlutterCommand {
   @override
   final String description = 'Show information about the installed tooling.';
 
-  @override
-  Future<Set<DevelopmentArtifact>> get requiredArtifacts async {
-    return <DevelopmentArtifact>{
-      // This is required because we use gen_snapshot to check if the host
-      // machine can execute the provided artifacts. See `_genSnapshotRuns`
-      // in `doctor.dart`.
-      DevelopmentArtifact.androidGenSnapshot,
-    };
-  }
-
   @override
   Future<FlutterCommandResult> runCommand() async {
     globals.flutterVersion.fetchTagsAndUpdate();

packages/flutter_tools/lib/src/doctor.dart

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import 'package:meta/meta.dart';
+import 'package:process/process.dart';
 
 import 'android/android_studio_validator.dart';
 import 'android/android_workflow.dart';
@@ -11,8 +12,8 @@ import 'base/async_guard.dart';
 import 'base/context.dart';
 import 'base/file_system.dart';
 import 'base/logger.dart';
+import 'base/os.dart';
 import 'base/platform.dart';
-import 'base/process.dart';
 import 'base/terminal.dart';
 import 'base/user_messages.dart';
 import 'base/utils.dart';
@@ -81,7 +82,16 @@ class _DefaultDoctorValidatorsProvider implements DoctorValidatorsProvider {
     ];
     final ProxyValidator proxyValidator = ProxyValidator(platform: globals.platform);
     _validators = <DoctorValidator>[
-      FlutterValidator(),
+      FlutterValidator(
+        fileSystem: globals.fs,
+        platform: globals.platform,
+        flutterVersion: () => globals.flutterVersion,
+        processManager: globals.processManager,
+        userMessages: userMessages,
+        artifacts: globals.artifacts,
+        flutterRoot: () => Cache.flutterRoot,
+        operatingSystemUtils: globals.os,
+      ),
       if (androidWorkflow.appliesToHostPlatform)
         GroupedValidator(<DoctorValidator>[androidValidator, androidLicenseValidator]),
       if (globals.iosWorkflow.appliesToHostPlatform || macOSWorkflow.appliesToHostPlatform)
@@ -650,8 +660,39 @@ class ValidationMessage {
   int get hashCode => type.hashCode ^ message.hashCode ^ contextUrl.hashCode;
 }
 
+/// A validator that checks the version of Flutter, as well as some auxillary information
+/// such as the pub or Flutter cache overrides.
+///
+/// This is primarily useful for diagnosing issues on Github bug reports by displaying
+/// specific commit information.
 class FlutterValidator extends DoctorValidator {
-  FlutterValidator() : super('Flutter');
+  FlutterValidator({
+    @required Platform platform,
+    @required FlutterVersion Function() flutterVersion,
+    @required UserMessages userMessages,
+    @required FileSystem fileSystem,
+    @required Artifacts artifacts,
+    @required ProcessManager processManager,
+    @required String Function() flutterRoot,
+    @required OperatingSystemUtils operatingSystemUtils,
+  }) : _flutterVersion = flutterVersion,
+       _platform = platform,
+       _userMessages = userMessages,
+       _fileSystem = fileSystem,
+       _artifacts = artifacts,
+       _processManager = processManager,
+       _flutterRoot = flutterRoot,
+       _operatingSystemUtils = operatingSystemUtils,
+       super('Flutter');
+
+  final Platform _platform;
+  final FlutterVersion Function() _flutterVersion;
+  final String Function() _flutterRoot;
+  final UserMessages _userMessages;
+  final FileSystem _fileSystem;
+  final Artifacts _artifacts;
+  final ProcessManager _processManager;
+  final OperatingSystemUtils _operatingSystemUtils;
 
   @override
   Future<ValidationResult> validate() async {
@@ -661,66 +702,65 @@ class FlutterValidator extends DoctorValidator {
     String frameworkVersion;
 
     try {
-      final FlutterVersion version = globals.flutterVersion;
+      final FlutterVersion version = _flutterVersion();
       versionChannel = version.channel;
       frameworkVersion = version.frameworkVersion;
-      messages.add(ValidationMessage(userMessages.flutterVersion(
+      messages.add(ValidationMessage(_userMessages.flutterVersion(
         frameworkVersion,
-        Cache.flutterRoot,
+        _flutterRoot(),
       )));
-      messages.add(ValidationMessage(userMessages.flutterRevision(
+      messages.add(ValidationMessage(_userMessages.flutterRevision(
         version.frameworkRevisionShort,
         version.frameworkAge,
         version.frameworkDate,
       )));
-      messages.add(ValidationMessage(userMessages.engineRevision(version.engineRevisionShort)));
-      messages.add(ValidationMessage(userMessages.dartRevision(version.dartSdkVersion)));
-      if (globals.platform.environment.containsKey('PUB_HOSTED_URL')) {
-        messages.add(ValidationMessage(userMessages.pubMirrorURL(globals.platform.environment['PUB_HOSTED_URL'])));
+      messages.add(ValidationMessage(_userMessages.engineRevision(version.engineRevisionShort)));
+      messages.add(ValidationMessage(_userMessages.dartRevision(version.dartSdkVersion)));
+      if (_platform.environment.containsKey('PUB_HOSTED_URL')) {
+        messages.add(ValidationMessage(_userMessages.pubMirrorURL(_platform.environment['PUB_HOSTED_URL'])));
       }
-      if (globals.platform.environment.containsKey('FLUTTER_STORAGE_BASE_URL')) {
-        messages.add(ValidationMessage(userMessages.flutterMirrorURL(globals.platform.environment['FLUTTER_STORAGE_BASE_URL'])));
+      if (_platform.environment.containsKey('FLUTTER_STORAGE_BASE_URL')) {
+        messages.add(ValidationMessage(_userMessages.flutterMirrorURL(_platform.environment['FLUTTER_STORAGE_BASE_URL'])));
       }
     } on VersionCheckError catch (e) {
       messages.add(ValidationMessage.error(e.message));
       valid = ValidationType.partial;
     }
 
-    final String genSnapshotPath =
-      globals.artifacts.getArtifactPath(Artifact.genSnapshot);
-
     // Check that the binaries we downloaded for this platform actually run on it.
-    if (globals.fs.file(genSnapshotPath).existsSync()
-        && !_genSnapshotRuns(genSnapshotPath)) {
-      final StringBuffer buf = StringBuffer();
-      buf.writeln(userMessages.flutterBinariesDoNotRun);
-      if (globals.platform.isLinux) {
-        buf.writeln(userMessages.flutterBinariesLinuxRepairCommands);
-      }
-      messages.add(ValidationMessage.error(buf.toString()));
+    // If the binaries are not downloaded (because android is not enabled), then do
+    // not run this check.
+    final String genSnapshotPath = _artifacts.getArtifactPath(Artifact.genSnapshot);
+    if (_fileSystem.file(genSnapshotPath).existsSync() && !_genSnapshotRuns(genSnapshotPath)) {
+      final StringBuffer buffer = StringBuffer();
+      buffer.writeln(_userMessages.flutterBinariesDoNotRun);
+      if (_platform.isLinux) {
+        buffer.writeln(_userMessages.flutterBinariesLinuxRepairCommands);
+      }
+      messages.add(ValidationMessage.error(buffer.toString()));
       valid = ValidationType.partial;
     }
 
     return ValidationResult(
       valid,
       messages,
-      statusInfo: userMessages.flutterStatusInfo(
+      statusInfo: _userMessages.flutterStatusInfo(
         versionChannel,
         frameworkVersion,
-        globals.os.name,
-        globals.platform.localeName,
+        _operatingSystemUtils.name,
+        _platform.localeName,
       ),
     );
   }
-}
 
-bool _genSnapshotRuns(String genSnapshotPath) {
+  bool _genSnapshotRuns(String genSnapshotPath) {
     const int kExpectedExitCode = 255;
     try {
-    return processUtils.runSync(<String>[genSnapshotPath]).exitCode == kExpectedExitCode;
+      return _processManager.runSync(<String>[genSnapshotPath]).exitCode == kExpectedExitCode;
     } on Exception {
       return false;
     }
+  }
 }
 
 class NoIdeValidator extends DoctorValidator {

packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart

@@ -6,10 +6,8 @@ import 'dart:async';
 
 import 'package:args/command_runner.dart';
 import 'package:file/memory.dart';
-import 'package:flutter_tools/src/artifacts.dart';
 import 'package:flutter_tools/src/base/common.dart';
 import 'package:flutter_tools/src/base/file_system.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';
@@ -424,16 +422,6 @@ void main() {
   });
 
   group('doctor with fake validators', () {
-    Artifacts artifacts;
-    String genSnapshotPath;
-    FileSystem memoryFileSystem;
-
-    setUp(() {
-      memoryFileSystem = MemoryFileSystem.test();
-      artifacts = Artifacts.test();
-      genSnapshotPath = artifacts.getArtifactPath(Artifact.genSnapshot);
-    });
-
     testUsingContext('validate non-verbose output format for run without issues', () async {
       expect(await FakeQuietDoctor(logger).diagnose(verbose: false), isTrue);
       expect(logger.statusText, equals(
@@ -574,92 +562,6 @@ void main() {
               '! Doctor found issues in 4 categories.\n'
       ));
     }, overrides: noColorTerminalOverride);
-
-    testUsingContext('gen_snapshot does not work', () async {
-      memoryFileSystem.file(genSnapshotPath).createSync(recursive: true);
-      when(mockProcessManager.runSync(
-        <String>[genSnapshotPath],
-        workingDirectory: anyNamed('workingDirectory'),
-        environment: anyNamed('environment'),
-      )).thenReturn(ProcessResult(101, 1, '', ''));
-
-      expect(await FlutterValidatorDoctor(logger).diagnose(verbose: false), isTrue);
-      final List<String> statusLines = logger.statusText.split('\n');
-      for (final String msg in userMessages.flutterBinariesDoNotRun.split('\n')) {
-        expect(statusLines, contains(contains(msg)));
-      }
-      if (globals.platform.isLinux) {
-        for (final String msg in userMessages.flutterBinariesLinuxRepairCommands.split('\n')) {
-          expect(statusLines, contains(contains(msg)));
-        }
-      }
-    }, overrides: <Type, Generator>{
-      Artifacts: () => artifacts,
-      FileSystem: () => memoryFileSystem,
-      OutputPreferences: () => OutputPreferences(wrapText: false),
-      ProcessManager: () => mockProcessManager,
-      Platform: _kNoColorOutputPlatform,
-    });
-
-    testUsingContext('gen_snapshot binary not available', () async {
-      expect(await FlutterValidatorDoctor(logger).diagnose(verbose: false), isTrue);
-      // gen_snapshot is downloaded on demand, and the doctor should not
-      // fail if the gen_snapshot binary is not present.
-      expect(logger.statusText, contains('No issues found!'));
-    }, overrides: <Type, Generator>{
-      Artifacts: () => artifacts,
-      FileSystem: () => MemoryFileSystem.test(),
-      ProcessManager: () => FakeProcessManager.any(),
-    });
-
-    testUsingContext('version checking does not work', () async {
-      memoryFileSystem.file(genSnapshotPath).createSync(recursive: true);
-      final VersionCheckError versionCheckError = VersionCheckError('version error');
-
-      when(mockFlutterVersion.channel).thenReturn('unknown');
-      when(mockFlutterVersion.frameworkVersion).thenReturn('0.0.0');
-      when(mockFlutterVersion.frameworkDate).thenThrow(versionCheckError);
-
-      when(mockProcessManager.runSync(
-        <String>[genSnapshotPath],
-        workingDirectory: anyNamed('workingDirectory'),
-        environment: anyNamed('environment'),
-      )).thenReturn(ProcessResult(101, 255, '', ''));
-
-      expect(await FlutterValidatorDoctor(logger).diagnose(verbose: false), isTrue);
-
-      expect(logger.statusText, equals(
-        'Doctor summary (to see all details, run flutter doctor -v):\n'
-          '[!] Flutter (Channel unknown, 0.0.0, on fake OS name and version, locale en_US.UTF-8)\n'
-          '    ✗ version error\n\n'
-          '! Doctor found issues in 1 category.\n'
-      ));
-    }, overrides: <Type, Generator>{
-      Artifacts: () => artifacts,
-      FileSystem: () => memoryFileSystem,
-      OutputPreferences: () => OutputPreferences(wrapText: false),
-      ProcessManager: () => mockProcessManager,
-      Platform: _kNoColorOutputPlatform,
-      FlutterVersion: () => mockFlutterVersion,
-    });
-
-    testUsingContext('shows mirrors', () async {
-      (globals.platform as FakePlatform).environment = <String, String>{
-        'PUB_HOSTED_URL': 'https://example.com/pub',
-        'FLUTTER_STORAGE_BASE_URL': 'https://example.com/flutter',
-      };
-
-      expect(await FlutterValidatorDoctor(logger).diagnose(verbose: true), isTrue);
-      expect(logger.statusText, contains('Pub download mirror https://example.com/pub'));
-      expect(logger.statusText, contains('Flutter download mirror https://example.com/flutter'));
-    }, overrides: <Type, Generator>{
-      Artifacts: () => artifacts,
-      FileSystem: () => memoryFileSystem,
-      OutputPreferences: () => OutputPreferences(wrapText: false),
-      ProcessManager: () => mockProcessManager,
-      Platform: _kNoColorOutputPlatform,
-      FlutterVersion: () => mockFlutterVersion,
-    });
   });
 
   testUsingContext('validate non-verbose output wrapping', () async {
@@ -1195,18 +1097,6 @@ class FakeGroupedDoctorWithStatus extends Doctor {
   }
 }
 
-class FlutterValidatorDoctor extends Doctor {
-  FlutterValidatorDoctor(Logger logger) : super(logger: logger);
-
-  List<DoctorValidator> _validators;
-  @override
-  List<DoctorValidator> get validators {
-    return _validators ??= <DoctorValidator>[
-      FlutterValidator(),
-    ];
-  }
-}
-
 /// A doctor that takes any two validators. Used to check behavior when
 /// merging ValidationTypes (installed, missing, partial).
 class FakeSmallGroupDoctor extends Doctor {

packages/flutter_tools/test/general.shard/flutter_validator_test.dart

+// Copyright 2014 The Flutter Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+import 'package:file/memory.dart';
+import 'package:flutter_tools/src/artifacts.dart';
+import 'package:flutter_tools/src/base/os.dart';
+import 'package:flutter_tools/src/base/platform.dart';
+import 'package:flutter_tools/src/base/user_messages.dart';
+import 'package:flutter_tools/src/doctor.dart';
+import 'package:flutter_tools/src/version.dart';
+import 'package:mockito/mockito.dart';
+
+import '../src/common.dart';
+import '../src/context.dart';
+
+void main() {
+  testWithoutContext('FlutterValidator shows an error message if gen_snapshot is '
+    'downloaded and exits with code 1', () async {
+    final MockFlutterVersion flutterVersion = MockFlutterVersion();
+    final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+    final Artifacts artifacts = Artifacts.test();
+    final FlutterValidator flutterValidator = FlutterValidator(
+      platform: FakePlatform(
+        operatingSystem: 'linux',
+        localeName: 'en_US.UTF-8',
+        environment: <String, String>{},
+      ),
+      flutterVersion: () => flutterVersion,
+      userMessages: UserMessages(),
+      artifacts: artifacts,
+      fileSystem: fileSystem,
+      flutterRoot: () => 'sdk/flutter',
+      operatingSystemUtils: FakeOperatingSystemUtils(name: 'Linux'),
+      processManager: FakeProcessManager.list(<FakeCommand>[
+        const FakeCommand(
+          command: <String>['Artifact.genSnapshot'],
+          exitCode: 1,
+        )
+      ])
+    );
+    fileSystem.file(artifacts.getArtifactPath(Artifact.genSnapshot)).createSync(recursive: true);
+
+
+    expect(await flutterValidator.validate(), matchDoctorValidation(
+      validationType: ValidationType.partial,
+      statusInfo: 'Channel unknown, Unknown, on Linux, locale en_US.UTF-8',
+      messages: containsAll(const <ValidationMessage>[
+        ValidationMessage.error(
+          'Downloaded executables cannot execute on host.\n'
+          'See https://github.com/flutter/flutter/issues/6207 for more information\n'
+          'On Debian/Ubuntu/Mint: sudo apt-get install lib32stdc++6\n'
+          'On Fedora: dnf install libstdc++.i686\n'
+          'On Arch: pacman -S lib32-gcc-libs\n',
+        ),
+      ])),
+    );
+  });
+
+  testWithoutContext('FlutterValidator does not run gen_snapshot binary check if it is not already downloaded', () async {
+    final FlutterValidator flutterValidator = FlutterValidator(
+      platform: FakePlatform(
+        operatingSystem: 'windows',
+        localeName: 'en_US.UTF-8',
+        environment: <String, String>{},
+      ),
+      flutterVersion: () => MockFlutterVersion(),
+      userMessages: UserMessages(),
+      artifacts: Artifacts.test(),
+      fileSystem: MemoryFileSystem.test(),
+      operatingSystemUtils: FakeOperatingSystemUtils(name: 'Windows'),
+      processManager: FakeProcessManager.list(<FakeCommand>[]),
+      flutterRoot: () => 'sdk/flutter',
+    );
+
+    // gen_snapshot is downloaded on demand, and the doctor should not
+    // fail if the gen_snapshot binary is not present.
+    expect(await flutterValidator.validate(), matchDoctorValidation(
+      validationType: ValidationType.installed,
+      statusInfo: 'Channel unknown, Unknown, on Windows, locale en_US.UTF-8',
+      messages: anything,
+    ));
+  });
+
+  testWithoutContext('FlutterValidator handles exception thrown by version checking', () async {
+    final MockFlutterVersion flutterVersion = MockFlutterVersion();
+    final FlutterValidator flutterValidator = FlutterValidator(
+      platform: FakePlatform(operatingSystem: 'windows', localeName: 'en_US.UTF-8'),
+      flutterVersion: () => flutterVersion,
+      userMessages: UserMessages(),
+      artifacts: Artifacts.test(),
+      fileSystem: MemoryFileSystem.test(),
+      operatingSystemUtils: FakeOperatingSystemUtils(name: 'Windows'),
+      processManager: FakeProcessManager.list(<FakeCommand>[]),
+      flutterRoot: () => 'sdk/flutter',
+    );
+
+    when(flutterVersion.channel).thenReturn('unknown');
+    when(flutterVersion.frameworkVersion).thenReturn('0.0.0');
+    when(flutterVersion.frameworkDate).thenThrow(VersionCheckError('version error'));
+
+    expect(await flutterValidator.validate(), matchDoctorValidation(
+      validationType: ValidationType.partial,
+      statusInfo: 'Channel unknown, 0.0.0, on Windows, locale en_US.UTF-8',
+      messages: const <ValidationMessage>[
+        ValidationMessage('Flutter version 0.0.0 at sdk/flutter'),
+        ValidationMessage.error('version error'),
+      ]),
+    );
+  });
+
+  testWithoutContext('FlutterValidator shows mirrors on pub and flutter cloud storage', () async {
+    final Platform platform = FakePlatform(
+      operatingSystem: 'windows',
+      localeName: 'en_US.UTF-8',
+      environment: <String, String>{
+        'PUB_HOSTED_URL': 'https://example.com/pub',
+        'FLUTTER_STORAGE_BASE_URL': 'https://example.com/flutter',
+      },
+    );
+    final MockFlutterVersion flutterVersion = MockFlutterVersion();
+    final MemoryFileSystem fileSystem = MemoryFileSystem.test();
+    final Artifacts artifacts = Artifacts.test();
+    final FlutterValidator flutterValidator = FlutterValidator(
+      platform: platform,
+      flutterVersion: () => flutterVersion,
+      userMessages: UserMessages(),
+      artifacts: artifacts,
+      fileSystem: fileSystem,
+      processManager: FakeProcessManager.any(),
+      operatingSystemUtils: FakeOperatingSystemUtils(name: 'Windows'),
+      flutterRoot: () => 'sdk/flutter'
+    );
+
+    expect(await flutterValidator.validate(), matchDoctorValidation(
+      validationType: ValidationType.installed,
+      statusInfo: 'Channel unknown, Unknown, on Windows, locale en_US.UTF-8',
+      messages: containsAll(const <ValidationMessage>[
+        ValidationMessage('Pub download mirror https://example.com/pub'),
+        ValidationMessage('Flutter download mirror https://example.com/flutter'),
+      ])
+    ));
+  });
+}
+
+class MockFlutterVersion extends Mock implements FlutterVersion {}
+class FakeOperatingSystemUtils extends Fake implements OperatingSystemUtils {
+  FakeOperatingSystemUtils({this.name});
+
+  @override
+  final String name;
+}

packages/flutter_tools/test/general.shard/runner/runner_test.dart

@@ -6,6 +6,7 @@ import 'dart:async';
 
 import 'package:file/memory.dart';
 import 'package:flutter_tools/runner.dart' as runner;
+import 'package:flutter_tools/src/artifacts.dart';
 import 'package:flutter_tools/src/base/common.dart';
 import 'package:flutter_tools/src/base/file_system.dart';
 import 'package:flutter_tools/src/base/io.dart' as io;
@@ -92,6 +93,7 @@ void main() {
       FileSystem: () => MemoryFileSystem.test(),
       ProcessManager: () => FakeProcessManager.any(),
       Usage: () => CrashingUsage(),
+      Artifacts: () => Artifacts.test(),
     });
 
     // This Completer completes when CrashingFlutterCommand.runCommand
@@ -132,8 +134,8 @@ void main() {
       }),
       FileSystem: () => MemoryFileSystem.test(),
       ProcessManager: () => FakeProcessManager.any(),
-
       CrashReporter: () => WaitingCrashReporter(commandCompleter.future),
+      Artifacts: () => Artifacts.test(),
     });
 
     testUsingContext('create local report', () async {
@@ -193,6 +195,7 @@ void main() {
       FileSystem: () => MemoryFileSystem.test(),
       ProcessManager: () => FakeProcessManager.any(),
       UserMessages: () => CustomBugInstructions(),
+      Artifacts: () => Artifacts.test(),
     });
   });
 }

packages/flutter_tools/test/src/common.dart

@@ -9,6 +9,7 @@ import 'package:args/command_runner.dart';
 import 'package:flutter_tools/src/base/logger.dart';
 import 'package:flutter_tools/src/base/platform.dart';
 import 'package:flutter_tools/src/convert.dart';
+import 'package:flutter_tools/src/doctor.dart';
 import 'package:vm_service/vm_service.dart' as vm_service;
 import 'package:path/path.dart' as path; // ignore: package_path_import
 
@@ -413,3 +414,15 @@ class ConfiguredFileSystem extends ForwardingFileSystem {
     return (entities[path] as Directory) ?? super.directory(path);
   }
 }
+
+/// Matches a doctor validation result.
+Matcher matchDoctorValidation({
+  ValidationType validationType,
+  String statusInfo,
+  dynamic messages
+}) {
+  return const test_package.TypeMatcher<ValidationResult>()
+    .having((ValidationResult result) => result.type, 'type', validationType)
+    .having((ValidationResult result) => result.statusInfo, 'statusInfo', statusInfo)
+    .having((ValidationResult result) => result.messages, 'messages', messages);
+}