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

jonahwilliams · 2019-11-27T23:53:50Z

Description

Removes direct usage of context getters from ProjectFileInvalidator. Allows testing without testbed or context.

…tcher_strategy

jonahwilliams · 2019-12-12T20:31:02Z

FYI @christopherfujino @blasten @jmagman @zanderso

For an example of removing direct context dependencies. I think the tests are a lot easier to read without a testing zone.

christopherfujino · 2019-12-12T20:44:30Z

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

-void _testProjectFileInvalidator({@required bool asyncScanning}) {
-  const ProjectFileInvalidator projectFileInvalidator = ProjectFileInvalidator();
+  for (bool asyncScanning in <bool>[true, false]) {
+    test('No last compile', () async {


should these tests have unique names (i.e. interpolate asyncScanning)?

Good call, fixed

christopherfujino · 2019-12-12T20:47:29Z

This is pretty cool. I agree, the test is more readable and explicit.

zanderso

Generally like the idea.

zanderso · 2019-12-12T21:55:42Z

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

-void _testProjectFileInvalidator({@required bool asyncScanning}) {
-  const ProjectFileInvalidator projectFileInvalidator = ProjectFileInvalidator();
+  for (bool asyncScanning in <bool>[true, false]) {
+    test('No last compile, asyncScanning: $asyncScanning', () async {


These probably need to use testUsingContext() with a context that maps everything to a fake implementation that crashes or a Mock with no behavior to ensure that, e.g. some library called by the ProjectFileInvalidator now or in the future doesn't use default globals, like for the file system:

flutter/packages/flutter_tools/lib/src/base/file_system.dart

Line 25 in 948e2b0

context.get<FileSystem>() ?? _kLocalFs,

We could wrap in a test context, possible fiddle with context a bit so that it threw if accessed at all? I think that would be preferable to creating a bunch of throwing mocks

Giving this a shot by wrapping the existing test method ... we'll see if this works. else we could name it testWithoutContext ... or hopefully something shorter

blasten · 2019-12-12T21:57:47Z

packages/flutter_tools/lib/src/run_hot.dart

 class ProjectFileInvalidator {
-  const ProjectFileInvalidator();
+  ProjectFileInvalidator(
+    this._fileSystem,


I tend to prefer named parameters when a method/constructor involves more than one parameter.

Seems like a good idea. Generally there is a bit more boilerplate involved if we want to use named parameters but still have private fields which is why I avoided it.

Still updated to use named parameters & private fields, its really a minor complaint. Plus it reads a lot better

zanderso · 2019-12-13T18:21:19Z

packages/flutter_tools/test/src/common.dart

 }
+
+@isTest
+void test(String description, FutureOr<void> body(), {


Will this wrapper be in place for all tool tests that just say test() ?

Yes, we could take this opportunity to enforce some reasonable default behavior in the future if necessary, such as providing zone based throwing implementations of the real filesystem or network behavior.

zanderso · 2019-12-13T18:22:07Z

packages/flutter_tools/test/src/common.dart

+  Map<String, dynamic> onPlatform,
+  int retry,
+  }) {
+  return runZoned(() {


Will the test body run in this zone?

zanderso · 2019-12-13T18:26:32Z

packages/flutter_tools/test/src/common.dart

+
+  @override
+  T get<T>() {
+    throw UnsupportedError('context.get is not supported in this test.');


Consider including $T in the exception message.

blasten · 2019-12-13T21:22:15Z

packages/flutter_tools/lib/src/run_hot.dart

 class ProjectFileInvalidator {
-  const ProjectFileInvalidator();
+  ProjectFileInvalidator({
+    @required FileSystem fileSystem,


zanderso · 2019-12-16T20:28:20Z

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

    });
+
+    testWithoutContext('Throws if accessing the Zone', () {
+      expect(() => fs, throwsA(isInstanceOf<UnsupportedError>()));


Should this test just do a context.get() directly so that it isn't dependent on how the fs getter is implemented? A separate test could check that fs checks the context, but I'm not sure how valuable that test would be.

zanderso · 2019-12-16T20:35:04Z

packages/flutter_tools/test/src/common.dart

 }
+
+@isTest
+void testWithoutContext(String description, FutureOr<void> body(), {


This needs some comments about when to use and/or not use this. I think it would also be good to have a code health issue filed on github linked from here that describes what the next steps are, and maybe suggests some candidate components that could benefit.

Filled #47161 and added a doc comment

jonahwilliams and others added 2 commits November 27, 2019 15:13

add hybrid watching strategt to speed up dependency checking

93237b7

fix windows path

3d664a1

fluttergithubbot added the tool Affects the "flutter" command-line tool. See also t: labels. label Nov 27, 2019

googlebot added the cla: yes label Nov 27, 2019

Jonah Williams added 4 commits November 27, 2019 21:20

Update run_hot.dart

b9c9027

Update run_hot.dart

4849c5c

add more test cases

4d20876

fix usage of local platform

1a189ea

zanderso added the work in progress; do not review label Dec 5, 2019

jonahwilliams added 3 commits December 12, 2019 11:26

Merge branch 'master' of github.com:flutter/flutter into switching_wa…

09cdade

…tcher_strategy

remove changes

ef14386

Update run_hot.dart

afcf83f

jonahwilliams changed the title ~~Add hybrid watching strategy to speed up dependency checking~~ [flutter_tool] restructure ProjectFileInvalidator to no longer directly depend on context Dec 12, 2019

christopherfujino reviewed Dec 12, 2019

View reviewed changes

Update project_file_invalidator_test.dart

49d4b0d

zanderso reviewed Dec 12, 2019

View reviewed changes

blasten reviewed Dec 12, 2019

View reviewed changes

jonahwilliams added 2 commits December 12, 2019 15:02

Attempt at creating no context

18cbb98

forward testOn correctly

238f327

zanderso reviewed Dec 13, 2019

View reviewed changes

blasten reviewed Dec 13, 2019

View reviewed changes

jonahwilliams added 2 commits December 14, 2019 13:10

Address comments

5789fb4

use testWithoutContext

60edd19

jonahwilliams removed the work in progress; do not review label Dec 16, 2019

jonahwilliams requested a review from zanderso December 16, 2019 18:46

zanderso reviewed Dec 16, 2019

View reviewed changes

jonahwilliams mentioned this pull request Dec 16, 2019

Reduce usage of context getters and globals in the flutter tool #47161

Open

add doc comment and make test more specific

c97b9a1

zanderso approved these changes Dec 16, 2019

View reviewed changes

Update common_test.dart

332e77c

jonahwilliams merged commit a723c94 into flutter:master Dec 17, 2019

jonahwilliams deleted the switching_watcher_strategy branch December 17, 2019 02:12

github-actions bot locked as resolved and limited conversation to collaborators Aug 2, 2021

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

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

Uh oh!

Conversation

jonahwilliams commented Nov 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

jonahwilliams commented Dec 12, 2019

Uh oh!

christopherfujino Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

christopherfujino commented Dec 12, 2019

Uh oh!

zanderso left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonahwilliams Dec 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jonahwilliams commented Nov 27, 2019 •

edited

Loading

christopherfujino Dec 12, 2019 •

edited

Loading

jonahwilliams Dec 12, 2019 •

edited

Loading