From 9421627324c27c5256e87fe3244a8867de9f338b Mon Sep 17 00:00:00 2001 From: Ian Hickson Date: Mon, 11 Oct 2021 14:13:03 -0700 Subject: [PATCH] Enable `only_throw_errors` (#91567) --- analysis_options.yaml | 2 +- dev/analysis_options.yaml | 1 + examples/layers/test/analysis_options.yaml | 5 ++++ packages/flutter/lib/src/widgets/async.dart | 2 +- packages/flutter/lib/src/widgets/image.dart | 4 +++- packages/flutter/test/analysis_options.yaml | 5 ++++ .../lib/src/common/handler_factory.dart | 8 ++++--- .../lib/src/driver/vmservice_driver.dart | 2 +- .../test/flutter_goldens_test.dart | 2 +- .../flutter_test/lib/src/_goldens_io.dart | 5 ++-- .../flutter_test/lib/src/_goldens_web.dart | 6 ++--- .../flutter_test/lib/src/_matchers_io.dart | 4 ++-- packages/flutter_test/lib/src/binding.dart | 23 +++++++++--------- .../flutter_test/lib/src/buffer_matcher.dart | 2 +- .../flutter_test/lib/src/widget_tester.dart | 14 ++++++----- packages/flutter_test/test/goldens_test.dart | 2 +- packages/flutter_test/test/matchers_test.dart | 2 +- .../test/stack_manipulation_test.dart | 12 ++++------ .../test/test_async_utils_test.dart | 24 +++++++++++-------- .../test_default_binary_messenger_test.dart | 8 +++++-- packages/flutter_tools/analysis_options.yaml | 1 + 21 files changed, 77 insertions(+), 57 deletions(-) create mode 100644 examples/layers/test/analysis_options.yaml create mode 100644 packages/flutter/test/analysis_options.yaml diff --git a/analysis_options.yaml b/analysis_options.yaml index 3e6f118466..e1e9658722 100644 --- a/analysis_options.yaml +++ b/analysis_options.yaml @@ -137,7 +137,7 @@ linter: - null_closures # - omit_local_variable_types # opposite of always_specify_types # - one_member_abstracts # too many false positives - # - only_throw_errors # https://github.com/flutter/flutter/issues/5792 + - only_throw_errors # this does get disabled in a few places where we have legacy code that uses strings et al - overridden_fields - package_api_docs - package_names diff --git a/dev/analysis_options.yaml b/dev/analysis_options.yaml index 68626cc5f4..091804565d 100644 --- a/dev/analysis_options.yaml +++ b/dev/analysis_options.yaml @@ -3,3 +3,4 @@ include: ../analysis_options.yaml linter: rules: avoid_print: false # We use prints as debugging tools here all the time. + only_throw_errors: false # Tests use a less... rigorous style. diff --git a/examples/layers/test/analysis_options.yaml b/examples/layers/test/analysis_options.yaml new file mode 100644 index 0000000000..60222f820c --- /dev/null +++ b/examples/layers/test/analysis_options.yaml @@ -0,0 +1,5 @@ +include: ../../../analysis_options.yaml + +linter: + rules: + only_throw_errors: false # We are more flexible for tests. diff --git a/packages/flutter/lib/src/widgets/async.dart b/packages/flutter/lib/src/widgets/async.dart index 7d5319f9c7..5f8ebc46e5 100644 --- a/packages/flutter/lib/src/widgets/async.dart +++ b/packages/flutter/lib/src/widgets/async.dart @@ -249,7 +249,7 @@ class AsyncSnapshot { if (hasData) return data!; if (hasError) - throw error!; + throw error!; // ignore: only_throw_errors, since we're just propagating an existing error throw StateError('Snapshot has neither data nor error'); } diff --git a/packages/flutter/lib/src/widgets/image.dart b/packages/flutter/lib/src/widgets/image.dart index 41726191af..81ed66599f 100644 --- a/packages/flutter/lib/src/widgets/image.dart +++ b/packages/flutter/lib/src/widgets/image.dart @@ -1132,8 +1132,10 @@ class _ImageState extends State with WidgetsBindingObserver { _lastStack = stackTrace; }); assert(() { - if (widget.errorBuilder == null) + if (widget.errorBuilder == null) { + // ignore: only_throw_errors, since we're just proxying the error. throw error; // Ensures the error message is printed to the console. + } return true; }()); } diff --git a/packages/flutter/test/analysis_options.yaml b/packages/flutter/test/analysis_options.yaml new file mode 100644 index 0000000000..5b7a383f6c --- /dev/null +++ b/packages/flutter/test/analysis_options.yaml @@ -0,0 +1,5 @@ +include: ../analysis_options.yaml + +linter: + rules: + only_throw_errors: false # We are more flexible for tests. diff --git a/packages/flutter_driver/lib/src/common/handler_factory.dart b/packages/flutter_driver/lib/src/common/handler_factory.dart index 7c3aa340d7..8840802bb2 100644 --- a/packages/flutter_driver/lib/src/common/handler_factory.dart +++ b/packages/flutter_driver/lib/src/common/handler_factory.dart @@ -92,7 +92,7 @@ mixin CreateFinderFactory { case 'String': return find.byKey(ValueKey(arguments.keyValue as String)); default: - throw 'Unsupported ByValueKey type: ${arguments.keyValueType}'; + throw UnimplementedError('Unsupported ByValueKey type: ${arguments.keyValueType}'); } } @@ -194,8 +194,10 @@ mixin CommandHandlerFactory { Future _enterText(Command command) async { if (!_testTextInput.isRegistered) { - throw 'Unable to fulfill `FlutterDriver.enterText`. Text emulation is ' - 'disabled. You can enable it using `FlutterDriver.setTextEntryEmulation`.'; + throw StateError( + 'Unable to fulfill `FlutterDriver.enterText`. Text emulation is ' + 'disabled. You can enable it using `FlutterDriver.setTextEntryEmulation`.', + ); } final EnterText enterTextCommand = command as EnterText; _testTextInput.enterText(enterTextCommand.text); diff --git a/packages/flutter_driver/lib/src/driver/vmservice_driver.dart b/packages/flutter_driver/lib/src/driver/vmservice_driver.dart index 64b9ad7849..bcaa5bde34 100644 --- a/packages/flutter_driver/lib/src/driver/vmservice_driver.dart +++ b/packages/flutter_driver/lib/src/driver/vmservice_driver.dart @@ -148,7 +148,7 @@ class VMServiceFlutterDriver extends FlutterDriver { return vms.Success(); } else { // Failed to resume due to another reason. Fail hard. - throw e; + throw e; // ignore: only_throw_errors, proxying the error from upstream. } }); } diff --git a/packages/flutter_goldens/test/flutter_goldens_test.dart b/packages/flutter_goldens/test/flutter_goldens_test.dart index e47bdb30bf..551e72c740 100644 --- a/packages/flutter_goldens/test/flutter_goldens_test.dart +++ b/packages/flutter_goldens/test/flutter_goldens_test.dart @@ -686,7 +686,7 @@ class FakeProcessManager extends Fake implements ProcessManager { // See also dev/automated_tests/flutter_test/flutter_gold_test.dart class FakeSkiaGoldClient extends Fake implements SkiaGoldClient { Map expectationForTestValues = {}; - Object? getExpectationForTestThrowable; + Exception? getExpectationForTestThrowable; @override Future getExpectationForTest(String testName) async { if (getExpectationForTestThrowable != null) { diff --git a/packages/flutter_test/lib/src/_goldens_io.dart b/packages/flutter_test/lib/src/_goldens_io.dart index fd854fd6d9..cf00bad736 100644 --- a/packages/flutter_test/lib/src/_goldens_io.dart +++ b/packages/flutter_test/lib/src/_goldens_io.dart @@ -10,8 +10,7 @@ import 'dart:ui'; import 'package:flutter/foundation.dart'; import 'package:path/path.dart' as path; -// ignore: deprecated_member_use -import 'package:test_api/test_api.dart' as test_package show TestFailure; +import 'package:test_api/expect.dart' show fail; import 'goldens.dart'; import 'test_async_utils.dart'; @@ -117,7 +116,7 @@ class LocalFileComparator extends GoldenFileComparator with LocalComparisonOutpu Future> getGoldenBytes(Uri golden) async { final File goldenFile = _getGoldenFile(golden); if (!goldenFile.existsSync()) { - throw test_package.TestFailure( + fail( 'Could not be compared against non-existent file: "$golden"' ); } diff --git a/packages/flutter_test/lib/src/_goldens_web.dart b/packages/flutter_test/lib/src/_goldens_web.dart index 373e234870..2b9b2bed39 100644 --- a/packages/flutter_test/lib/src/_goldens_web.dart +++ b/packages/flutter_test/lib/src/_goldens_web.dart @@ -6,8 +6,7 @@ import 'dart:convert'; import 'dart:html' as html; import 'dart:typed_data'; -// ignore: deprecated_member_use -import 'package:test_api/test_api.dart' as test_package show TestFailure; +import 'package:test_api/expect.dart' show fail; import 'goldens.dart'; @@ -72,9 +71,8 @@ class DefaultWebGoldenComparator extends WebGoldenComparator { final String response = request.response as String; if (response == 'true') { return true; - } else { - throw test_package.TestFailure(response); } + fail(response); } @override diff --git a/packages/flutter_test/lib/src/_matchers_io.dart b/packages/flutter_test/lib/src/_matchers_io.dart index 037cd3bce7..878fa427b5 100644 --- a/packages/flutter_test/lib/src/_matchers_io.dart +++ b/packages/flutter_test/lib/src/_matchers_io.dart @@ -81,14 +81,14 @@ class MatchesGoldenFile extends AsyncMatcher { } imageFuture = captureImage(elements.single); } else { - throw 'must provide a Finder, Image, Future, List, or Future>'; + throw AssertionError('must provide a Finder, Image, Future, List, or Future>'); } final TestWidgetsFlutterBinding binding = TestWidgetsFlutterBinding.ensureInitialized() as TestWidgetsFlutterBinding; return binding.runAsync(() async { final ui.Image? image = await imageFuture; if (image == null) { - throw 'Future completed to null'; + throw AssertionError('Future completed to null'); } final ByteData? bytes = await image.toByteData(format: ui.ImageByteFormat.png); if (bytes == null) diff --git a/packages/flutter_test/lib/src/binding.dart b/packages/flutter_test/lib/src/binding.dart index 995283bf90..34220c11f4 100644 --- a/packages/flutter_test/lib/src/binding.dart +++ b/packages/flutter_test/lib/src/binding.dart @@ -14,7 +14,8 @@ import 'package:flutter/scheduler.dart'; import 'package:flutter/services.dart'; import 'package:flutter/widgets.dart'; import 'package:stack_trace/stack_trace.dart' as stack_trace; -import 'package:test_api/test_api.dart' as test_package; // ignore: deprecated_member_use +import 'package:test_api/expect.dart' show fail; +import 'package:test_api/test_api.dart' as test_package show Timeout; // ignore: deprecated_member_use import 'package:vector_math/vector_math_64.dart'; import '_binding_io.dart' if (dart.library.html) '_binding_web.dart' as binding; @@ -1005,11 +1006,11 @@ class AutomatedTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { assert(() { if (_pendingAsyncTasks == null) return true; - throw test_package.TestFailure( - 'Reentrant call to runAsync() denied.\n' - 'runAsync() was called, then before its future completed, it ' - 'was called again. You must wait for the first returned future ' - 'to complete before calling runAsync() again.' + fail( + 'Reentrant call to runAsync() denied.\n' + 'runAsync() was called, then before its future completed, it ' + 'was called again. You must wait for the first returned future ' + 'to complete before calling runAsync() again.' ); }()); @@ -1573,11 +1574,11 @@ class LiveTestWidgetsFlutterBinding extends TestWidgetsFlutterBinding { assert(() { if (!_runningAsyncTasks) return true; - throw test_package.TestFailure( - 'Reentrant call to runAsync() denied.\n' - 'runAsync() was called, then before its future completed, it ' - 'was called again. You must wait for the first returned future ' - 'to complete before calling runAsync() again.' + fail( + 'Reentrant call to runAsync() denied.\n' + 'runAsync() was called, then before its future completed, it ' + 'was called again. You must wait for the first returned future ' + 'to complete before calling runAsync() again.' ); }()); diff --git a/packages/flutter_test/lib/src/buffer_matcher.dart b/packages/flutter_test/lib/src/buffer_matcher.dart index e3acd59335..875fa0cefa 100644 --- a/packages/flutter_test/lib/src/buffer_matcher.dart +++ b/packages/flutter_test/lib/src/buffer_matcher.dart @@ -29,7 +29,7 @@ class _BufferGoldenMatcher extends AsyncMatcher { } else if (item is Future>) { buffer = Uint8List.fromList(await item); } else { - throw 'Expected `List` or `Future>`, instead found: ${item.runtimeType}'; + throw AssertionError('Expected `List` or `Future>`, instead found: ${item.runtimeType}'); } final Uri testNameUri = goldenFileComparator.getTestUri(key, version); if (autoUpdateGoldenFiles) { diff --git a/packages/flutter_test/lib/src/widget_tester.dart b/packages/flutter_test/lib/src/widget_tester.dart index eb9e64417a..dcc2ea5617 100644 --- a/packages/flutter_test/lib/src/widget_tester.dart +++ b/packages/flutter_test/lib/src/widget_tester.dart @@ -620,7 +620,7 @@ class WidgetTester extends WidgetController implements HitTestDispatcher, Ticker assert(() { final TestWidgetsFlutterBinding widgetsBinding = binding; return widgetsBinding is LiveTestWidgetsFlutterBinding && - widgetsBinding.framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.benchmark; + widgetsBinding.framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.benchmark; }()); dynamic caughtException; @@ -632,7 +632,7 @@ class WidgetTester extends WidgetController implements HitTestDispatcher, Ticker await idle(); if (caughtException != null) { - throw caughtException as Object; + throw caughtException as Object; // ignore: only_throw_errors, rethrowing caught exception. } } @@ -650,10 +650,12 @@ class WidgetTester extends WidgetController implements HitTestDispatcher, Ticker final WidgetsBinding binding = this.binding; if (binding is LiveTestWidgetsFlutterBinding && binding.framePolicy == LiveTestWidgetsFlutterBindingFramePolicy.benchmark) { - throw 'When using LiveTestWidgetsFlutterBindingFramePolicy.benchmark, ' - 'hasScheduledFrame is never set to true. This means that pumpAndSettle() ' - 'cannot be used, because it has no way to know if the application has ' - 'stopped registering new frames.'; + test_package.fail( + 'When using LiveTestWidgetsFlutterBindingFramePolicy.benchmark, ' + 'hasScheduledFrame is never set to true. This means that pumpAndSettle() ' + 'cannot be used, because it has no way to know if the application has ' + 'stopped registering new frames.', + ); } return true; }()); diff --git a/packages/flutter_test/test/goldens_test.dart b/packages/flutter_test/test/goldens_test.dart index 6082843534..c5b431c88f 100644 --- a/packages/flutter_test/test/goldens_test.dart +++ b/packages/flutter_test/test/goldens_test.dart @@ -109,7 +109,7 @@ void main() { Uri.parse('/foo/bar/'), ); TestAsyncUtils.verifyAllScopesClosed(); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } on FlutterError catch (e) { final List lines = e.message.split('\n'); expectSync(lines[0], 'Asynchronous call to guarded function leaked.'); diff --git a/packages/flutter_test/test/matchers_test.dart b/packages/flutter_test/test/matchers_test.dart index f157214bc3..3877dd0435 100644 --- a/packages/flutter_test/test/matchers_test.dart +++ b/packages/flutter_test/test/matchers_test.dart @@ -703,7 +703,7 @@ class _FakeComparator implements GoldenFileComparator { case _ComparatorBehavior.returnFalse: return Future.value(false); case _ComparatorBehavior.throwTestFailure: - throw TestFailure('fake message'); + fail('fake message'); } } diff --git a/packages/flutter_test/test/stack_manipulation_test.dart b/packages/flutter_test/test/stack_manipulation_test.dart index a484026ba5..45b4940b2f 100644 --- a/packages/flutter_test/test/stack_manipulation_test.dart +++ b/packages/flutter_test/test/stack_manipulation_test.dart @@ -9,7 +9,7 @@ void main() { test('stack manipulation: reportExpectCall', () { try { expect(false, isTrue); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } catch (e, stack) { final List information = []; expect(reportExpectCall(stack, information), 4); @@ -19,12 +19,8 @@ void main() { expect(lines[1], matches(r'^ .*stack_manipulation_test.dart line [0-9]+$')); } - try { - throw Object(); - } catch (e, stack) { - final List information = []; - expect(reportExpectCall(stack, information), 0); - expect(information, isEmpty); - } + final List information = []; + expect(reportExpectCall(StackTrace.current, information), 0); + expect(information, isEmpty); }); } diff --git a/packages/flutter_test/test/test_async_utils_test.dart b/packages/flutter_test/test/test_async_utils_test.dart index 5ac2f2b185..67d667229f 100644 --- a/packages/flutter_test/test/test_async_utils_test.dart +++ b/packages/flutter_test/test/test_async_utils_test.dart @@ -29,9 +29,13 @@ class TestAPISubclass extends TestAPI { } } +class RecognizableTestException implements Exception { + const RecognizableTestException(); +} + Future _guardedThrower() { return TestAsyncUtils.guard(() async { - throw 'Hello'; + throw const RecognizableTestException(); }); } @@ -42,7 +46,7 @@ void main() { f1 = testAPI.testGuard1(); try { f2 = testAPI.testGuard2(); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } on FlutterError catch (e) { final List lines = e.message.split('\n'); real_test.expect(lines[0], 'Guarded function conflict.'); @@ -64,7 +68,7 @@ void main() { f1 = testAPI.testGuard1(); try { f2 = testAPI.testGuard2(); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } on FlutterError catch (e) { final List lines = e.message.split('\n'); real_test.expect(lines[0], 'Guarded function conflict.'); @@ -86,7 +90,7 @@ void main() { f1 = testAPI.testGuard1(); try { f2 = testAPI.testGuard3(); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } on FlutterError catch (e) { final List lines = e.message.split('\n'); real_test.expect(lines[0], 'Guarded function conflict.'); @@ -108,7 +112,7 @@ void main() { f1 = testAPI.testGuard1(); try { flutter_test.expect(0, 0); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } on FlutterError catch (e) { final List lines = e.message.split('\n'); real_test.expect(lines[0], 'Guarded function conflict.'); @@ -129,7 +133,7 @@ void main() { try { f1 = tester.pump(); f2 = tester.pump(); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } on FlutterError catch (e) { final List lines = e.message.split('\n'); real_test.expect(lines[0], 'Guarded function conflict.'); @@ -171,7 +175,7 @@ void main() { try { f1 = tester.pump(); TestAsyncUtils.verifyAllScopesClosed(); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } on FlutterError catch (e) { final List lines = e.message.split('\n'); real_test.expect(lines[0], 'Asynchronous call to guarded function leaked.'); @@ -196,7 +200,7 @@ void main() { try { f1 = tester.pump(); TestAsyncUtils.verifyAllScopesClosed(); - throw 'unexpectedly did not throw'; + fail('unexpectedly did not throw'); } on FlutterError catch (e) { final List lines = e.message.split('\n'); real_test.expect(lines[0], 'Asynchronous call to guarded function leaked.'); @@ -220,8 +224,8 @@ void main() { try { await _guardedThrower(); expect(false, true); // _guardedThrower should throw and we shouldn't reach here - } on String catch (s) { - expect(s, 'Hello'); + } on RecognizableTestException catch (e) { + expect(e, const RecognizableTestException()); } }); diff --git a/packages/flutter_test/test/test_default_binary_messenger_test.dart b/packages/flutter_test/test/test_default_binary_messenger_test.dart index 30cf93ddf6..5052882682 100644 --- a/packages/flutter_test/test/test_default_binary_messenger_test.dart +++ b/packages/flutter_test/test/test_default_binary_messenger_test.dart @@ -7,12 +7,16 @@ import 'dart:ui' as ui; import 'package:flutter/services.dart'; import 'package:flutter_test/flutter_test.dart'; +class RecognizableTestException implements Exception { + const RecognizableTestException(); +} + class TestDelegate extends BinaryMessenger { @override Future? send(String channel, ByteData? message) async { expect(channel, ''); expect(message, isNull); - throw 'Vic Fontaine'; + throw const RecognizableTestException(); } // Rest of the API isn't needed for this test. @@ -32,7 +36,7 @@ void main() { await TestDefaultBinaryMessenger(delegate).send('', null); expect(true, isFalse); // should not reach here } catch (error) { - expect(error, 'Vic Fontaine'); + expect(error, const RecognizableTestException()); } }); } diff --git a/packages/flutter_tools/analysis_options.yaml b/packages/flutter_tools/analysis_options.yaml index d777166af5..2a89b53168 100644 --- a/packages/flutter_tools/analysis_options.yaml +++ b/packages/flutter_tools/analysis_options.yaml @@ -6,6 +6,7 @@ linter: curly_braces_in_flow_control_structures: true library_private_types_in_public_api: false # Tool does not have any public API. no_runtimeType_toString: false # We use runtimeType for debugging in the tool. + only_throw_errors: false # For historical reasons we throw strings a lot. prefer_relative_imports: true public_member_api_docs: false # Tool does not have any public API. unawaited_futures: true