From f31fc1bd0faa9b706468cd4fed58b55e7d8eb509 Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Wed, 12 Jun 2019 23:12:35 -0700 Subject: [PATCH] More removing of timeouts. (#33932) --- .../test_smoke_test/timeout_fail_test.dart | 4 +- .../test_smoke_test/timeout_pass_test.dart | 6 +- packages/flutter_test/lib/src/binding.dart | 112 +++++++++++------- packages/flutter_test/lib/src/matchers.dart | 28 ++--- .../flutter_test/lib/src/widget_tester.dart | 31 +++-- 5 files changed, 104 insertions(+), 77 deletions(-) diff --git a/dev/automated_tests/test_smoke_test/timeout_fail_test.dart b/dev/automated_tests/test_smoke_test/timeout_fail_test.dart index 8cd70be8c6..9738bc9101 100644 --- a/dev/automated_tests/test_smoke_test/timeout_fail_test.dart +++ b/dev/automated_tests/test_smoke_test/timeout_fail_test.dart @@ -7,7 +7,7 @@ import 'package:flutter_test/flutter_test.dart'; void main() { testWidgets('flutter_test timeout logic - addTime - negative', (WidgetTester tester) async { await tester.runAsync(() async { - await Future.delayed(const Duration(milliseconds: 3500)); + await Future.delayed(const Duration(milliseconds: 3500)); // must be more than 1000ms more than the initial timeout }, additionalTime: const Duration(milliseconds: 200)); - }); + }, initialTimeout: const Duration(milliseconds: 2000)); } diff --git a/dev/automated_tests/test_smoke_test/timeout_pass_test.dart b/dev/automated_tests/test_smoke_test/timeout_pass_test.dart index 8aec967b94..83694aec36 100644 --- a/dev/automated_tests/test_smoke_test/timeout_pass_test.dart +++ b/dev/automated_tests/test_smoke_test/timeout_pass_test.dart @@ -7,7 +7,7 @@ import 'package:flutter_test/flutter_test.dart'; void main() { testWidgets('flutter_test timeout logic - addTime - positive', (WidgetTester tester) async { await tester.runAsync(() async { - await Future.delayed(const Duration(milliseconds: 2500)); // must be longer than default timeout. - }, additionalTime: const Duration(milliseconds: 2000)); // default timeout is 2s, so this makes it 4s. - }); + await Future.delayed(const Duration(milliseconds: 2500)); // must be longer than initial timeout below. + }, additionalTime: const Duration(milliseconds: 2000)); // initial timeout is 2s, so this makes it 4s. + }, initialTimeout: const Duration(milliseconds: 2000)); } diff --git a/packages/flutter_test/lib/src/binding.dart b/packages/flutter_test/lib/src/binding.dart index bccd8ac8ea..2b53a0037a 100644 --- a/packages/flutter_test/lib/src/binding.dart +++ b/packages/flutter_test/lib/src/binding.dart @@ -129,9 +129,34 @@ abstract class TestWidgetsFlutterBinding extends BindingBase bool get disableShadows => false; /// Increase the timeout for the current test by the given duration. - void addTime(Duration duration) { - // Noop, see [AutomatedTestWidgetsFlutterBinding. addTime] for an actual implementation. - } + /// + /// This only matters if the test has an `initialTimeout` set on + /// [testWidgets], and the test is running via `flutter test`. By default, + /// tests do not have such a timeout. Tests run using `flutter run` never time + /// out even if one is specified. + /// + /// This method has no effect on the timeout specified via `timeout` on + /// [testWidgets]. That timeout is implemented by the `test` package. + /// + /// By default, each [pump] and [pumpWidget] call increases the timeout by a + /// hundred milliseconds, and each [matchesGoldenFile] expectation increases + /// it by a minute. If there is no timeout in the first place, this has no + /// effect. + /// + /// The granularity of timeouts is coarse: the time is checked once per + /// second, and only when the test is not executing. It is therefore possible + /// for a timeout to be exceeded by hundreds of milliseconds and for the test + /// to still succeed. If precise timing is required, it should be implemented + /// as a part of the test rather than relying on this mechanism. + /// + /// See also: + /// + /// * [testWidgets], on which a timeout can be set using the `timeout` + /// argument. + /// * [defaultTestTimeout], the maximum that the timeout can reach. + /// (That timeout is implemented by the `test` package.) + // See AutomatedTestWidgetsFlutterBinding.addTime for an actual implementation. + void addTime(Duration duration); /// The value to set [debugCheckIntrinsicSizes] to while tests are running. /// @@ -183,11 +208,15 @@ abstract class TestWidgetsFlutterBinding extends BindingBase /// The number of outstanding microtasks in the queue. int get microtaskCount; - /// The default test timeout for tests when using this binding. + /// The default maximum test timeout for tests when using this binding. /// - /// The [AutomatedTestWidgetsFlutterBinding] layers in an additional timeout - /// mechanism beyond this, with much more aggressive timeouts. See - /// [AutomatedTestWidgetsFlutterBinding.addTime]. + /// This controls the default for the `timeout` argument on `testWidgets`. It + /// is 10 minutes for [AutomatedTestWidgetsFlutterBinding] (tests running + /// using `flutter test`), and unlimited for tests using + /// [LiveTestWidgetsFlutterBinding] (tests running using `flutter run`). + /// + /// This is the maximum that the timeout controlled by `initialTimeout` on + /// [testWidgets] can reach when augmented using [addTime]. test_package.Timeout get defaultTestTimeout; /// The current time. @@ -233,8 +262,8 @@ abstract class TestWidgetsFlutterBinding extends BindingBase /// /// The `additionalTime` argument is used by the /// [AutomatedTestWidgetsFlutterBinding] implementation to increase the - /// current timeout. See [AutomatedTestWidgetsFlutterBinding.addTime] for - /// details. The value is ignored by the [LiveTestWidgetsFlutterBinding]. + /// current timeout, if any. See [AutomatedTestWidgetsFlutterBinding.addTime] + /// for details. Future runAsync( Future callback(), { Duration additionalTime = const Duration(milliseconds: 1000), @@ -422,7 +451,10 @@ abstract class TestWidgetsFlutterBinding extends BindingBase /// The `description` is used by the [LiveTestWidgetsFlutterBinding] to /// show a label on the screen during the test. The description comes from /// the value passed to [testWidgets]. It must not be null. - Future runTest(Future testBody(), VoidCallback invariantTester, { String description = '' }); + /// + /// The `timeout` argument sets the initial timeout, if any. It can + /// be increased with [addTime]. By default there is no timeout. + Future runTest(Future testBody(), VoidCallback invariantTester, { String description = '', Duration timeout }); /// This is called during test execution before and after the body has been /// executed. @@ -739,10 +771,8 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { @override bool get checkIntrinsicSizes => true; - // The timeout here is absurdly high because we do our own timeout logic and - // this is just a backstop. @override - test_package.Timeout get defaultTestTimeout => const test_package.Timeout(Duration(minutes: 5)); + test_package.Timeout get defaultTestTimeout => const test_package.Timeout(Duration(minutes: 10)); @override bool get inTest => _currentFakeAsync != null; @@ -933,6 +963,7 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { void _checkTimeout(Timer timer) { assert(_timeoutTimer == timer); + assert(_timeout != null); if (_timeoutStopwatch.elapsed > _timeout) { _timeoutCompleter.completeError( TimeoutException( @@ -944,33 +975,10 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { } } - /// Increase the timeout for the current test by the given duration. - /// - /// Tests by default time out after two seconds, but the timeout can be - /// increased before an expensive operation to allow it to complete without - /// hitting the test timeout. - /// - /// By default, each [pump] and [pumpWidget] call increases the timeout by a - /// hundred milliseconds, and each [matchesGoldenFile] expectation increases - /// it by several seconds. - /// - /// In general, unit tests are expected to run very fast, and this method is - /// usually not necessary. - /// - /// The granularity of timeouts is coarse: the time is checked once per - /// second, and only when the test is not executing. It is therefore possible - /// for a timeout to be exceeded by hundreds of milliseconds and for the test - /// to still succeed. If precise timing is required, it should be implemented - /// as a part of the test rather than relying on this mechanism. - /// - /// See also: - /// - /// * [defaultTestTimeout], the maximum that the timeout can reach. - /// (That timeout is implemented by the test package.) @override void addTime(Duration duration) { - assert(_timeout != null, 'addTime can only be called during a test.'); - _timeout += duration; + if (_timeout != null) + _timeout += duration; } @override @@ -978,7 +986,7 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { Future testBody(), VoidCallback invariantTester, { String description = '', - Duration timeout = const Duration(seconds: 2), + Duration timeout, }) { assert(description != null); assert(!inTest); @@ -986,9 +994,11 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { assert(_clock == null); _timeout = timeout; - _timeoutStopwatch = Stopwatch()..start(); - _timeoutTimer = Timer.periodic(const Duration(seconds: 1), _checkTimeout); - _timeoutCompleter = Completer(); + if (_timeout != null) { + _timeoutStopwatch = Stopwatch()..start(); + _timeoutTimer = Timer.periodic(const Duration(seconds: 1), _checkTimeout); + _timeoutCompleter = Completer(); + } final FakeAsync fakeAsync = FakeAsync(); _currentFakeAsync = fakeAsync; // reset in postTest @@ -997,7 +1007,7 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { fakeAsync.run((FakeAsync localFakeAsync) { assert(fakeAsync == _currentFakeAsync); assert(fakeAsync == localFakeAsync); - testBodyResult = _runTest(testBody, invariantTester, description, timeout: _timeoutCompleter.future); + testBodyResult = _runTest(testBody, invariantTester, description, timeout: _timeoutCompleter?.future); assert(inTest); }); @@ -1051,7 +1061,7 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { _clock = null; _currentFakeAsync = null; _timeoutCompleter = null; - _timeoutTimer.cancel(); + _timeoutTimer?.cancel(); _timeoutTimer = null; _timeoutStopwatch = null; _timeout = null; @@ -1203,6 +1213,12 @@ class LiveTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { /// ``` LiveTestWidgetsFlutterBindingFramePolicy framePolicy = LiveTestWidgetsFlutterBindingFramePolicy.fadePointers; + @override + void addTime(Duration duration) { + // We don't support timeouts on the LiveTestWidgetsFlutterBinding. + // See runTest(). + } + @override void scheduleFrame() { if (framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.benchmark) @@ -1341,6 +1357,8 @@ class LiveTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { ); }()); + addTime(additionalTime); // doesn't do anything since we don't actually track the timeout, but just for correctness... + _runningAsyncTasks = true; try { return await callback(); @@ -1358,11 +1376,15 @@ class LiveTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { } @override - Future runTest(Future testBody(), VoidCallback invariantTester, { String description = '' }) async { + Future runTest(Future testBody(), VoidCallback invariantTester, { String description = '', Duration timeout }) async { assert(description != null); assert(!inTest); _inTest = true; renderView._setDescription(description); + // We drop the timeout on the floor in `flutter run` mode. + // We could support it, but we'd have to automatically add the entire duration of pumps + // and timers and so on, since those operate in real time when using this binding, but + // the timeouts expect them to happen near-instantaneously. return _runTest(testBody, invariantTester, description); } diff --git a/packages/flutter_test/lib/src/matchers.dart b/packages/flutter_test/lib/src/matchers.dart index d635041740..586cc80e34 100644 --- a/packages/flutter_test/lib/src/matchers.dart +++ b/packages/flutter_test/lib/src/matchers.dart @@ -1646,28 +1646,23 @@ class _MatchesReferenceImage extends AsyncMatcher { final TestWidgetsFlutterBinding binding = TestWidgetsFlutterBinding.ensureInitialized(); return binding.runAsync(() async { final ui.Image image = await imageFuture; - final ByteData bytes = await image.toByteData() - .timeout(const Duration(seconds: 10), onTimeout: () => null); - if (bytes == null) { - return 'Failed to generate an image from engine within the 10,000ms timeout.'; - } + final ByteData bytes = await image.toByteData(); + if (bytes == null) + return 'could not be encoded.'; - final ByteData referenceBytes = await referenceImage.toByteData() - .timeout(const Duration(seconds: 10), onTimeout: () => null); - if (referenceBytes == null) { - return 'Failed to generate an image from engine within the 10,000ms timeout.'; - } + final ByteData referenceBytes = await referenceImage.toByteData(); + if (referenceBytes == null) + return 'could not have its reference image encoded.'; - if (referenceImage.height != image.height || referenceImage.width != image.width) { + if (referenceImage.height != image.height || referenceImage.width != image.width) return 'does not match as width or height do not match. $image != $referenceImage'; - } final int countDifferentPixels = _countDifferentPixels( Uint8List.view(bytes.buffer), Uint8List.view(referenceBytes.buffer), ); return countDifferentPixels == 0 ? null : 'does not match on $countDifferentPixels pixels'; - }, additionalTime: const Duration(seconds: 21)); + }, additionalTime: const Duration(minutes: 1)); } @override @@ -1704,10 +1699,9 @@ class _MatchesGoldenFile extends AsyncMatcher { final TestWidgetsFlutterBinding binding = TestWidgetsFlutterBinding.ensureInitialized(); return binding.runAsync(() async { final ui.Image image = await imageFuture; - final ByteData bytes = await image.toByteData(format: ui.ImageByteFormat.png) - .timeout(const Duration(seconds: 10), onTimeout: () => null); + final ByteData bytes = await image.toByteData(format: ui.ImageByteFormat.png); if (bytes == null) - return 'Failed to generate screenshot from engine within the 10,000ms timeout.'; + return 'could not encode screenshot.'; if (autoUpdateGoldenFiles) { await goldenFileComparator.update(key, bytes.buffer.asUint8List()); return null; @@ -1718,7 +1712,7 @@ class _MatchesGoldenFile extends AsyncMatcher { } on TestFailure catch (ex) { return ex.message; } - }, additionalTime: const Duration(seconds: 11)); + }, additionalTime: const Duration(minutes: 1)); } @override diff --git a/packages/flutter_test/lib/src/widget_tester.dart b/packages/flutter_test/lib/src/widget_tester.dart index 0000f91131..b0054360c3 100644 --- a/packages/flutter_test/lib/src/widget_tester.dart +++ b/packages/flutter_test/lib/src/widget_tester.dart @@ -49,14 +49,24 @@ typedef WidgetTesterCallback = Future Function(WidgetTester widgetTester); /// /// The callback can be asynchronous (using `async`/`await` or /// using explicit [Future]s). -/// Tests using the [AutomatedTestWidgetsFlutterBinding] -/// have a default time out of two seconds, -/// which is automatically increased for some expensive operations, -/// and can also be manually increased by calling -/// [AutomatedTestWidgetsFlutterBinding.addTime]. -/// The maximum that this timeout can reach (automatically or manually increased) -/// is defined by the timeout property, -/// which defaults to [TestWidgetsFlutterBinding.defaultTestTimeout]. +/// +/// There are two kinds of timeouts that can be specified. The `timeout` +/// argument specifies the backstop timeout implemented by the `test` package. +/// If set, it should be relatively large (minutes). It defaults to ten minutes +/// for tests run by `flutter test`, and is unlimited for tests run by `flutter +/// run`; specifically, it defaults to +/// [TestWidgetsFlutterBinding.defaultTestTimeout]. +/// +/// The `initialTimeout` argument specifies the timeout implemented by the +/// `flutter_test` package itself. If set, it may be relatively small (seconds), +/// as it is automatically increased for some expensive operations, and can also +/// be manually increased by calling +/// [AutomatedTestWidgetsFlutterBinding.addTime]. The effective maximum value of +/// this timeout (even after calling `addTime`) is the one specified by the +/// `timeout` argument. +/// +/// In general, timeouts are race conditions and cause flakes, so best practice +/// is to avoid the use of timeouts in tests. /// /// If the `enableSemantics` parameter is set to `true`, /// [WidgetTester.ensureSemantics] will have been called before the tester is @@ -89,11 +99,11 @@ void testWidgets( WidgetTesterCallback callback, { bool skip = false, test_package.Timeout timeout, + Duration initialTimeout, bool semanticsEnabled = false, }) { final TestWidgetsFlutterBinding binding = TestWidgetsFlutterBinding.ensureInitialized(); final WidgetTester tester = WidgetTester._(binding); - timeout ??= binding.defaultTestTimeout; test( description, () { @@ -110,10 +120,11 @@ void testWidgets( }, tester._endOfTestVerifications, description: description ?? '', + timeout: initialTimeout, ); }, skip: skip, - timeout: timeout, + timeout: timeout ?? binding.defaultTestTimeout, ); }