[flutter_tool] restructure ProjectFileInvalidator to no longer directly depend on context (#45739)

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

@@ -26,6 +26,10 @@ class ContextDependencyCycleException implements Exception {
   String toString() => 'Dependency cycle detected: ${cycle.join(' -> ')}';
 }
 
+/// The Zone key used to look up the [AppContext].
+@visibleForTesting
+const Object contextKey = _Key.key;
+
 /// The current [AppContext], as determined by the [Zone] hierarchy.
 ///
 /// This will be the first context found as we scan up the zone hierarchy, or
@@ -33,7 +37,7 @@ class ContextDependencyCycleException implements Exception {
 /// context will not have any values associated with it.
 ///
 /// This is guaranteed to never return `null`.
-AppContext get context => Zone.current[_Key.key] as AppContext ?? AppContext._root;
+AppContext get context => Zone.current[contextKey] as AppContext ?? AppContext._root;
 
 /// A lookup table (mapping types to values) and an implied scope, in which
 /// code is run.

packages/flutter_tools/lib/src/run_hot.dart

@@ -26,7 +26,12 @@ import 'reporting/reporting.dart';
 import 'resident_runner.dart';
 import 'vmservice.dart';
 
-ProjectFileInvalidator get projectFileInvalidator => context.get<ProjectFileInvalidator>() ?? const ProjectFileInvalidator();
+ProjectFileInvalidator get projectFileInvalidator => context.get<ProjectFileInvalidator>() ?? _defaultInvalidator;
+final ProjectFileInvalidator _defaultInvalidator = ProjectFileInvalidator(
+  fileSystem: fs,
+  platform: platform,
+  logger: logger,
+);
 
 HotRunnerConfig get hotRunnerConfig => context.get<HotRunnerConfig>();
 
@@ -1112,8 +1117,20 @@ class HotRunner extends ResidentRunner {
   }
 }
 
+/// The [ProjectFileInvalidator] track the dependencies for a running
+/// application to determine when they are dirty.
 class ProjectFileInvalidator {
-  const ProjectFileInvalidator();
+  ProjectFileInvalidator({
+    @required FileSystem fileSystem,
+    @required Platform platform,
+    @required Logger logger,
+  }): _fileSystem = fileSystem,
+      _platform = platform,
+      _logger = logger;
+
+  final FileSystem _fileSystem;
+  final Platform _platform;
+  final Logger _logger;
 
   static const String _pubCachePathLinuxAndMac = '.pub-cache';
   static const String _pubCachePathWindows = 'Pub/Cache';
@@ -1142,13 +1159,13 @@ class ProjectFileInvalidator {
     }
 
     final Stopwatch stopwatch = Stopwatch()..start();
-
     final List<Uri> urisToScan = <Uri>[
       // Don't watch pub cache directories to speed things up a little.
-      ...urisToMonitor.where(_isNotInPubCache),
+      for (Uri uri in urisToMonitor)
+        if (_isNotInPubCache(uri)) uri,
 
       // We need to check the .packages file too since it is not used in compilation.
-      fs.file(packagesPath).uri,
+      _fileSystem.file(packagesPath).uri,
     ];
     final List<Uri> invalidatedFiles = <Uri>[];
 
@@ -1157,8 +1174,8 @@ class ProjectFileInvalidator {
       final List<Future<void>> waitList = <Future<void>>[];
       for (final Uri uri in urisToScan) {
         waitList.add(pool.withResource<void>(
-          () => fs
-            .stat(uri.toFilePath(windows: platform.isWindows))
+          () => _fileSystem
+            .stat(uri.toFilePath(windows: _platform.isWindows))
             .then((FileStat stat) {
               final DateTime updatedAt = stat.modified;
               if (updatedAt != null && updatedAt.isAfter(lastCompiled)) {
@@ -1170,14 +1187,14 @@ class ProjectFileInvalidator {
       await Future.wait<void>(waitList);
     } else {
       for (final Uri uri in urisToScan) {
-        final DateTime updatedAt = fs.statSync(
-            uri.toFilePath(windows: platform.isWindows)).modified;
+        final DateTime updatedAt = _fileSystem.statSync(
+            uri.toFilePath(windows: _platform.isWindows)).modified;
         if (updatedAt != null && updatedAt.isAfter(lastCompiled)) {
           invalidatedFiles.add(uri);
         }
       }
     }
-    printTrace(
+    _logger.printTrace(
       'Scanned through ${urisToScan.length} files in '
       '${stopwatch.elapsedMilliseconds}ms'
       '${asyncScanning ? " (async)" : ""}',
@@ -1185,8 +1202,8 @@ class ProjectFileInvalidator {
     return invalidatedFiles;
   }
 
-  static bool _isNotInPubCache(Uri uri) {
-    return !(platform.isWindows && uri.path.contains(_pubCachePathWindows))
+  bool _isNotInPubCache(Uri uri) {
+    return !(_platform.isWindows && uri.path.contains(_pubCachePathWindows))
         && !uri.path.contains(_pubCachePathLinuxAndMac);
   }
 }

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

@@ -3,6 +3,7 @@
 // found in the LICENSE file.
 
 import 'package:flutter_tools/src/base/common.dart';
+import 'package:flutter_tools/src/base/context.dart';
 
 import '../../src/common.dart';
 
@@ -23,5 +24,9 @@ void main() {
     test('throws ToolExit with message and exit code', () {
       expect(() => throwToolExit('message', exitCode: 42), throwsToolExit(exitCode: 42, message: 'message'));
     });
+
+    testWithoutContext('Throws if accessing the Zone', () {
+      expect(() => context.get<Object>(), throwsA(isInstanceOf<UnsupportedError>()));
+    });
   });
 }

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

@@ -3,67 +3,69 @@
 // found in the LICENSE file.
 
 import 'package:file/memory.dart';
-import 'package:flutter_tools/src/base/file_system.dart';
+import 'package:flutter_tools/src/base/logger.dart';
+import 'package:flutter_tools/src/base/platform.dart';
 import 'package:flutter_tools/src/run_hot.dart';
-import 'package:meta/meta.dart';
 
 import '../src/common.dart';
-import '../src/context.dart';
 
 // assumption: tests have a timeout less than 100 days
 final DateTime inFuture = DateTime.now().add(const Duration(days: 100));
 
 void main() {
-  group('ProjectFileInvalidator', () {
-    _testProjectFileInvalidator(asyncScanning: false);
-  });
-  group('ProjectFileInvalidator (async scanning)', () {
-    _testProjectFileInvalidator(asyncScanning: true);
-  });
-}
+  for (bool asyncScanning in <bool>[true, false]) {
+    testWithoutContext('No last compile, asyncScanning: $asyncScanning', () async {
+      final ProjectFileInvalidator projectFileInvalidator = ProjectFileInvalidator(
+        fileSystem: MemoryFileSystem(),
+        platform: FakePlatform(),
+        logger: BufferLogger(),
+      );
+
+      expect(
+        await projectFileInvalidator.findInvalidated(
+          lastCompiled: null,
+          urisToMonitor: <Uri>[],
+          packagesPath: '',
+          asyncScanning: asyncScanning,
+        ),
+        isEmpty,
+      );
+    });
 
-void _testProjectFileInvalidator({@required bool asyncScanning}) {
-  const ProjectFileInvalidator projectFileInvalidator = ProjectFileInvalidator();
+    testWithoutContext('Empty project, asyncScanning: $asyncScanning', () async {
+      final ProjectFileInvalidator projectFileInvalidator = ProjectFileInvalidator(
+        fileSystem: MemoryFileSystem(),
+        platform: FakePlatform(),
+        logger: BufferLogger(),
+      );
 
-  testUsingContext('No last compile', () async {
-    expect(
-      await projectFileInvalidator.findInvalidated(
-        lastCompiled: null,
-        urisToMonitor: <Uri>[],
-        packagesPath: '',
-        asyncScanning: asyncScanning,
-      ),
-      isEmpty,
-    );
-  });
+      expect(
+        await projectFileInvalidator.findInvalidated(
+          lastCompiled: inFuture,
+          urisToMonitor: <Uri>[],
+          packagesPath: '',
+          asyncScanning: asyncScanning,
+        ),
+        isEmpty,
+      );
+    });
 
-  testUsingContext('Empty project', () async {
-    expect(
-      await projectFileInvalidator.findInvalidated(
-        lastCompiled: inFuture,
-        urisToMonitor: <Uri>[],
-        packagesPath: '',
-        asyncScanning: asyncScanning,
-      ),
-      isEmpty,
-    );
-  }, overrides: <Type, Generator>{
-    FileSystem: () => MemoryFileSystem(),
-    ProcessManager: () => FakeProcessManager.any(),
-  });
+    testWithoutContext('Non-existent files are ignored, asyncScanning: $asyncScanning', () async {
+      final ProjectFileInvalidator projectFileInvalidator = ProjectFileInvalidator(
+        fileSystem: MemoryFileSystem(),
+        platform: FakePlatform(),
+        logger: BufferLogger(),
+      );
 
-  testUsingContext('Non-existent files are ignored', () async {
-    expect(
-      await projectFileInvalidator.findInvalidated(
-        lastCompiled: inFuture,
-        urisToMonitor: <Uri>[Uri.parse('/not-there-anymore'),],
-        packagesPath: '',
-        asyncScanning: asyncScanning,
-      ),
-      isEmpty,
-    );
-  }, overrides: <Type, Generator>{
-    FileSystem: () => MemoryFileSystem(),
-    ProcessManager: () => FakeProcessManager.any(),
-  });
+      expect(
+        await projectFileInvalidator.findInvalidated(
+          lastCompiled: inFuture,
+          urisToMonitor: <Uri>[Uri.parse('/not-there-anymore'),],
+          packagesPath: '',
+          asyncScanning: asyncScanning,
+        ),
+        isEmpty,
+      );
+    });
+  }
 }

packages/flutter_tools/test/src/common.dart

@@ -6,13 +6,15 @@ import 'dart:async';
 
 import 'package:args/command_runner.dart';
 import 'package:flutter_tools/src/base/common.dart';
+import 'package:flutter_tools/src/base/context.dart';
 import 'package:flutter_tools/src/base/file_system.dart';
 import 'package:flutter_tools/src/base/platform.dart';
 import 'package:flutter_tools/src/base/process.dart';
 import 'package:flutter_tools/src/commands/create.dart';
 import 'package:flutter_tools/src/runner/flutter_command.dart';
 import 'package:flutter_tools/src/runner/flutter_command_runner.dart';
-import 'package:test_api/test_api.dart' as test_package show TypeMatcher; // ignore: deprecated_member_use
+import 'package:meta/meta.dart';
+import 'package:test_api/test_api.dart' as test_package show TypeMatcher, test; // ignore: deprecated_member_use
 import 'package:test_api/test_api.dart' hide TypeMatcher, isInstanceOf; // ignore: deprecated_member_use
 // ignore: deprecated_member_use
 export 'package:test_core/test_core.dart' hide TypeMatcher, isInstanceOf; // Defines a 'package:test' shim.
@@ -137,3 +139,67 @@ Future<void> expectToolExitLater(Future<dynamic> future, Matcher messageMatcher)
     fail('ToolExit expected, got $e\n$trace');
   }
 }
+
+/// Executes a test body in zone that does not allow context-based injection.
+///
+/// For classes which have been refactored to excluded context-based injection
+/// or globals like [fs] or [platform], prefer using this test method as it
+/// will prevent accidentally including these context getters in future code
+/// changes.
+///
+/// For more information, see https://github.com/flutter/flutter/issues/47161
+@isTest
+void testWithoutContext(String description, FutureOr<void> body(), {
+  String testOn,
+  Timeout timeout,
+  bool skip,
+  List<String> tags,
+  Map<String, dynamic> onPlatform,
+  int retry,
+  }) {
+  return test_package.test(
+    description, () async {
+      return runZoned(body, zoneValues: <Object, Object>{
+        contextKey: const NoContext(),
+      });
+    },
+    timeout: timeout,
+    skip: skip,
+    tags: tags,
+    onPlatform: onPlatform,
+    retry: retry,
+    testOn: testOn,
+  );
+}
+
+/// An implementation of [AppContext] that throws if context.get is called in the test.
+///
+/// The intention of the class is to ensure we do not accidentally regress when
+/// moving towards more explicit dependency injection by accidentally using
+/// a Zone value in place of a constructor parameter.
+class NoContext implements AppContext {
+  const NoContext();
+
+  @override
+  T get<T>() {
+    throw UnsupportedError(
+      'context.get<$T> is not supported in test methods. '
+      'Use Testbed or testUsingContext if accessing Zone injected '
+      'values.'
+    );
+  }
+
+  @override
+  String get name => 'No Context';
+
+  @override
+  Future<V> run<V>({
+    FutureOr<V> Function() body,
+    String name,
+    Map<Type, Generator> overrides,
+    Map<Type, Generator> fallbacks,
+    ZoneSpecification zoneSpecification,
+  }) async {
+    return body();
+  }
+}