Skip to content

[flutter_tools] Decouple FlutterPlatform from Process #74236

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 3 commits into from
Feb 17, 2021
Merged

[flutter_tools] Decouple FlutterPlatform from Process #74236

merged 3 commits into from
Feb 17, 2021

Conversation

jiahaog
Copy link
Member

@jiahaog jiahaog commented Jan 19, 2021 •
edited
Loading

Defines a TestDevice with the interface required for running tests on the device. Refactors the existing implementation (with the Process based flutter tester) into an implementation of TestDevice.

Smaller changes:

  • Remove port from installHook, this isn't used here and in Google3
  • Changes the watcher API to deal with TestDevice instead of Process
  • Removes childindex from the watcher as it's only used for logging
  • Minor changes to trace messages
  • Some tests were moved from flutter_platform_test.dart to flutter_tester_device_test.dart to reflect the abstraction changes
  • Introduces a FontConfigManager to manage the shared state of the font config files across multiple tests
  • Updates the exitCode check to not try and match the terminating signal on Windows. Matching of the terminating signal only occurs on macOS and Linux, and this mismatch is currently hidden today because controllerSinkClosed is always false when the process is terminated.

Manually Tested:

  • flutter test
  • flutter test --coverage
  • IntelliJ & VSCode running and debugging of tests

Related:

@flutter-dashboard flutter-dashboard bot added the tool Affects the "flutter" command-line tool. See also t: labels. label Jan 19, 2021
@google-cla google-cla bot added the cla: yes label Jan 19, 2021
@jiahaog jiahaog changed the title Simplify test decouple process Decouple FlutterPlatform from Process Jan 19, 2021
@jiahaog jiahaog force-pushed the simplify-test-decouple-process branch from cb48a44 to 3357ba8 Compare January 19, 2021 12:18
@jiahaog jiahaog mentioned this pull request Jan 19, 2021
Merged
@jiahaog jiahaog force-pushed the simplify-test-decouple-process branch from 3357ba8 to f0398b8 Compare January 20, 2021 04:41
@jiahaog
Copy link
Member Author

jiahaog commented Jan 20, 2021

I just noticed the existence of FlutterTesterDevice after making this refactor... It doesn't seem like FlutterTesterDevice is used at all for flutter test, but perhaps another approach would be to refactor FlutterPlatform to use Device to run tests, with FlutterTesterDevice as the default implementation, then we can teach the test command to use other Device implementations for integration tests. @jonahwilliams please take a look and let me know what you think.

@jonahwilliams
Copy link
Contributor

The way in which the tester runs tests is a bit different from the Device - the device itself is expected to handle packaging and does so in a very non-optimal way for tests. I think it could be fixed but I don't know if it would be a better idea.

@jiahaog jiahaog mentioned this pull request Jan 25, 2021
Merged
@jiahaog jiahaog force-pushed the simplify-test-decouple-process branch from f0398b8 to 8d8c1f8 Compare January 29, 2021 01:19
@jiahaog jiahaog force-pushed the simplify-test-decouple-process branch 2 times, most recently from 5f0002d to 9773958 Compare February 8, 2021 03:25
@jiahaog jiahaog changed the title Decouple FlutterPlatform from Process [flutter_tools] Decouple FlutterPlatform from Process Feb 8, 2021
@jiahaog jiahaog force-pushed the simplify-test-decouple-process branch 3 times, most recently from a4fd7ab to 3b1c146 Compare February 8, 2021 06:30
@jiahaog jiahaog force-pushed the simplify-test-decouple-process branch from 3b1c146 to cbb1971 Compare February 8, 2021 11:46
@jiahaog jiahaog marked this pull request as ready for review February 8, 2021 13:00
@jiahaog jiahaog requested a review from jonahwilliams February 8, 2021 13:41
jonahwilliams
jonahwilliams reviewed Feb 11, 2021
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really like this approach.

While you're refactoring this code, could you also remove the usage of the globals library where possible? For example, taking a Logger in the constructor instead of of calling globals.printStatus

packages/flutter_tools/lib/src/test/flutter_tester_device.dart Outdated
.transform<String>(utf8.decoder)
.transform<String>(const LineSplitter())
.listen(
(String line) async {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it possible to re-use the existing ObservatoryDiscovery class here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible but the pieces don't quite fit together, not sure if it's worth doing now. ProtocolDiscovery.observatory requires a DeviceLogReader, but we don't quite have that here. I'd say doing that would be easier when we eventually (?) refactor to use the FlutterTesterDevice abstraction here, where pieces like the log reader are already created for us.

packages/flutter_tools/test/general.shard/flutter_tester_device_test.dart Outdated
Stream<List<int>> get stderr => const Stream<List<int>>.empty();
}

class MockPlatform extends Mock implements Platform {}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FakePlatform should be used instead of MockPlatform generally, should be just as easy to use.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this was done in #75757, updated the branch to match. I left the MockProcess as it is, instead of using FakeProcess, as what we have in-place today is more suitable for performing assertions on the environment passed. IIUC FakeProcess will stall and eventually timeout the test when the environment doesn't match, which is harder to debug when something goes wrong as compared to what we have currently.

@jiahaog
Copy link
Member Author

jiahaog commented Feb 16, 2021

could you also remove the usage of the globals library where possible?

I did this for flutter_tester_device.dart, but continued to pass in the globals in flutter_platform.dart. Removing the globals here would probably involve passing them down from the commands/test.dart, which can be done separately.

jonahwilliams
jonahwilliams approved these changes Feb 16, 2021
View reviewed changes
Copy link
Contributor

@jonahwilliams jonahwilliams left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@jiahaog jiahaog mentioned this pull request Feb 17, 2021
Merged
@fluttergithubbot fluttergithubbot merged commit 9e55af5 into flutter:master Feb 17, 2021
@jiahaog jiahaog deleted the simplify-test-decouple-process branch February 17, 2021 22:18
@jmagman jmagman mentioned this pull request Feb 18, 2021
Merged
@dnfield dnfield mentioned this pull request Mar 31, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@jonahwilliams jonahwilliams jonahwilliams approved these changes

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
tool Affects the "flutter" command-line tool. See also t: labels.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jiahaog @jonahwilliams @fluttergithubbot