Skip to content

Commit

Permalink
Produce a more helpful error for isInstanceOf(int.class) and simila…
Browse files Browse the repository at this point in the history
…r, and make analogous `isNotInstanceOf` calls fail.

Users should write `isInstanceOf(Integer.class)` instead.

As always, the biggest danger is in `isNotInstanceOf` assertions, which currently silently pass for any input if given a primitive type.

If we wanted, we could automatically treat `int.class` like `Integer.class`. But I think we can get away with being stricter, and I think it's clearest to have users write the boxed type: It's not as if Truth can tell whether its input was "originally" an `int` or not.

(As a followup, I was considering also rejecting `Void.class`. But such a change seems as likely to break someone's clever "make this always pass/fail" hack than to detect real problems. Still, it may be worth a thought someday.)

Before:
```
expected instance of: int
but was instance of : java.lang.Integer
with value          : 1
```

After:
```
java.lang.IllegalArgumentException: Cannot check instanceof for primitive type int. Pass the wrapper class instead.
```
RELNOTES=n/a
PiperOrigin-RevId: 478924507
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Oct 5, 2022
1 parent fbeba69 commit 3a47110
Show file tree
Hide file tree
Showing 2 changed files with 33 additions and 2 deletions.
17 changes: 15 additions & 2 deletions core/src/main/java/com/google/common/truth/Subject.java
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
import static com.google.common.base.CaseFormat.UPPER_CAMEL;
import static com.google.common.base.CharMatcher.whitespace;
import static com.google.common.base.MoreObjects.firstNonNull;
import static com.google.common.base.Preconditions.checkArgument;
import static com.google.common.base.Strings.lenientFormat;
import static com.google.common.truth.Fact.fact;
import static com.google.common.truth.Fact.simpleFact;
Expand Down Expand Up @@ -300,7 +301,7 @@ public void isInstanceOf(Class<?> clazz) {
failWithActual("expected instance of", clazz.getName());
return;
}
if (!Platform.isInstanceOfType(actual, clazz)) {
if (!isInstanceOfType(actual, clazz)) {
if (classMetadataUnsupported()) {
throw new UnsupportedOperationException(
actualCustomStringRepresentation()
Expand Down Expand Up @@ -329,7 +330,7 @@ public void isNotInstanceOf(Class<?> clazz) {
if (actual == null) {
return; // null is not an instance of clazz.
}
if (Platform.isInstanceOfType(actual, clazz)) {
if (isInstanceOfType(actual, clazz)) {
failWithActual("expected not to be an instance of", clazz.getName());
/*
* TODO(cpovirk): Consider including actual.getClass() if it's not clazz itself but only a
Expand All @@ -338,6 +339,18 @@ public void isNotInstanceOf(Class<?> clazz) {
}
}

private static boolean isInstanceOfType(Object instance, Class<?> clazz) {
checkArgument(
!clazz.isPrimitive(),
"Cannot check instanceof for primitive type %s. Pass the wrapper class instead.",
clazz.getSimpleName());
/*
* TODO(cpovirk): Make the message include `Primitives.wrap(clazz).getSimpleName()` once that
* method is available in a public guava-gwt release that we depend on.
*/
return Platform.isInstanceOfType(instance, clazz);
}

/** Fails unless the subject is equal to any element in the given iterable. */
public void isIn(Iterable<?> iterable) {
if (!Iterables.contains(iterable, actual)) {
Expand Down
18 changes: 18 additions & 0 deletions core/src/test/java/com/google/common/truth/SubjectTest.java
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,15 @@ public void isInstanceOfInterfaceForNull() {
expectFailure.whenTesting().that((Object) null).isInstanceOf(CharSequence.class);
}

@Test
public void isInstanceOfPrimitiveType() {
try {
assertThat(1).isInstanceOf(int.class);
fail();
} catch (IllegalArgumentException expected) {
}
}

@Test
public void isNotInstanceOfUnrelatedClass() {
assertThat("a").isNotInstanceOf(Long.class);
Expand Down Expand Up @@ -598,6 +607,15 @@ public void isNotInstanceOfImplementedInterface() {
expectFailure.whenTesting().that("a").isNotInstanceOf(CharSequence.class);
}

@Test
public void isNotInstanceOfPrimitiveType() {
try {
assertThat(1).isNotInstanceOf(int.class);
fail();
} catch (IllegalArgumentException expected) {
}
}

@Test
public void isIn() {
assertThat("b").isIn(oneShotIterable("a", "b", "c"));
Expand Down

0 comments on commit 3a47110

Please sign in to comment.