From 1271447bbe6b0f72eeee9028d60e723cf4b8e752 Mon Sep 17 00:00:00 2001 From: Jonah Williams Date: Mon, 5 Oct 2020 17:58:56 -0700 Subject: [PATCH] [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 --- .../lib/src/commands/doctor.dart | 10 -- packages/flutter_tools/lib/src/doctor.dart | 106 ++++++++---- .../commands.shard/hermetic/doctor_test.dart | 110 ------------- .../general.shard/flutter_validator_test.dart | 152 ++++++++++++++++++ .../general.shard/runner/runner_test.dart | 5 +- packages/flutter_tools/test/src/common.dart | 13 ++ 6 files changed, 242 insertions(+), 154 deletions(-) create mode 100644 packages/flutter_tools/test/general.shard/flutter_validator_test.dart diff --git a/packages/flutter_tools/lib/src/commands/doctor.dart b/packages/flutter_tools/lib/src/commands/doctor.dart index 2c682fe9da6..7f834115cff 100644 --- a/packages/flutter_tools/lib/src/commands/doctor.dart +++ b/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> get requiredArtifacts async { - return { - // 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 runCommand() async { globals.flutterVersion.fetchTagsAndUpdate(); diff --git a/packages/flutter_tools/lib/src/doctor.dart b/packages/flutter_tools/lib/src/doctor.dart index f3c75592286..ee83be87630 100644 --- a/packages/flutter_tools/lib/src/doctor.dart +++ b/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 = [ - 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([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 validate() async { @@ -661,65 +702,64 @@ 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); + // 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(buf.toString())); + 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) { - const int kExpectedExitCode = 255; - try { - return processUtils.runSync([genSnapshotPath]).exitCode == kExpectedExitCode; - } on Exception { - return false; + bool _genSnapshotRuns(String genSnapshotPath) { + const int kExpectedExitCode = 255; + try { + return _processManager.runSync([genSnapshotPath]).exitCode == kExpectedExitCode; + } on Exception { + return false; + } } } diff --git a/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart b/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart index 832d3eec200..75cee83641c 100644 --- a/packages/flutter_tools/test/commands.shard/hermetic/doctor_test.dart +++ b/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( - [genSnapshotPath], - workingDirectory: anyNamed('workingDirectory'), - environment: anyNamed('environment'), - )).thenReturn(ProcessResult(101, 1, '', '')); - - expect(await FlutterValidatorDoctor(logger).diagnose(verbose: false), isTrue); - final List 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: { - 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: { - 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( - [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: { - Artifacts: () => artifacts, - FileSystem: () => memoryFileSystem, - OutputPreferences: () => OutputPreferences(wrapText: false), - ProcessManager: () => mockProcessManager, - Platform: _kNoColorOutputPlatform, - FlutterVersion: () => mockFlutterVersion, - }); - - testUsingContext('shows mirrors', () async { - (globals.platform as FakePlatform).environment = { - '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: { - 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 _validators; - @override - List get validators { - return _validators ??= [ - FlutterValidator(), - ]; - } -} - /// A doctor that takes any two validators. Used to check behavior when /// merging ValidationTypes (installed, missing, partial). class FakeSmallGroupDoctor extends Doctor { diff --git a/packages/flutter_tools/test/general.shard/flutter_validator_test.dart b/packages/flutter_tools/test/general.shard/flutter_validator_test.dart new file mode 100644 index 00000000000..5ade6cb9ae1 --- /dev/null +++ b/packages/flutter_tools/test/general.shard/flutter_validator_test.dart @@ -0,0 +1,152 @@ +// 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: {}, + ), + flutterVersion: () => flutterVersion, + userMessages: UserMessages(), + artifacts: artifacts, + fileSystem: fileSystem, + flutterRoot: () => 'sdk/flutter', + operatingSystemUtils: FakeOperatingSystemUtils(name: 'Linux'), + processManager: FakeProcessManager.list([ + const FakeCommand( + command: ['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.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: {}, + ), + flutterVersion: () => MockFlutterVersion(), + userMessages: UserMessages(), + artifacts: Artifacts.test(), + fileSystem: MemoryFileSystem.test(), + operatingSystemUtils: FakeOperatingSystemUtils(name: 'Windows'), + processManager: FakeProcessManager.list([]), + 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([]), + 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('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: { + '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('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; +} diff --git a/packages/flutter_tools/test/general.shard/runner/runner_test.dart b/packages/flutter_tools/test/general.shard/runner/runner_test.dart index 5a7e8cf5d47..8c6dfb1e672 100644 --- a/packages/flutter_tools/test/general.shard/runner/runner_test.dart +++ b/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(), }); }); } diff --git a/packages/flutter_tools/test/src/common.dart b/packages/flutter_tools/test/src/common.dart index f5a1fe51861..c63ebd02e78 100644 --- a/packages/flutter_tools/test/src/common.dart +++ b/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() + .having((ValidationResult result) => result.type, 'type', validationType) + .having((ValidationResult result) => result.statusInfo, 'statusInfo', statusInfo) + .having((ValidationResult result) => result.messages, 'messages', messages); +}