From 62f72311ae0896a7ac257934a24e61c1182dd9c1 Mon Sep 17 00:00:00 2001 From: Paul Berry Date: Sat, 19 Jun 2021 16:58:04 +0000 Subject: [PATCH] Migration: make API tests fail if there's an unexpected error. I've recently wasted a lot of time tracking down strange behaviors that resulted from inadvertenly writing a migration API test containing a static error. Failing the test early if there's an error should avoid that problem in the future. Change-Id: Idc9d84901a517ce9555fa3180f5c669aea449ea5 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/204180 Reviewed-by: Samuel Rawlins Commit-Queue: Paul Berry --- .../lib/src/test_utilities/mock_sdk.dart | 4 +- pkg/nnbd_migration/test/api_test.dart | 67 ++++++++++++------- 2 files changed, 46 insertions(+), 25 deletions(-) diff --git a/pkg/analyzer/lib/src/test_utilities/mock_sdk.dart b/pkg/analyzer/lib/src/test_utilities/mock_sdk.dart index a7a7b278699..a19caf2c8fe 100644 --- a/pkg/analyzer/lib/src/test_utilities/mock_sdk.dart +++ b/pkg/analyzer/lib/src/test_utilities/mock_sdk.dart @@ -507,7 +507,9 @@ abstract class num implements Comparable { int toInt(); } -abstract class Match {} +abstract class Match { + int get start; +} class Object { const Object(); diff --git a/pkg/nnbd_migration/test/api_test.dart b/pkg/nnbd_migration/test/api_test.dart index cd978d63701..c5e44a76ba4 100644 --- a/pkg/nnbd_migration/test/api_test.dart +++ b/pkg/nnbd_migration/test/api_test.dart @@ -3,6 +3,8 @@ // BSD-style license that can be found in the LICENSE file. import 'package:analyzer/dart/analysis/results.dart'; +import 'package:analyzer/diagnostic/diagnostic.dart'; +import 'package:analyzer/error/error.dart'; import 'package:analyzer_plugin/protocol/protocol_common.dart'; import 'package:nnbd_migration/nnbd_migration.dart'; import 'package:test/test.dart'; @@ -50,7 +52,8 @@ abstract class _ProvisionalApiTestBase extends AbstractContextTest { Map input, Map expectedOutput, {Map migratedInput = const {}, bool removeViaComments = false, - bool warnOnWeakCode = false}) async { + bool warnOnWeakCode = false, + bool allowErrors = false}) async { for (var path in migratedInput.keys) { newFile(path, content: migratedInput[path]); } @@ -66,6 +69,11 @@ abstract class _ProvisionalApiTestBase extends AbstractContextTest { var resolvedLibrary = await session.getResolvedLibrary2(path); if (resolvedLibrary is ResolvedLibraryResult) { for (var unit in resolvedLibrary.units) { + var errors = + unit.errors.where((e) => e.severity == Severity.error).toList(); + if (!allowErrors && errors.isNotEmpty) { + fail('Unexpected error(s): $errors'); + } migration.prepareInput(unit); } } @@ -112,13 +120,15 @@ abstract class _ProvisionalApiTestBase extends AbstractContextTest { Future _checkSingleFileChanges(String content, String expected, {Map migratedInput = const {}, bool removeViaComments = false, - bool warnOnWeakCode = false}) async { + bool warnOnWeakCode = false, + bool allowErrors = false}) async { var sourcePath = convertPath('$testsPath/lib/test.dart'); await _checkMultipleFileChanges( {sourcePath: content}, {sourcePath: expected}, migratedInput: migratedInput, removeViaComments: removeViaComments, - warnOnWeakCode: warnOnWeakCode); + warnOnWeakCode: warnOnWeakCode, + allowErrors: allowErrors); } } @@ -516,6 +526,7 @@ class B implements List { final C c; B(this.c); B cast() => c._castFrom(this); + noSuchMethod(invocation) => super.noSuchMethod(invocation); } abstract class C { B _castFrom(B source); @@ -526,6 +537,7 @@ class B implements List { final C c; B(this.c); B cast() => c._castFrom(this); + noSuchMethod(invocation) => super.noSuchMethod(invocation); } abstract class C { B _castFrom(B source); @@ -609,7 +621,9 @@ main() { f(null, null); } '''; - await _checkSingleFileChanges(content, expected); + // Note: using allowErrors=true because variables introduced by a catch + // clause are final + await _checkSingleFileChanges(content, expected, allowErrors: true); } Future test_catch_with_on() async { @@ -649,7 +663,9 @@ main() { f(null, null); } '''; - await _checkSingleFileChanges(content, expected); + // Note: using allowErrors=true because variables introduced by a catch + // clause are final + await _checkSingleFileChanges(content, expected, allowErrors: true); } Future test_class_alias_synthetic_constructor_with_parameters() async { @@ -1947,7 +1963,9 @@ test() { x[0] = 1 as Null; } '''; - await _checkSingleFileChanges(content, expected); + // Note: using allowErrors=true because casting a literal int to a Null is + // an error + await _checkSingleFileChanges(content, expected, allowErrors: true); } Future test_downcast_type_argument_preserve_nullability() async { @@ -2112,7 +2130,7 @@ List _f(List xs) => []; var content = ''' enum E { value -}; +} E f() => E.value; int g() => f().index; @@ -2131,7 +2149,7 @@ void h() { var expected = ''' enum E { value -}; +} E f() => E.value; int g() => f().index; @@ -2979,7 +2997,8 @@ class C { } g(String s) {} '''; - await _checkSingleFileChanges(content, expected); + // Note: using allowErrors=true because an uninitialized field is an error + await _checkSingleFileChanges(content, expected, allowErrors: true); } Future test_field_formal_param_typed() async { @@ -3159,9 +3178,9 @@ class C { class C { int i; C() : i = 0; - factory C.factoryConstructor => C(); - factory C.factoryRedirect = D; - C.redirect : this(); + factory C.factoryConstructor() => C(); + factory C.factoryRedirect() = D; + C.redirect() : this(); } class D extends C {} '''; @@ -3169,9 +3188,9 @@ class D extends C {} class C { int i; C() : i = 0; - factory C.factoryConstructor => C(); - factory C.factoryRedirect = D; - C.redirect : this(); + factory C.factoryConstructor() => C(); + factory C.factoryRedirect() = D; + C.redirect() : this(); } class D extends C {} '''; @@ -3666,7 +3685,7 @@ int? test(C c) { void test({String foo}) async { var f = () { return "hello"; - } + }; foo.length; } @@ -3675,7 +3694,7 @@ void test({String foo}) async { void test({required String foo}) async { var f = () { return "hello"; - } + }; foo.length; } @@ -6969,7 +6988,7 @@ class C { } void main() { - C().m(null!); + C().m(null/*!*/); } '''; var expected = ''' @@ -7312,10 +7331,10 @@ void main() { setUp(() { i = 1; }); + f(int /*?*/ i) {} test('a', () { f(i); }); - f(int /*?*/ i) {} } '''; var expected = ''' @@ -7325,10 +7344,10 @@ void main() { setUp(() { i = 1; }); + f(int? i) {} test('a', () { f(i); }); - f(int? i) {} } '''; await _checkSingleFileChanges(content, expected); @@ -7464,10 +7483,10 @@ int? g() => f(); // The inference of C forces class C to be declared as // C. var content = ''' -class C { +abstract class C { void m(T t); } -class D { +abstract class D { void m(T t); } f(C c, D d) { @@ -7475,10 +7494,10 @@ f(C c, D d) { } '''; var expected = ''' -class C { +abstract class C { void m(T t); } -class D { +abstract class D { void m(T t); } f(C c, D d) {