From cc924424c157ed99cd0ac66c9597f59e32eff13b Mon Sep 17 00:00:00 2001 From: cpovirk Date: Mon, 17 Apr 2023 11:37:28 -0700 Subject: [PATCH] Disable `NullPointerTester` when `com.google.common` is being tested under Android VMs. As we already started to see in cl/522315614, it can't work there with type-use annotations. So, when more type-use annotations appear in cl/522310996, we'd see more failures. `NullPointerTester` will continue to run when guava-android is tested _under a JRE_, including in our open-source tests. All that this CL affects is the additional Android-emulator tests that we run on Google infrastructure. And while it's _nice_ to run all our tests under an emulator, their _primary_ purpose is to check for usages of APIs that aren't available under old versions of Android, not to look for differences in null-handling behavior between Android and JRE (which is possible but relatively unlikely). A more principled approach would be to edit every test that _uses_ `NullPointerTester` to skip the test under Android VMs. However, this would require editing 100+ files. And it would actually require editing even _more_: In every package that we define a `PackageSanityTests` class, `NullPointerTester` runs automatically on every class `Foo` in the package _unless_ `AbstractPackageSanityTests` finds that we've defined a method like `FooTest.testNulls()`. Thus, we'd likely have to define dozens more such methods. And those new methods may be not completely trivial in some cases, if I'm remembering how much `PackageSanityTests` does automatically (to, e.g., construct instances of the class to test against). Finally, if it helps, we did something similar to silently disable `SerializableTester` under GWT way back in cl/25672151, though I'm not sure I'd stand by that one today, since we could have modified _that_ class's users to skip the testing under GWT more easily :) In a fun twist, we have to keep the `@AndroidIncompatible` annotations in `ClassSanityTesterTest` and `NullPointerTesterTest`: - Before, those tests failed because they didn't see type-use `@Nullable` annotations and thus tried to pass `null` where it wasn't valid. - Now, those tests fail because they try to make `NullPointerTester` fail on badly annotated classes, but it doesn't fail because it's doesn't run at all! RELNOTES=n/a PiperOrigin-RevId: 524904788 --- .../com/google/common/testing/NullPointerTester.java | 12 ++++++++++++ .../google/common/testing/ClassSanityTesterTest.java | 2 +- .../google/common/testing/NullPointerTesterTest.java | 2 +- .../com/google/common/testing/NullPointerTester.java | 12 ++++++++++++ .../google/common/testing/ClassSanityTesterTest.java | 2 +- .../google/common/testing/NullPointerTesterTest.java | 2 +- 6 files changed, 28 insertions(+), 4 deletions(-) diff --git a/android/guava-testlib/src/com/google/common/testing/NullPointerTester.java b/android/guava-testlib/src/com/google/common/testing/NullPointerTester.java index 82b761bdcd11..413ba66cedeb 100644 --- a/android/guava-testlib/src/com/google/common/testing/NullPointerTester.java +++ b/android/guava-testlib/src/com/google/common/testing/NullPointerTester.java @@ -349,6 +349,13 @@ public int hashCode() { */ private void testParameter( Object instance, Invokable invokable, int paramIndex, Class testedClass) { + /* + * com.google.common is starting to rely on type-use annotations, which aren't visible under + * Android VMs. So we skip testing there. + */ + if (isAndroid() && Reflection.getPackageName(testedClass).startsWith("com.google.common")) { + return; + } if (isPrimitiveOrNullable(invokable.getParameters().get(paramIndex))) { return; // there's nothing to test } @@ -654,4 +661,9 @@ boolean isNullable(Parameter param) { abstract boolean isNullable(Parameter param); } + + private static boolean isAndroid() { + // Arguably it would make more sense to test "can we see type-use annotations" directly.... + return System.getProperty("java.runtime.name").contains("Android"); + } } diff --git a/android/guava-testlib/test/com/google/common/testing/ClassSanityTesterTest.java b/android/guava-testlib/test/com/google/common/testing/ClassSanityTesterTest.java index 3c8730516533..fee11890b7db 100644 --- a/android/guava-testlib/test/com/google/common/testing/ClassSanityTesterTest.java +++ b/android/guava-testlib/test/com/google/common/testing/ClassSanityTesterTest.java @@ -43,7 +43,7 @@ * * @author Ben Yu */ -@AndroidIncompatible // Android doesn't support the type-use annotations it needs +@AndroidIncompatible // NullPointerTester refuses to run for c.g.c under Android public class ClassSanityTesterTest extends TestCase { private final ClassSanityTester tester = new ClassSanityTester(); diff --git a/android/guava-testlib/test/com/google/common/testing/NullPointerTesterTest.java b/android/guava-testlib/test/com/google/common/testing/NullPointerTesterTest.java index 4db9310d2a50..d7344abf66dd 100644 --- a/android/guava-testlib/test/com/google/common/testing/NullPointerTesterTest.java +++ b/android/guava-testlib/test/com/google/common/testing/NullPointerTesterTest.java @@ -56,7 +56,7 @@ * @author Mick Killianey */ @SuppressWarnings("CheckReturnValue") -@AndroidIncompatible // Android doesn't support the type-use annotations it needs +@AndroidIncompatible // NullPointerTester refuses to run for c.g.c under Android public class NullPointerTesterTest extends TestCase { /** Non-NPE RuntimeException. */ diff --git a/guava-testlib/src/com/google/common/testing/NullPointerTester.java b/guava-testlib/src/com/google/common/testing/NullPointerTester.java index 5780dd721d1c..87349a8d882d 100644 --- a/guava-testlib/src/com/google/common/testing/NullPointerTester.java +++ b/guava-testlib/src/com/google/common/testing/NullPointerTester.java @@ -349,6 +349,13 @@ public int hashCode() { */ private void testParameter( Object instance, Invokable invokable, int paramIndex, Class testedClass) { + /* + * com.google.common is starting to rely on type-use annotations, which aren't visible under + * Android VMs. So we skip testing there. + */ + if (isAndroid() && Reflection.getPackageName(testedClass).startsWith("com.google.common")) { + return; + } if (isPrimitiveOrNullable(invokable.getParameters().get(paramIndex))) { return; // there's nothing to test } @@ -654,4 +661,9 @@ boolean isNullable(Parameter param) { abstract boolean isNullable(Parameter param); } + + private static boolean isAndroid() { + // Arguably it would make more sense to test "can we see type-use annotations" directly.... + return System.getProperty("java.runtime.name").contains("Android"); + } } diff --git a/guava-testlib/test/com/google/common/testing/ClassSanityTesterTest.java b/guava-testlib/test/com/google/common/testing/ClassSanityTesterTest.java index be7fbe08fcc8..ac4e05fba983 100644 --- a/guava-testlib/test/com/google/common/testing/ClassSanityTesterTest.java +++ b/guava-testlib/test/com/google/common/testing/ClassSanityTesterTest.java @@ -45,7 +45,7 @@ * * @author Ben Yu */ -@AndroidIncompatible // Android doesn't support the type-use annotations it needs +@AndroidIncompatible // NullPointerTester refuses to run for c.g.c under Android public class ClassSanityTesterTest extends TestCase { private final ClassSanityTester tester = new ClassSanityTester(); diff --git a/guava-testlib/test/com/google/common/testing/NullPointerTesterTest.java b/guava-testlib/test/com/google/common/testing/NullPointerTesterTest.java index 9c3d18d96712..050f05f6d97e 100644 --- a/guava-testlib/test/com/google/common/testing/NullPointerTesterTest.java +++ b/guava-testlib/test/com/google/common/testing/NullPointerTesterTest.java @@ -56,7 +56,7 @@ * @author Mick Killianey */ @SuppressWarnings("CheckReturnValue") -@AndroidIncompatible // Android doesn't support the type-use annotations it needs +@AndroidIncompatible // NullPointerTester refuses to run for c.g.c under Android public class NullPointerTesterTest extends TestCase { /** Non-NPE RuntimeException. */