From 050afd650f7cdfaee8c19c1bebadc3fcbc379bd0 Mon Sep 17 00:00:00 2001 From: Sigmund Cherem Date: Wed, 14 Feb 2024 20:12:05 +0000 Subject: [PATCH] [expect] introduce Expect.throwsWhen and Expect.throwsTypeErrorWhen Today, most tests that touch on a behavior variation end up skipping expectations or the entirety of a test for some testing configurations. Moving forward, we'd like skip less and try to account for the behavior variations if that's reasonable. This CL shows an approach to improve our test coverage for behavior variations. We introduce two new methods to [Expect] that allow us to conditionally check that a function throws, depending on variation predicates. The CL changes expectations for errors that don't occur when dart2js omits parameter type checks or implicit downcasts. Note: originally I had the intention to introduce a name parameter to `Expect.throws` and `Expect.throwsTypeError` to avoid introducing a new API. However, because these APIs are used for testing core language features, such as function parameters themselves, we decided to keep the use of features in these APIs as simple as it can be. CoreLibraryReviewExempt: no public library semantic change - only improving test coverage under variations Change-Id: I531657622655778491eaca8b37ba69ffaab559fc Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/351340 Reviewed-by: Lasse Nielsen Commit-Queue: Sigmund Cherem --- pkg/expect/lib/expect.dart | 24 ++++++++++++ tests/corelib/apply_test.dart | 7 +--- tests/corelib/int_modulo_arith_test.dart | 6 +-- tests/corelib/list_removeat_test.dart | 19 ++++++---- tests/corelib/string_split_test.dart | 7 ---- tests/language/covariant/field_test.dart | 13 +++---- tests/language/covariant/method_test.dart | 15 +++----- .../function/apply_generic2_test.dart | 16 +++----- tests/language/function/type2_test.dart | 6 +-- .../type_call_getter2_runtime_test.dart | 38 +++++++++---------- tests/language/function/type_test.dart | 4 +- .../function_subtype/bound_closure7_test.dart | 12 ++---- .../function_subtype/checked0_test.dart | 6 +-- .../this/as_covariant_call_checks_test.dart | 9 ++--- 14 files changed, 83 insertions(+), 99 deletions(-) diff --git a/pkg/expect/lib/expect.dart b/pkg/expect/lib/expect.dart index 9bb522b6f70..477f4c17d4b 100644 --- a/pkg/expect/lib/expect.dart +++ b/pkg/expect/lib/expect.dart @@ -4,6 +4,9 @@ /// This library contains an Expect class with static methods that can be used /// for simple unit-tests. +/// +/// This library is deliberately simple and uses very few language features. +/// This makes it safer to use for writing the language test suite. library expect; /// Whether the program is running without sound null safety. @@ -606,6 +609,22 @@ class Expect { _fail('Expect.throws$msg fails: Did not throw'); } + /// Calls [computation] and checks that it throws an [E] when [condition] is + /// `true`. + /// + /// If [condition] is `true`, the test succeeds if an [E] is thrown, and then + /// that error is returned. The test fails if nothing is thrown or a different + /// error is thrown. + /// If [condition] is `false`, the test succeeds if nothing is thrown, + /// returning `null`, and fails if anything is thrown. + static E? throwsWhen( + bool condition, void Function() computation, + [String? reason]) { + if (condition) return throws(computation, null, reason); + computation(); + return null; + } + static ArgumentError throwsArgumentError(void Function() f, [String reason = "ArgumentError"]) => Expect.throws(f, _defaultCheck, reason); @@ -639,6 +658,11 @@ class Expect { [String reason = "TypeError"]) => Expect.throws(f, _defaultCheck, reason); + /// Checks that [f] throws a [TypeError] if an only if [condition] is `true`. + static TypeError? throwsTypeErrorWhen(bool condition, void Function() f, + [String? reason]) => + Expect.throwsWhen(condition, f, reason); + static UnsupportedError throwsUnsupportedError(void Function() f, [String reason = "UnsupportedError"]) => Expect.throws(f, _defaultCheck, reason); diff --git a/tests/corelib/apply_test.dart b/tests/corelib/apply_test.dart index 2c3e2aa3214..5bad3e4a6d4 100644 --- a/tests/corelib/apply_test.dart +++ b/tests/corelib/apply_test.dart @@ -73,11 +73,8 @@ main() { // Test that apply works on callable objects when it is passed to a method // that expects Function (and not dynamic). - if (v.checkedImplicitDowncasts) { - Expect.throws(() => testList(42, new Callable(), [13, 29])); - } else { - testList(42, new Callable(), [13, 29]); - } + Expect.throwsWhen( + v.checkedImplicitDowncasts, () => testList(42, new Callable(), [13, 29])); testListTyped(42, new Callable(), [13, 29]); } diff --git a/tests/corelib/int_modulo_arith_test.dart b/tests/corelib/int_modulo_arith_test.dart index 234ed518682..5007d2be223 100644 --- a/tests/corelib/int_modulo_arith_test.dart +++ b/tests/corelib/int_modulo_arith_test.dart @@ -125,11 +125,7 @@ testGcd() { // Test that gcd of value and other (non-negative) throws. testThrows(value, other) { callCombos(value, other, (a, b) { - if (v.checkedParameters || a is! int) { - Expect.throws(() => a.gcd(b)); - } else { - a.gcd(b); - } + Expect.throwsWhen(v.checkedParameters || a is! int, () => a.gcd(b)); }); } diff --git a/tests/corelib/list_removeat_test.dart b/tests/corelib/list_removeat_test.dart index 031ca309f34..4e9be3ae03e 100644 --- a/tests/corelib/list_removeat_test.dart +++ b/tests/corelib/list_removeat_test.dart @@ -26,14 +26,6 @@ void testModifiableList(l1) { // Index must be integer and in range. Expect.throwsRangeError(() => l1.removeAt(-1), "negative"); Expect.throwsRangeError(() => l1.removeAt(5), "too large"); - if (v.checkedParameters) { - Expect.throws( - () => l1.removeAt(null), - // With sound null safety a TypeError is thrown. - // Without sound null safety an ArgumentError is thrown. - (e) => e is TypeError || e is ArgumentError, - "is null"); - } Expect.equals(2, l1.removeAt(2), "l1-remove2"); Expect.equals(1, l1[1], "l1-1[1]"); @@ -47,6 +39,17 @@ void testModifiableList(l1) { Expect.equals(3, l1[1], "l1-2[1]"); Expect.equals(4, l1[2], "l1-2[2]"); Expect.equals(3, l1.length, "length-2"); + + // Note: this is the last expectation because, when `!v.checkedParameters` + // this ends up modifying [l1]. + final e = Expect.throwsWhen( + v.checkedParameters, () => l1.removeAt(null), "index is null"); + if (e != null) { + Expect.equals(!v.unsoundNullSafety, e is TypeError, + "TypeError expected in sound null safety"); + Expect.equals(v.unsoundNullSafety, e is ArgumentError, + "ArgumentError expected in unsound null safety"); + } } void main() { diff --git a/tests/corelib/string_split_test.dart b/tests/corelib/string_split_test.dart index 1c8a294abf7..a0f91896fb1 100644 --- a/tests/corelib/string_split_test.dart +++ b/tests/corelib/string_split_test.dart @@ -25,13 +25,6 @@ testSplit(List expect, String string, Pattern pattern) { // Ensure that the correct type is reified. actual = actual as List; - // Check that store of the wrong type throws. Don't test on configurations - // that don't perform type checks. - if (v.checkedParameters) { - Expect.throwsTypeError( - () => actual.add(42), 'List.add should not accept an int'); - } - Expect.listEquals(expect, actual, '"$string".split($patternString)'); } diff --git a/tests/language/covariant/field_test.dart b/tests/language/covariant/field_test.dart index 7da5cc3c5fc..14267eeb882 100644 --- a/tests/language/covariant/field_test.dart +++ b/tests/language/covariant/field_test.dart @@ -38,25 +38,22 @@ class C extends B { } main() { - // TODO(sigmund): replace with a Requirement comment when available. - if (!v.checkedParameters) return; - // Dynamic method calls should always have their arguments type checked. dynamic d = new C(); - Expect.throwsTypeError(() => d.s1 = new Object()); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => d.s1 = new Object()); // Interface calls should have any arguments marked "genericCovariantImpl" // type checked provided that the corresponding argument on the interface // target is marked "genericCovariantInterface". B b = new C(); - Expect.throwsTypeError(() => b.s2 = new Object()); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => b.s2 = new Object()); // Interface calls should have any arguments marked "covariant" type checked, // regardless of whether the corresponding argument on the interface target is // marked "genericCovariantInterface". - Expect.throwsTypeError(() => b.s3 = new Object()); - Expect.throwsTypeError(() => b.s4 = new Object()); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => b.s3 = new Object()); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => b.s4 = new Object()); // This calls should have any arguments marked "covariant" type checked. - Expect.throwsTypeError(() => b.s5 = new Object()); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => b.s5 = new Object()); } diff --git a/tests/language/covariant/method_test.dart b/tests/language/covariant/method_test.dart index 96196bd4b55..fe6196d411e 100644 --- a/tests/language/covariant/method_test.dart +++ b/tests/language/covariant/method_test.dart @@ -38,30 +38,27 @@ class C extends B { } main() { - // TODO(sigmund): replace with a Requirement comment when available. - if (!v.checkedParameters) return; - // Dynamic method calls should always have their arguments type checked. dynamic d = new C(); - Expect.throwsTypeError(() => d.f1(new Object())); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => d.f1(new Object())); // Closure calls should have any arguments marked "genericCovariantImpl" type // checked. B b = new C(); void Function(Object) f = b.f2; - Expect.throwsTypeError(() => f(new Object())); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => f(new Object())); // Interface calls should have any arguments marked "genericCovariantImpl" // type checked provided that the corresponding argument on the interface // target is marked "genericCovariantInterface". - Expect.throwsTypeError(() => b.f2(new Object())); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => b.f2(new Object())); // Interface calls should have any arguments marked "covariant" type checked, // regardless of whether the corresponding argument on the interface target is // marked "genericCovariantInterface". - Expect.throwsTypeError(() => b.f3(new Object())); - Expect.throwsTypeError(() => b.f4(new Object())); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => b.f3(new Object())); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => b.f4(new Object())); // This calls should have any arguments marked "covariant" type checked. - Expect.throwsTypeError(() => b.f5(new Object())); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => b.f5(new Object())); } diff --git a/tests/language/function/apply_generic2_test.dart b/tests/language/function/apply_generic2_test.dart index 5d90b9cd7d8..51748ccc79b 100644 --- a/tests/language/function/apply_generic2_test.dart +++ b/tests/language/function/apply_generic2_test.dart @@ -30,10 +30,8 @@ main() { check('[null, 33, null, 11, 22, null]', Function.apply(new CCC().memberFn, [], {#a4: 11, #a5: 22, #a2: 33})); - if (v.checkedParameters) { - Expect.throwsTypeError( - () => Function.apply(new CCC().memberFn, [], {#a3: 'hi'})); - } + Expect.throwsTypeErrorWhen(v.checkedParameters, + () => Function.apply(new CCC().memberFn, [], {#a3: 'hi'})); check('[11, 22, 33, null, null]', Function.apply(makeFn(), [], {#a1: 11, #a2: 22, #a3: 33})); @@ -41,14 +39,12 @@ main() { check('[null, 33, null, 11, 22]', Function.apply(makeFn(), [], {#a4: 11, #a5: 22, #a2: 33})); - if (v.checkedParameters) { - Expect.throwsTypeError(() => Function.apply(makeFn(), [], {#a3: 'hi'})); - } + Expect.throwsTypeErrorWhen( + v.checkedParameters, () => Function.apply(makeFn(), [], {#a3: 'hi'})); check('[null, 33, null, 11, 22, null]', Function.apply(staticFn, [], {#a4: 11, #a5: 22, #a2: 33})); - if (v.checkedParameters) { - Expect.throwsTypeError(() => Function.apply(staticFn, [], {#a3: 'hi'})); - } + Expect.throwsTypeErrorWhen( + v.checkedParameters, () => Function.apply(staticFn, [], {#a3: 'hi'})); } diff --git a/tests/language/function/type2_test.dart b/tests/language/function/type2_test.dart index e62801557c7..06532e0c7e8 100644 --- a/tests/language/function/type2_test.dart +++ b/tests/language/function/type2_test.dart @@ -16,9 +16,5 @@ class B extends A { } main() { - if (v.checkedParameters) { - Expect.throwsTypeError(() => new B()); - } else { - new B(); - } + Expect.throwsTypeErrorWhen(v.checkedParameters, () => new B()); } diff --git a/tests/language/function/type_call_getter2_runtime_test.dart b/tests/language/function/type_call_getter2_runtime_test.dart index 7aebf1db88e..8eba2dc1caa 100644 --- a/tests/language/function/type_call_getter2_runtime_test.dart +++ b/tests/language/function/type_call_getter2_runtime_test.dart @@ -24,29 +24,27 @@ main() { B b = new B(); C c = new C(); - if (v.checkedImplicitDowncasts) { - Expect.throwsTypeError(() { - Function a1 = a as dynamic; - }); + Expect.throwsTypeErrorWhen(v.checkedImplicitDowncasts, () { + Function a1 = a as dynamic; + }); - Expect.throwsTypeError(() { - F a2 = a as dynamic; - }); + Expect.throwsTypeErrorWhen(v.checkedImplicitDowncasts, () { + F a2 = a as dynamic; + }); - Expect.throwsTypeError(() { - Function b1 = b as dynamic; - }); + Expect.throwsTypeErrorWhen(v.checkedImplicitDowncasts, () { + Function b1 = b as dynamic; + }); - Expect.throwsTypeError(() { - F b2 = b as dynamic; - }); + Expect.throwsTypeErrorWhen(v.checkedImplicitDowncasts, () { + F b2 = b as dynamic; + }); - Expect.throwsTypeError(() { - Function c1 = c as dynamic; - }); + Expect.throwsTypeErrorWhen(v.checkedImplicitDowncasts, () { + Function c1 = c as dynamic; + }); - Expect.throwsTypeError(() { - F c2 = c as dynamic; - }); - } + Expect.throwsTypeErrorWhen(v.checkedImplicitDowncasts, () { + F c2 = c as dynamic; + }); } diff --git a/tests/language/function/type_test.dart b/tests/language/function/type_test.dart index 8c7459c3ee0..eda5791c064 100644 --- a/tests/language/function/type_test.dart +++ b/tests/language/function/type_test.dart @@ -9,9 +9,7 @@ import "package:expect/variations.dart" as v; typedef FListInt(List l); main() { - // TODO(sigmund): replace with a Requirement comment when available. - if (!v.checkedImplicitDowncasts) return; - Expect.throwsTypeError(() { + Expect.throwsTypeErrorWhen(v.checkedImplicitDowncasts, () { // Static result type of f(), i.e. FList, is a subtype of FListInt. // However, run time type of returned function is not a subtype of FListInt. // Run time type check should not be eliminated. diff --git a/tests/language/function_subtype/bound_closure7_test.dart b/tests/language/function_subtype/bound_closure7_test.dart index 2b0deca22a6..90079568b8c 100644 --- a/tests/language/function_subtype/bound_closure7_test.dart +++ b/tests/language/function_subtype/bound_closure7_test.dart @@ -24,18 +24,14 @@ void main() { Expect.isTrue(f is Foo>); Expect.isFalse(f is Foo); Expect.isFalse(f is Foo); - if (v.checkedParameters) { - Expect.throwsTypeError(() => f(f)); - Expect.throwsTypeError(() => f(42)); - } + Expect.throwsTypeErrorWhen(v.checkedParameters, () => f(f)); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => f(42)); Foo> bazInt = baz; // implicit instantiation baz f = bazInt; Expect.isTrue(f(bar)); Expect.isFalse(f is Foo); - if (v.checkedParameters) { - Expect.throwsTypeError(() => f(f)); - Expect.throwsTypeError(() => f(42)); - } + Expect.throwsTypeErrorWhen(v.checkedParameters, () => f(f)); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => f(42)); } diff --git a/tests/language/function_subtype/checked0_test.dart b/tests/language/function_subtype/checked0_test.dart index 775e4c32ab3..50f9c567204 100644 --- a/tests/language/function_subtype/checked0_test.dart +++ b/tests/language/function_subtype/checked0_test.dart @@ -30,11 +30,7 @@ class C { void test(String nameOfT, bool expectedResult) { check(bool expectedResult, f()) { - if (!expectedResult) { - if (v.checkedParameters) Expect.throwsTypeError(f); - } else { - f(); - } + Expect.throwsTypeErrorWhen(!expectedResult && v.checkedParameters, f); } dynamic foo = fooF, baz = bazF, boz = bozF; diff --git a/tests/language/this/as_covariant_call_checks_test.dart b/tests/language/this/as_covariant_call_checks_test.dart index 37844c9c6b7..1296554ec59 100644 --- a/tests/language/this/as_covariant_call_checks_test.dart +++ b/tests/language/this/as_covariant_call_checks_test.dart @@ -45,14 +45,11 @@ void loop(A obj, bool violateType) { } void main() { - // TODO(sigmund): update to use a Requirements comment instead - if (!v.checkedParameters) return; - A().field = 10; final obj = A(); loop(obj, false); loop(obj, false); - Expect.throwsTypeError(() => obj.testMethod(true)); - Expect.throwsTypeError(() => obj.testSetter(true)); - Expect.throwsTypeError(() => obj.testField(true)); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => obj.testMethod(true)); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => obj.testSetter(true)); + Expect.throwsTypeErrorWhen(v.checkedParameters, () => obj.testField(true)); }