Some test cleanup for flutter_tools. (#90227)

dev/bots/test.dart

@@ -278,6 +278,8 @@ Future<void> _runGeneralToolTests() async {
     testPaths: <String>[path.join('test', 'general.shard')],
     enableFlutterToolAsserts: false,
     // Detect unit test time regressions (poor time delay handling, etc).
+    // This overrides the 15 minute default for tools tests.
+    // See the README.md and dart_test.yaml files in the flutter_tools package.
     perTestTimeout: const Duration(seconds: 2),
   );
 }
@@ -295,8 +297,6 @@ Future<void> _runWebToolTests() async {
     path.join(flutterRoot, 'packages', 'flutter_tools'),
     forceSingleCore: true,
     testPaths: <String>[path.join('test', 'web.shard')],
-    enableFlutterToolAsserts: true,
-    perTestTimeout: const Duration(minutes: 3),
     includeLocalEngineEnv: true,
   );
 }

packages/flutter_tools/README.md

@@ -39,17 +39,30 @@ $ flutter analyze
 As with other parts of the Flutter repository, all changes in behavior [must be
 tested](https://github.com/flutter/flutter/wiki/Style-guide-for-Flutter-repo#write-test-find-bug).
 Tests live under the `test/` subdirectory.
-- Hermetic unit tests of tool internals go under `test/general.shard`.
-- Tests of tool commands go under `test/commands.shard`. Hermetic tests go under
-  its `hermetic/` subdirectory. Non-hermetic tests go under its `permeable`
-  sub-directory. Avoid adding tests here and prefer writing either a unit test or a full
-  integration test.
-- Integration tests (e.g. tests that run the tool in a subprocess) go under
-  `test/integration.shard`.
 
-In general, the tests for the code in a file called `file.dart` should go in a
+- Hermetic unit tests of tool internals go under `test/general.shard`
-file called `file_test.dart` in the subdirectory that matches the behavior of
+  and must run in significantly less than two seconds.
-the test.
+
+- Tests of tool commands go under `test/commands.shard`. Hermetic
+  tests go under its `hermetic/` subdirectory. Non-hermetic tests go
+  under its `permeable` sub-directory. Avoid adding tests here and
+  prefer writing either a unit test or a full integration test.
+
+- Integration tests (e.g. tests that run the tool in a subprocess) go
+  under `test/integration.shard`.
+
+- Slow web-related tests go in the `test/web.shard` directory.
+
+In general, the tests for the code in a file called `file.dart` should
+go in a file called `file_test.dart` in the subdirectory that matches
+the behavior of the test.
+
+The `dart_test.yaml` file configures the timeout for these tests to be
+15 minutes. The `test.dart` script that is used in CI overrides this
+to two seconds for the `test/general.shard` directory, to catch
+behaviour that is unexpectedly slow.
+
+Please avoid setting any other timeouts.
 
 #### Using local engine builds in integration tests
 
@@ -73,29 +86,26 @@ To run all of the unit tests:
 $ flutter test test/general.shard
 ```
 
-The tests in `test/integration.shard` are slower to run than the tests in
+The tests in `test/integration.shard` are slower to run than the tests
-`test/general.shard`. Depending on your development computer, you might
+in `test/general.shard`. Depending on your development computer, you
-want to increase timeouts and limit concurrency. Generally it is easier to run
+might want to limit concurrency. Generally it is easier to run these
-these on CI, or to manually verify the behavior you are changing instead of running
+on CI, or to manually verify the behavior you are changing instead of
-the test.
+running the test.
 
-The integration tests also require the `FLUTTER_ROOT` environment variable
+The integration tests also require the `FLUTTER_ROOT` environment
-to be set. The full invocation to run everything might therefore look something like:
+variable to be set. The full invocation to run everything might
+therefore look something like:
 
 ```shell
 $ FLUTTER_ROOT=~/path/to/flutter-sdk
-$ flutter test --timeout 2x --concurrency 1
+$ flutter test --concurrency 1
 ```
 
-This will take about an hour to complete.
+This may take some time (on the order of an hour). The unit tests
+alone take much less time (on the order of a minute).
 
-To run only the tests in `test/general.shard` (which takes about a minute),
+You can run the tests in a specific file, e.g.:
-in this directory run:
-```shell
-$ flutter test test/general.shard
-```
 
-To run the tests in a specific file, run:
 ```shell
 $ flutter test test/general.shard/utils_test.dart
 ```

packages/flutter_tools/dart_test.yaml

@@ -6,6 +6,9 @@
 # overloaded. For this reason, we set the test timeout to
 # significantly more than it would be by default, and we never set the
 # timeouts in the tests themselves.
+#
+# For the `test/general.shard` specifically, the `dev/bots/test.dart` script
+# overrides this, reducing it to only 2000ms. Unit tests must run fast!
 timeout: 15m
 
 tags:

packages/flutter_tools/test/integration.shard/deferred_components_test.dart

@@ -62,7 +62,7 @@ void main() {
     expect(archive.findFile('base/assets/flutter_assets/test_assets/asset1.txt') != null, true);
 
     expect(result.exitCode, 0);
-  }, timeout: const Timeout(Duration(minutes: 5)));
+  });
 
   testWithoutContext('simple build appbundle all targets succeeds', () async {
     final DeferredComponentsProject project = DeferredComponentsProject(BasicDeferredComponentsConfig());
@@ -105,7 +105,7 @@ void main() {
     expect(archive.findFile('base/assets/flutter_assets/test_assets/asset1.txt') != null, true);
 
     expect(result.exitCode, 0);
-  }, timeout: const Timeout(Duration(minutes: 5)));
+  });
 
   testWithoutContext('simple build appbundle no-deferred-components succeeds', () async {
     final DeferredComponentsProject project = DeferredComponentsProject(BasicDeferredComponentsConfig());
@@ -151,7 +151,7 @@ void main() {
     expect(archive.findFile('base/assets/flutter_assets/test_assets/asset1.txt') != null, true);
 
     expect(result.exitCode, 0);
-  }, timeout: const Timeout(Duration(minutes: 5)));
+  });
 
   testWithoutContext('simple build appbundle mismatched golden no-validate-deferred-components succeeds', () async {
     final DeferredComponentsProject project = DeferredComponentsProject(MismatchedGoldenDeferredComponentsConfig());
@@ -198,7 +198,7 @@ void main() {
     expect(archive.findFile('base/assets/flutter_assets/test_assets/asset1.txt') != null, true);
 
     expect(result.exitCode, 0);
-  }, timeout: const Timeout(Duration(minutes: 5)));
+  });
 
   testWithoutContext('simple build appbundle missing android dynamic feature module fails', () async {
     final DeferredComponentsProject project = DeferredComponentsProject(NoAndroidDynamicFeatureModuleDeferredComponentsConfig());

packages/flutter_tools/test/integration.shard/flutter_attach_test.dart

@@ -104,5 +104,5 @@ void main() {
       continuePollingValue: '',
       matches: equals(vmServiceUri),
     );
-  }, timeout: const Timeout.factor(4));
+  });
 }

packages/flutter_tools/test/integration.shard/flutter_build_null_unsafe_test.dart

@@ -78,6 +78,6 @@ String unsafeString = null;
         if (targetPlatform == 'ios') '--no-codesign',
       ], workingDirectory: projectRoot.path);
       expect(result.exitCode, 0);
-    }, timeout: const Timeout(Duration(minutes: 3)));
+    });
   }
 }

packages/flutter_tools/test/integration.shard/flutter_build_with_compilation_error_test.dart

@@ -71,6 +71,6 @@ int x = 'String';
         contains("A value of type 'String' can't be assigned to a variable of type 'int'."),
       );
       expect(result.exitCode, 1);
-    }, timeout: const Timeout(Duration(minutes: 3)));
+    });
   }
 }

packages/flutter_tools/test/integration.shard/flutter_run_test.dart

@@ -69,5 +69,5 @@ void main() {
       continuePollingValue: '',
       matches: isNotEmpty,
     );
-  }, timeout: const Timeout.factor(4));
+  });
 }

packages/flutter_tools/test/integration.shard/gradle_non_android_plugin_test.dart

@@ -90,5 +90,5 @@ void main() {
       'app-release.apk',
     );
     expect(fileSystem.file(exampleAppApk), exists);
-  }, timeout: const Timeout(Duration(minutes: 5)));
+  });
 }

packages/flutter_tools/test/integration.shard/macos_content_validation_test.dart

@@ -145,8 +145,6 @@ void main() {
         ...getLocalEngineArguments(),
         'clean',
       ], workingDirectory: workingDirectory);
-    }, skip: !platform.isMacOS, // [intended] only makes sense for macos platform.
+    }, skip: !platform.isMacOS); // [intended] only makes sense for macos platform.
-       timeout: const Timeout(Duration(minutes: 5)),
-    );
   }
 }

packages/flutter_tools/test/integration.shard/overall_experience_test.dart

@@ -25,10 +25,7 @@
 // @dart = 2.8
 // This file is ready to transition, just uncomment /*?*/, /*!*/, and /*late*/.
 
-// TODO(gspencergoog): Remove this tag once this test's state leaks/test
+// This file intentionally assumes the tests run in order.
-// dependencies have been fixed.
-// https://github.com/flutter/flutter/issues/85160
-// Fails with "flutter test --test-randomize-ordering-seed=1000"
 @Tags(<String>['no-shuffle'])
 
 import 'dart:async';
@@ -40,7 +37,7 @@ import 'package:pedantic/pedantic.dart';
 import 'package:process/process.dart';
 
 import '../src/common.dart';
-import 'test_utils.dart' show fileSystem, platform;
+import 'test_utils.dart' show fileSystem;
 
 const ProcessManager processManager = LocalProcessManager();
 final String flutterRoot = getFlutterRoot();
@@ -189,7 +186,7 @@ Future<ProcessTestResult> runFlutter(
   List<Transition> transitions, {
   bool debug = false,
   bool logging = true,
-  Duration expectedMaxDuration = const Duration(minutes: 10), // must be less than test timeout of 15 minutes!
+  Duration expectedMaxDuration = const Duration(minutes: 10), // must be less than test timeout of 15 minutes! See ../../dart_test.yaml.
 }) async {
   final Stopwatch clock = Stopwatch()..start();
   final Process process = await processManager.start(
@@ -358,7 +355,7 @@ void main() {
     } finally {
       tryToDelete(fileSystem.directory(tempDirectory));
     }
-  }, skip: platform.isWindows); // https://github.com/flutter/flutter/issues/87924
+  }, skip: Platform.isWindows); // [intended] Windows doesn't support sending signals so we don't care if it can store the PID.
 
   testWithoutContext('flutter run handle SIGUSR1/2', () async {
     final String tempDirectory = fileSystem.systemTempDirectory.createTempSync('flutter_overall_experience_test.').resolveSymbolicLinksSync();
@@ -622,5 +619,5 @@ void main() {
       '',
       'Application finished.',
     ]);
-  }, skip: Platform.isWindows); // TODO(zanderso): Re-enable when this test is reliable on device lab, https://github.com/flutter/flutter/issues/81556
+  });
 }

packages/flutter_tools/test/src/common.dart

@@ -148,7 +148,6 @@ Matcher containsIgnoringWhitespace(String toSearch) {
 @isTest
 void test(String description, FutureOr<void> Function() body, {
   String? testOn,
-  Timeout? timeout,
   dynamic skip,
   List<String>? tags,
   Map<String, dynamic>? onPlatform,
@@ -162,12 +161,14 @@ void test(String description, FutureOr<void> Function() body, {
       });
       return body();
     },
-    timeout: timeout,
     skip: skip,
     tags: tags,
     onPlatform: onPlatform,
     retry: retry,
     testOn: testOn,
+    // We don't support "timeout"; see ../../dart_test.yaml which
+    // configures all tests to have a 15 minute timeout which should
+    // definitely be enough.
   );
 }
 
@@ -182,7 +183,6 @@ void test(String description, FutureOr<void> Function() body, {
 @isTest
 void testWithoutContext(String description, FutureOr<void> Function() body, {
   String? testOn,
-  Timeout? timeout,
   dynamic skip,
   List<String>? tags,
   Map<String, dynamic>? onPlatform,
@@ -194,12 +194,14 @@ void testWithoutContext(String description, FutureOr<void> Function() body, {
         contextKey: const _NoContext(),
       });
     },
-    timeout: timeout,
     skip: skip,
     tags: tags,
     onPlatform: onPlatform,
     retry: retry,
     testOn: testOn,
+    // We don't support "timeout"; see ../../dart_test.yaml which
+    // configures all tests to have a 15 minute timeout which should
+    // definitely be enough.
   );
 }
 

packages/flutter_tools/test/src/context.dart

@@ -62,7 +62,6 @@ void testUsingContext(
   Map<Type, Generator> overrides = const <Type, Generator>{},
   bool initializeFlutterRoot = true,
   String testOn,
-  Timeout timeout,
   bool skip, // should default to `false`, but https://github.com/dart-lang/test/issues/545 doesn't allow this
 }) {
   if (overrides[FileSystem] != null && overrides[ProcessManager] == null) {
@@ -167,7 +166,10 @@ void testUsingContext(
       // BotDetector implementation in the overrides.
       BotDetector: overrides[BotDetector] ?? () => const FakeBotDetector(true),
     });
-  }, testOn: testOn, skip: skip, timeout: timeout);
+  }, testOn: testOn, skip: skip);
+  // We don't support "timeout"; see ../../dart_test.yaml which
+  // configures all tests to have a 15 minute timeout which should
+  // definitely be enough.
 }
 
 void _printBufferedErrors(AppContext testContext) {

packages/flutter_tools/test/web.shard/chrome_test.dart

@@ -12,9 +12,9 @@ import 'package:flutter_tools/src/base/os.dart';
 import 'package:flutter_tools/src/base/platform.dart';
 import 'package:flutter_tools/src/web/chrome.dart';
 
-import '../../src/common.dart';
+import '../src/common.dart';
-import '../../src/fake_process_manager.dart';
+import '../src/fake_process_manager.dart';
-import '../../src/fakes.dart';
+import '../src/fakes.dart';
 
 const List<String> kChromeArgs = <String>[
   '--disable-background-timer-throttling',
@@ -547,7 +547,7 @@ void main() {
       contains('Unable to connect to Chrome debug port:'),
     );
     expect(logger.errorText, contains('SocketException'));
-  }, timeout: const Timeout.factor(2));
+  });
 }
 
 Future<Chromium> _testLaunchChrome(String userDataDir, FakeProcessManager processManager, ChromiumLauncher chromeLauncher) {