From b75564155966cc4bf28417772362d281137f2d9e Mon Sep 17 00:00:00 2001 From: John McDole Date: Thu, 12 Sep 2024 16:19:15 -0700 Subject: [PATCH] Address frame policy benchmark flakes (#155130) Recently the microbenchmarks were flakey, but from an older bug. Turns out, `LiveTestWidgetsFlutterBindingFramePolicy` is defaulted to `fadePointers` with this fun note: > This can result in additional frames being pumped beyond those that the test itself requests, which can cause differences in behavior Both `text_intrinsic_bench` and `build_bench` use a similar pattern: * Load stocks app * Open the menu * Switch to `benchmark` frame policy What happens, rarely, is that `LiveTestWidgetsFlutterBinding.pumpBenchmark()` will call (async) `handleBeginFrame` and `handleDrawFrame`. `handleDrawFrame` juggles a tri-state boolean (null, false, true). This boolean is only reset to `null` when handleDrawFrame is called back to back, say, from an extra frame that was scheduled. 1. Switch tri-state boolean to an enum, its easier to read 2. remove asserts that compile away in benchmarks (`--profile`) 3. use `Error.throwWithStackTrace` to keep stack traces. I've been running this test on device lab hardware for hundreds of runs and have not hit a failure yet. Fixes #150542 Fixes #150543 - throw stack! --- .../lib/layout/text_intrinsic_bench.dart | 2 +- .../lib/stocks/build_bench.dart | 2 +- packages/flutter_test/lib/src/binding.dart | 24 +++++++++++++------ .../flutter_test/lib/src/widget_tester.dart | 8 +++++-- 4 files changed, 25 insertions(+), 11 deletions(-) diff --git a/dev/benchmarks/microbenchmarks/lib/layout/text_intrinsic_bench.dart b/dev/benchmarks/microbenchmarks/lib/layout/text_intrinsic_bench.dart index b43bea0b1a..32f3176b13 100644 --- a/dev/benchmarks/microbenchmarks/lib/layout/text_intrinsic_bench.dart +++ b/dev/benchmarks/microbenchmarks/lib/layout/text_intrinsic_bench.dart @@ -33,7 +33,7 @@ Future execute() async { await benchmarkWidgets((WidgetTester tester) async { runApp(intrinsicTextHeight); // Wait for the UI to stabilize. - await tester.pump(const Duration(seconds: 1)); + await tester.pumpAndSettle(const Duration(seconds: 1)); final TestViewConfiguration big = TestViewConfiguration.fromView( size: const Size(360.0, 640.0), diff --git a/dev/benchmarks/microbenchmarks/lib/stocks/build_bench.dart b/dev/benchmarks/microbenchmarks/lib/stocks/build_bench.dart index 33eb5bd1d1..d8224411ec 100644 --- a/dev/benchmarks/microbenchmarks/lib/stocks/build_bench.dart +++ b/dev/benchmarks/microbenchmarks/lib/stocks/build_bench.dart @@ -29,7 +29,7 @@ Future> runBuildBenchmark() async { await tester.pump(const Duration(seconds: 1)); // Complete startup animation await tester.tapAt(const Offset(20.0, 40.0)); // Open drawer await tester.pump(); // Start drawer animation - await tester.pump(const Duration(seconds: 1)); // Complete drawer animation + await tester.pumpAndSettle(const Duration(seconds: 1)); // Complete drawer animation final Element appState = tester.element(find.byType(stocks.StocksApp)); binding.framePolicy = LiveTestWidgetsFlutterBindingFramePolicy.benchmark; diff --git a/packages/flutter_test/lib/src/binding.dart b/packages/flutter_test/lib/src/binding.dart index d15e61f916..d763c15329 100644 --- a/packages/flutter_test/lib/src/binding.dart +++ b/packages/flutter_test/lib/src/binding.dart @@ -1649,6 +1649,12 @@ enum LiveTestWidgetsFlutterBindingFramePolicy { benchmarkLive, } +enum _HandleDrawFrame { + reset, + drawFrame, + skipFrame, +} + /// A variant of [TestWidgetsFlutterBinding] for executing tests /// on a device, typically via `flutter run`, or via integration tests. /// This is intended to allow interactive test development. @@ -1779,31 +1785,35 @@ class LiveTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { return super.reassembleApplication(); } - bool? _doDrawThisFrame; + _HandleDrawFrame _drawFrame = _HandleDrawFrame.reset; @override void handleBeginFrame(Duration? rawTimeStamp) { - assert(_doDrawThisFrame == null); + if (_drawFrame != _HandleDrawFrame.reset) { + throw StateError('handleBeginFrame() called before previous handleDrawFrame()'); + } if (_expectingFrame || _expectingFrameToReassemble || (framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.fullyLive) || (framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.benchmarkLive) || (framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.benchmark) || (framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.fadePointers && _viewNeedsPaint)) { - _doDrawThisFrame = true; + _drawFrame = _HandleDrawFrame.drawFrame; super.handleBeginFrame(rawTimeStamp); } else { - _doDrawThisFrame = false; + _drawFrame = _HandleDrawFrame.skipFrame; } } @override void handleDrawFrame() { - assert(_doDrawThisFrame != null); - if (_doDrawThisFrame!) { + if (_drawFrame == _HandleDrawFrame.reset) { + throw StateError('handleDrawFrame() called without paired handleBeginFrame()'); + } + if (_drawFrame == _HandleDrawFrame.drawFrame) { super.handleDrawFrame(); } - _doDrawThisFrame = null; + _drawFrame = _HandleDrawFrame.reset; _viewNeedsPaint = false; _expectingFrameToReassemble = false; if (_expectingFrame) { // set during pump diff --git a/packages/flutter_test/lib/src/widget_tester.dart b/packages/flutter_test/lib/src/widget_tester.dart index 23a91a6c27..595f98f86d 100644 --- a/packages/flutter_test/lib/src/widget_tester.dart +++ b/packages/flutter_test/lib/src/widget_tester.dart @@ -683,7 +683,11 @@ class WidgetTester extends WidgetController implements HitTestDispatcher, Ticker }()); dynamic caughtException; - void handleError(dynamic error, StackTrace stackTrace) => caughtException ??= error; + StackTrace? stackTrace; + void handleError(dynamic error, StackTrace trace) { + caughtException ??= error; + stackTrace ??= trace; + } await Future.microtask(() { binding.handleBeginFrame(duration); }).catchError(handleError); await idle(); @@ -691,7 +695,7 @@ class WidgetTester extends WidgetController implements HitTestDispatcher, Ticker await idle(); if (caughtException != null) { - throw caughtException as Object; // ignore: only_throw_errors, rethrowing caught exception. + Error.throwWithStackTrace(caughtException as Object, stackTrace!); } }