Enable only_throw_errors (flutter#91567)

andrewkolos · Oct 11, 2021 · 9421627 · 9421627
1 parent 71c876f
commit 9421627
Show file tree

Hide file tree

Showing 21 changed files with 77 additions and 57 deletions.
diff --git 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
@@ -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
@@ -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
@@ -249,7 +249,7 @@ class AsyncSnapshot<T> {
     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
@@ -1132,8 +1132,10 @@ class _ImageState extends State<Image> 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
@@ -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
@@ -92,7 +92,7 @@ mixin CreateFinderFactory {
       case 'String':
         return find.byKey(ValueKey<String>(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<Result> _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
@@ -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
@@ -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<String, String> expectationForTestValues = <String, String>{};
-  Object? getExpectationForTestThrowable;
+  Exception? getExpectationForTestThrowable;
   @override
   Future<String> 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
@@ -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<List<int>> 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
@@ -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
@@ -81,14 +81,14 @@ class MatchesGoldenFile extends AsyncMatcher {
       }
       imageFuture = captureImage(elements.single);
     } else {
-      throw 'must provide a Finder, Image, Future<Image>, List<int>, or Future<List<int>>';
+      throw AssertionError('must provide a Finder, Image, Future<Image>, List<int>, or Future<List<int>>');
     }
 
     final TestWidgetsFlutterBinding binding = TestWidgetsFlutterBinding.ensureInitialized() as TestWidgetsFlutterBinding;
     return binding.runAsync<String?>(() async {
       final ui.Image? image = await imageFuture;
       if (image == null) {
-        throw 'Future<Image> completed to null';
+        throw AssertionError('Future<Image> 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
@@ -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
@@ -29,7 +29,7 @@ class _BufferGoldenMatcher extends AsyncMatcher {
     } else if (item is Future<List<int>>) {
       buffer = Uint8List.fromList(await item);
     } else {
-      throw 'Expected `List<int>` or `Future<List<int>>`, instead found: ${item.runtimeType}';
+      throw AssertionError('Expected `List<int>` or `Future<List<int>>`, 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
@@ -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
@@ -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<String> 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
@@ -703,7 +703,7 @@ class _FakeComparator implements GoldenFileComparator {
       case _ComparatorBehavior.returnFalse:
         return Future<bool>.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
@@ -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<DiagnosticsNode> information = <DiagnosticsNode>[];
       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<DiagnosticsNode> information = <DiagnosticsNode>[];
-      expect(reportExpectCall(stack, information), 0);
-      expect(information, isEmpty);
-    }
+    final List<DiagnosticsNode> information = <DiagnosticsNode>[];
+    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
@@ -29,9 +29,13 @@ class TestAPISubclass extends TestAPI {
   }
 }
 
+class RecognizableTestException implements Exception {
+  const RecognizableTestException();
+}
+
 Future<Object> _guardedThrower() {
   return TestAsyncUtils.guard<Object>(() 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<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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<String> 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
@@ -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<ByteData?>? 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
@@ -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