From 91cccc8e6b83ada2dcffe266ff4a6d3c8d0ea582 Mon Sep 17 00:00:00 2001 From: Michael Goderbauer Date: Mon, 11 Mar 2024 16:05:24 -0700 Subject: [PATCH] Fail tests on exceptions raised after test completed (#144706) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A test was failing silently because of this (see https://github.com/flutter/flutter/issues/144353 and fixed in https://github.com/flutter/flutter/pull/144709). The failure went undetected for months. Ideally, this should have been a regular non-silent failure. This change makes that so. `package:test` can properly handle reported exceptions outside of test cases. With this change, the test fails as follows: ``` 00:03 +82: Smoke test material/color_scheme/dynamic_content_color.0.dart ══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════ The following assertion was thrown running a test (but after the test had completed): setState() called after dispose(): _DynamicColorExampleState#1cd37(lifecycle state: defunct, not mounted) This error happens if you call setState() on a State object for a widget that no longer appears in the widget tree (e.g., whose parent widget no longer includes the widget in its build). This error can occur when code calls setState() from a timer or an animation callback. The preferred solution is to cancel the timer or stop listening to the animation in the dispose() callback. Another solution is to check the "mounted" property of this object before calling setState() to ensure the object is still in the tree. This error might indicate a memory leak if setState() is being called because another object is retaining a reference to this State object after it has been removed from the tree. To avoid memory leaks, consider breaking the reference to this object during dispose(). When the exception was thrown, this was the stack: #0 State.setState. (package:flutter/src/widgets/framework.dart:1167:9) #1 State.setState (package:flutter/src/widgets/framework.dart:1202:6) #2 _DynamicColorExampleState._updateImage (package:flutter_api_samples/material/color_scheme/dynamic_content_color.0.dart:191:5) ════════════════════════════════════════════════════════════════════════════════════════════════════ 00:03 +81 -1: Smoke test material/context_menu/context_menu_controller.0.dart 00:03 +81 -1: Smoke test material/color_scheme/dynamic_content_color.0.dart [E] Test failed. See exception logs above. The test description was: Smoke test material/color_scheme/dynamic_content_color.0.dart This test failed after it had already completed. Make sure to use a matching library which informs the test runner of pending async work. ``` --- .../fail_test_on_exception_after_test.dart | 29 +++++++++++++++++++ dev/bots/test.dart | 21 +++++++++++++- dev/bots/test/test_test.dart | 4 +-- packages/flutter_test/lib/src/binding.dart | 7 ++--- 4 files changed, 54 insertions(+), 7 deletions(-) create mode 100644 dev/automated_tests/test_smoke_test/fail_test_on_exception_after_test.dart diff --git a/dev/automated_tests/test_smoke_test/fail_test_on_exception_after_test.dart b/dev/automated_tests/test_smoke_test/fail_test_on_exception_after_test.dart new file mode 100644 index 00000000000..b3b66bd185b --- /dev/null +++ b/dev/automated_tests/test_smoke_test/fail_test_on_exception_after_test.dart @@ -0,0 +1,29 @@ +// Copyright 2014 The Flutter Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +import 'dart:async'; + +import 'package:flutter_test/flutter_test.dart'; + +// This is a test to make sure that an asynchronous exception thrown after a +// test ended actually causes a test failure. +// See //flutter/dev/bots/test.dart + +void main() { + final Completer complete = Completer(); + + testWidgets('test smoke test -- this test SHOULD FAIL', (WidgetTester tester) async { + tester.runAsync(() async { + Timer.run(() { + complete.complete(); + throw StateError('Exception thrown after test completed.'); + }); + }); + }); + + tearDown(() async { + print('Waiting for asynchronous exception...'); + await complete.future; + }); +} diff --git a/dev/bots/test.dart b/dev/bots/test.dart index ada45fcbc01..d43fdbf352c 100644 --- a/dev/bots/test.dart +++ b/dev/bots/test.dart @@ -346,7 +346,26 @@ Future _runTestHarnessTests() async { : 'Failed to find the stack trace for the pending Timer.\n\n' 'stdout:\n${result.flattenedStdout}\n\n' 'stderr:\n${result.flattenedStderr}'; - }), + }, + ), + () => _runFlutterTest( + automatedTests, + script: path.join('test_smoke_test', 'fail_test_on_exception_after_test.dart'), + expectFailure: true, + printOutput: false, + outputChecker: (CommandResult result) { + const String expectedError = '══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════\n' + 'The following StateError was thrown running a test (but after the test had completed):\n' + 'Bad state: Exception thrown after test completed.'; + if (result.flattenedStdout!.contains(expectedError)) { + return null; + } + return 'Failed to find expected output on stdout.\n\n' + 'Expected output:\n$expectedError\n\n' + 'Actual stdout:\n${result.flattenedStdout}\n\n' + 'Actual stderr:\n${result.flattenedStderr}'; + }, + ), () => _runFlutterTest( automatedTests, script: path.join('test_smoke_test', 'crash1_test.dart'), diff --git a/dev/bots/test/test_test.dart b/dev/bots/test/test_test.dart index 7f5ae0c2d08..9994f66d006 100644 --- a/dev/bots/test/test_test.dart +++ b/dev/bots/test/test_test.dart @@ -128,13 +128,13 @@ void main() { {'SHARD': kTestHarnessShardName, 'SUBSHARD': '1_3'}, ); expectExitCode(result, 0); - expect(result.stdout, contains('Selecting subshard 1 of 3 (tests 1-3 of 8)')); + expect(result.stdout, contains('Selecting subshard 1 of 3 (tests 1-3 of 9)')); result = await runScript( {'SHARD': kTestHarnessShardName, 'SUBSHARD': '3_3'}, ); expectExitCode(result, 0); - expect(result.stdout, contains('Selecting subshard 3 of 3 (tests 7-8 of 8)')); + expect(result.stdout, contains('Selecting subshard 3 of 3 (tests 7-9 of 9)')); }); test('exits with code 1 when SUBSHARD index greater than total', () async { diff --git a/packages/flutter_test/lib/src/binding.dart b/packages/flutter_test/lib/src/binding.dart index 6a0824163e2..999ff5863af 100644 --- a/packages/flutter_test/lib/src/binding.dart +++ b/packages/flutter_test/lib/src/binding.dart @@ -910,15 +910,14 @@ abstract class TestWidgetsFlutterBinding extends BindingBase // Ideally, once the test has failed we would stop getting errors from the test. // However, if someone tries hard enough they could get in a state where this happens. // If we silently dropped these errors on the ground, nobody would ever know. So instead - // we report them to the console. They don't cause test failures, but hopefully someone - // will see them in the logs at some point. + // we raise them and fail the test after it has already completed. debugPrint = debugPrintOverride; // just in case the test overrides it -- otherwise we won't see the error! - FlutterError.dumpErrorToConsole(FlutterErrorDetails( + reportTestException(FlutterErrorDetails( exception: exception, stack: stack, context: ErrorDescription('running a test (but after the test had completed)'), library: 'Flutter test framework', - ), forceReport: true); + ), description); return; } // This is where test failures, e.g. those in expect(), will end up.