Skip to content

Commit b755641

Browse files
authored
Address frame policy benchmark flakes (flutter#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 flutter#150542 Fixes flutter#150543 - throw stack!
1 parent 74caead commit b755641

File tree

4 files changed

+25
-11
lines changed

4 files changed

+25
-11
lines changed

dev/benchmarks/microbenchmarks/lib/layout/text_intrinsic_bench.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,7 @@ Future<void> execute() async {
3333
await benchmarkWidgets((WidgetTester tester) async {
3434
runApp(intrinsicTextHeight);
3535
// Wait for the UI to stabilize.
36-
await tester.pump(const Duration(seconds: 1));
36+
await tester.pumpAndSettle(const Duration(seconds: 1));
3737

3838
final TestViewConfiguration big = TestViewConfiguration.fromView(
3939
size: const Size(360.0, 640.0),

dev/benchmarks/microbenchmarks/lib/stocks/build_bench.dart

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -29,7 +29,7 @@ Future<List<double>> runBuildBenchmark() async {
2929
await tester.pump(const Duration(seconds: 1)); // Complete startup animation
3030
await tester.tapAt(const Offset(20.0, 40.0)); // Open drawer
3131
await tester.pump(); // Start drawer animation
32-
await tester.pump(const Duration(seconds: 1)); // Complete drawer animation
32+
await tester.pumpAndSettle(const Duration(seconds: 1)); // Complete drawer animation
3333

3434
final Element appState = tester.element(find.byType(stocks.StocksApp));
3535
binding.framePolicy = LiveTestWidgetsFlutterBindingFramePolicy.benchmark;

packages/flutter_test/lib/src/binding.dart

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1649,6 +1649,12 @@ enum LiveTestWidgetsFlutterBindingFramePolicy {
16491649
benchmarkLive,
16501650
}
16511651

1652+
enum _HandleDrawFrame {
1653+
reset,
1654+
drawFrame,
1655+
skipFrame,
1656+
}
1657+
16521658
/// A variant of [TestWidgetsFlutterBinding] for executing tests
16531659
/// on a device, typically via `flutter run`, or via integration tests.
16541660
/// This is intended to allow interactive test development.
@@ -1779,31 +1785,35 @@ class LiveTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding {
17791785
return super.reassembleApplication();
17801786
}
17811787

1782-
bool? _doDrawThisFrame;
1788+
_HandleDrawFrame _drawFrame = _HandleDrawFrame.reset;
17831789

17841790
@override
17851791
void handleBeginFrame(Duration? rawTimeStamp) {
1786-
assert(_doDrawThisFrame == null);
1792+
if (_drawFrame != _HandleDrawFrame.reset) {
1793+
throw StateError('handleBeginFrame() called before previous handleDrawFrame()');
1794+
}
17871795
if (_expectingFrame ||
17881796
_expectingFrameToReassemble ||
17891797
(framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.fullyLive) ||
17901798
(framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.benchmarkLive) ||
17911799
(framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.benchmark) ||
17921800
(framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.fadePointers && _viewNeedsPaint)) {
1793-
_doDrawThisFrame = true;
1801+
_drawFrame = _HandleDrawFrame.drawFrame;
17941802
super.handleBeginFrame(rawTimeStamp);
17951803
} else {
1796-
_doDrawThisFrame = false;
1804+
_drawFrame = _HandleDrawFrame.skipFrame;
17971805
}
17981806
}
17991807

18001808
@override
18011809
void handleDrawFrame() {
1802-
assert(_doDrawThisFrame != null);
1803-
if (_doDrawThisFrame!) {
1810+
if (_drawFrame == _HandleDrawFrame.reset) {
1811+
throw StateError('handleDrawFrame() called without paired handleBeginFrame()');
1812+
}
1813+
if (_drawFrame == _HandleDrawFrame.drawFrame) {
18041814
super.handleDrawFrame();
18051815
}
1806-
_doDrawThisFrame = null;
1816+
_drawFrame = _HandleDrawFrame.reset;
18071817
_viewNeedsPaint = false;
18081818
_expectingFrameToReassemble = false;
18091819
if (_expectingFrame) { // set during pump

packages/flutter_test/lib/src/widget_tester.dart

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -683,15 +683,19 @@ class WidgetTester extends WidgetController implements HitTestDispatcher, Ticker
683683
}());
684684

685685
dynamic caughtException;
686-
void handleError(dynamic error, StackTrace stackTrace) => caughtException ??= error;
686+
StackTrace? stackTrace;
687+
void handleError(dynamic error, StackTrace trace) {
688+
caughtException ??= error;
689+
stackTrace ??= trace;
690+
}
687691

688692
await Future<void>.microtask(() { binding.handleBeginFrame(duration); }).catchError(handleError);
689693
await idle();
690694
await Future<void>.microtask(() { binding.handleDrawFrame(); }).catchError(handleError);
691695
await idle();
692696

693697
if (caughtException != null) {
694-
throw caughtException as Object; // ignore: only_throw_errors, rethrowing caught exception.
698+
Error.throwWithStackTrace(caughtException as Object, stackTrace!);
695699
}
696700
}
697701

0 commit comments

Comments
 (0)