-
Notifications
You must be signed in to change notification settings - Fork 28.9k
Description
There are 200 instances in 53 files where tests in flutter_tools
uses processManager.run
, or some variant of that command, which runs an external process, effectively blocks (sometimes using async, but still halts execution from a test perspective), and then, upon completion, checks if the result was successful.
For example, something like this:
test('flutter does great things', () async {
ProcessResult result = processManager.run(flutterBin, ['create', 'foo']);
expect(result, const ProcessResultMatcher());
result = processManager.run(flutterBin, ['build', 'apk']);
expect(result, const ProcessResultMatcher());
});
There is an inherit problem with this pattern and timeouts. The flutter_tools
package insists that no test take longer than 15 minutes, and there are overall timeout commitments at the LUCI and CI level (often between 30m and 60m).
In practical terms, that means that if any processManager.run
goes over either a test or task timeout, the output will appear befuddlingly incomplete, and it will be unclear from a triage perspective if the test shard needs re-sharding, the particular test is just taking too long, or if there is an underlying failure, such as a OOM or crash which does not cause the process to complete as expected.
One such example is #158560:
15:26 +87: test/integration.shard/isolated/native_assets_without_cbuild_assemble_test.dart: (setUpAll)
15:26 +87: test/integration.shard/isolated/native_assets_without_cbuild_assemble_test.dart: flutter build "linux" succeeds without libraries
15:51 +88: test/integration.shard/isolated/native_assets_without_cbuild_assemble_test.dart: flutter build "apk" succeeds without libraries
30:54 +88 -1: test/integration.shard/isolated/native_assets_without_cbuild_assemble_test.dart: flutter build "apk" succeeds without libraries [E]
TimeoutException after 0:15:00.000000: Test timed out after 15 minutes.
dart:isolate _RawReceivePort._handleMessage
... there is no way to know what flutter build apk
was doing (was it making progress? did it crash?).
I made a 1-off adjustment in #158757, but I'd suggest:
- Integration tests in the tool should always stream process output
- There should be a top-level function for doing so, that does it the right way
- A test should assert that integration tests do not call processManager.run directly