Skip to content

Commit

Permalink
Disable NullPointerTester when com.google.common is being tested …
Browse files Browse the repository at this point in the history
…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
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Apr 17, 2023
1 parent 3f039fc commit cc92442
Show file tree
Hide file tree
Showing 6 changed files with 28 additions and 4 deletions.
Expand Up @@ -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
}
Expand Down Expand Up @@ -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");
}
}
Expand Up @@ -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();
Expand Down
Expand Up @@ -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. */
Expand Down
12 changes: 12 additions & 0 deletions guava-testlib/src/com/google/common/testing/NullPointerTester.java
Expand Up @@ -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
}
Expand Down Expand Up @@ -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");
}
}
Expand Up @@ -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();
Expand Down
Expand Up @@ -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. */
Expand Down

0 comments on commit cc92442

Please sign in to comment.