[flutter_tools] Decouple FlutterPlatform from Process #74236
Conversation
flutter-dashboard
bot
added
the
tool
jiahaog
changed the title
jiahaog
force-pushed
the
simplify-test-decouple-process
branch
from
cb48a44 to
3357ba8
Compare
jiahaog
force-pushed
the
simplify-test-decouple-process
branch
from
3357ba8 to
f0398b8
Compare
|
I just noticed the existence of |
|
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
force-pushed
the
simplify-test-decouple-process
branch
from
f0398b8 to
8d8c1f8
Compare
jiahaog
force-pushed
the
simplify-test-decouple-process
branch
2 times, most recently
from
5f0002d to
9773958
Compare
jiahaog
changed the title
jiahaog
force-pushed
the
simplify-test-decouple-process
branch
3 times, most recently
from
a4fd7ab to
3b1c146
Compare
jiahaog
force-pushed
the
simplify-test-decouple-process
branch
from
3b1c146 to
cbb1971
Compare
jonahwilliams
left a comment
jonahwilliams
left a comment
There was a problem hiding this comment.
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
| .transform<String>(utf8.decoder) | ||
| .transform<String>(const LineSplitter()) | ||
| .listen( | ||
| (String line) async { |
There was a problem hiding this comment.
Is it possible to re-use the existing ObservatoryDiscovery class here?
There was a problem hiding this comment.
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.
| Stream<List<int>> get stderr => const Stream<List<int>>.empty(); | ||
| } | ||
|
|
||
| class MockPlatform extends Mock implements Platform {} |
There was a problem hiding this comment.
FakePlatform should be used instead of MockPlatform generally, should be just as easy to use.
There was a problem hiding this comment.
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.
I did this for |
3 participants
Defines a
TestDevicewith the interface required for running tests on the device. Refactors the existing implementation (with theProcessbased flutter tester) into an implementation ofTestDevice.Smaller changes:
portfrominstallHook, this isn't used here and in Google3TestDeviceinstead ofProcesschildindexfrom the watcher as it's only used for loggingflutter_platform_test.darttoflutter_tester_device_test.dartto reflect the abstraction changesFontConfigManagerto manage the shared state of the font config files across multiple testscontrollerSinkClosedis always false when the process is terminated.Manually Tested:
flutter testflutter test --coverageRelated: