Skip to content

Commit

Permalink
Use assertThrows even in GWT/J2CL/J2KT-compatible code.
Browse files Browse the repository at this point in the history 
...by actually using our own homegrown versions that avoid reflection.

(followup to cl/563137030 + cl/570474348)

Also, suppress/address a few other warnings.

And incidentally, largely rewrite `ThrowablesTest`. We're allowed to learn a thing or two about writing tests over the course of 17+ years :) (I'd specifically highlight the old `testPropagateIfInstanceOf_uncheckedThrown` test, which has been replaced with the similarly `testPropagateIfInstanceOf_unchecked`: It might have appeared to have been a test that `propagateIfInstanceOf` would throw the given unchecked exception, but in fact what it actually shows is that _`propagate`_ does so. `propagateIfInstanceOf` does not and should not. Additionally, I think that my new `testPropagateIfPossible_twoDeclared_secondSame` covers a case that wasn't covered before.)

Future work:
- Perform this migration for our other packages.
- Generalize AssertThrowsMultipleStatements to cover our `assertThrows` methods.
- Maybe migrate non-GWT/J2CL/J2KT calls of `assertThrows` to our methods, too.

This migration should save us from warnings from the recent changes to the EmptyCatch check in cl/674409794.

The approach used in this change (of declaring both `ThrowingRunnable` and `ThrowingSupplier` overloads of `assertThrows`) saves us from needing to update our CheckReturnValue enforcement to recognize the new `assertThrows` variants. Note also that it lets us provide a slightly better failure message, as discussed back in junit-team/junit5#1394.

RELNOTES=n/a
PiperOrigin-RevId: 675634517
  • Loading branch information
@cpovirk
cpovirk authored and Google Java Core Libraries committed Sep 17, 2024
1 parent 294f07d commit e5b58e2
Show file tree
Hide file tree
Showing 32 changed files with 1,075 additions and 2,032 deletions.
56 changes: 12 additions & 44 deletions android/guava-tests/test/com/google/common/base/AbstractIteratorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,9 +16,13 @@

package com.google.common.base;

import static com.google.common.base.ReflectionFreeAssertThrows.assertThrows;

import com.google.common.annotations.GwtCompatible;
import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
import com.google.common.base.TestExceptions.SomeCheckedException;
import com.google.common.base.TestExceptions.SomeUncheckedException;
import com.google.common.testing.GcFinalization;
import java.lang.ref.WeakReference;
import java.util.Iterator;
Expand Down Expand Up @@ -70,11 +74,7 @@ public Integer computeNext() {
// Make sure computeNext() doesn't get invoked again
assertFalse(iter.hasNext());

try {
iter.next();
fail("no exception thrown");
} catch (NoSuchElementException expected) {
}
assertThrows(NoSuchElementException.class, iter::next);
}

public void testSneakyThrow() throws Exception {
Expand All @@ -95,21 +95,9 @@ public Integer computeNext() {
};

// The first time, the sneakily-thrown exception comes out
try {
iter.hasNext();
fail("No exception thrown");
} catch (Exception e) {
if (!(e instanceof SomeCheckedException)) {
throw e;
}
}

assertThrows(SomeCheckedException.class, iter::hasNext);
// But the second time, AbstractIterator itself throws an ISE
try {
iter.hasNext();
fail("No exception thrown");
} catch (IllegalStateException expected) {
}
assertThrows(IllegalStateException.class, iter::hasNext);
}

public void testException() {
Expand All @@ -123,12 +111,8 @@ public Integer computeNext() {
};

// It should pass through untouched
try {
iter.hasNext();
fail("No exception thrown");
} catch (SomeUncheckedException e) {
assertSame(exception, e);
}
SomeUncheckedException e = assertThrows(SomeUncheckedException.class, iter::hasNext);
assertSame(exception, e);
}

public void testExceptionAfterEndOfData() {
Expand All @@ -140,11 +124,7 @@ public Integer computeNext() {
throw new SomeUncheckedException();
}
};
try {
iter.hasNext();
fail("No exception thrown");
} catch (SomeUncheckedException expected) {
}
assertThrows(SomeUncheckedException.class, iter::hasNext);
}

public void testCantRemove() {
Expand All @@ -164,11 +144,7 @@ public Integer computeNext() {

assertEquals(0, (int) iter.next());

try {
iter.remove();
fail("No exception thrown");
} catch (UnsupportedOperationException expected) {
}
assertThrows(UnsupportedOperationException.class, iter::remove);
}


Expand Down Expand Up @@ -196,11 +172,7 @@ protected Integer computeNext() {
throw new AssertionError();
}
};
try {
iter.hasNext();
fail();
} catch (IllegalStateException expected) {
}
assertThrows(IllegalStateException.class, iter::hasNext);
}

// Technically we should test other reentrant scenarios (4 combinations of
Expand All @@ -217,8 +189,4 @@ void throwIt(Throwable t) throws T {
}
new SneakyThrower<Error>().throwIt(t);
}

private static class SomeCheckedException extends Exception {}

private static class SomeUncheckedException extends RuntimeException {}
}
26 changes: 6 additions & 20 deletions android/guava-tests/test/com/google/common/base/AsciiTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.google.common.base;

import static com.google.common.base.ReflectionFreeAssertThrows.assertThrows;

import com.google.common.annotations.GwtCompatible;
import com.google.common.annotations.GwtIncompatible;
import junit.framework.TestCase;
Expand Down Expand Up @@ -98,29 +100,13 @@ public void testTruncate() {
}

public void testTruncateIllegalArguments() {
try {
Ascii.truncate("foobar", 2, "...");
fail();
} catch (IllegalArgumentException expected) {
}
assertThrows(IllegalArgumentException.class, () -> Ascii.truncate("foobar", 2, "..."));

try {
Ascii.truncate("foobar", 8, "1234567890");
fail();
} catch (IllegalArgumentException expected) {
}
assertThrows(IllegalArgumentException.class, () -> Ascii.truncate("foobar", 8, "1234567890"));

try {
Ascii.truncate("foobar", -1, "...");
fail();
} catch (IllegalArgumentException expected) {
}
assertThrows(IllegalArgumentException.class, () -> Ascii.truncate("foobar", -1, "..."));

try {
Ascii.truncate("foobar", -1, "");
fail();
} catch (IllegalArgumentException expected) {
}
assertThrows(IllegalArgumentException.class, () -> Ascii.truncate("foobar", -1, ""));
}

public void testEqualsIgnoreCase() {
Expand Down
26 changes: 6 additions & 20 deletions android/guava-tests/test/com/google/common/base/FunctionsTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@

package com.google.common.base;

import static com.google.common.base.ReflectionFreeAssertThrows.assertThrows;

import com.google.common.annotations.GwtCompatible;
import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
Expand Down Expand Up @@ -70,11 +72,7 @@ public String toString() {
return "I'm a string";
}
}));
try {
Functions.toStringFunction().apply(null);
fail("expected NullPointerException");
} catch (NullPointerException expected) {
}
assertThrows(NullPointerException.class, () -> Functions.toStringFunction().apply(null));
}

@J2ktIncompatible
Expand All @@ -101,11 +99,7 @@ public void testForMapWithoutDefault() {
assertEquals(3, function.apply("Three").intValue());
assertNull(function.apply("Null"));

try {
function.apply("Two");
fail();
} catch (IllegalArgumentException expected) {
}
assertThrows(IllegalArgumentException.class, () -> function.apply("Two"));

new EqualsTester()
.addEqualityGroup(function, Functions.forMap(map))
Expand Down Expand Up @@ -225,17 +219,9 @@ public void testComposition() {
Functions.compose(integerToSpanish, japaneseToInteger);

assertEquals("Uno", japaneseToSpanish.apply("Ichi"));
try {
japaneseToSpanish.apply("Ni");
fail();
} catch (IllegalArgumentException e) {
}
assertThrows(IllegalArgumentException.class, () -> japaneseToSpanish.apply("Ni"));
assertEquals("Tres", japaneseToSpanish.apply("San"));
try {
japaneseToSpanish.apply("Shi");
fail();
} catch (IllegalArgumentException e) {
}
assertThrows(IllegalArgumentException.class, () -> japaneseToSpanish.apply("Shi"));

new EqualsTester()
.addEqualityGroup(japaneseToSpanish, Functions.compose(integerToSpanish, japaneseToInteger))
Expand Down
101 changes: 14 additions & 87 deletions android/guava-tests/test/com/google/common/base/JoinerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -16,19 +16,19 @@

package com.google.common.base;

import static com.google.common.base.ReflectionFreeAssertThrows.assertThrows;

import com.google.common.annotations.GwtCompatible;
import com.google.common.annotations.GwtIncompatible;
import com.google.common.annotations.J2ktIncompatible;
import com.google.common.base.Joiner.MapJoiner;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableMultimap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Lists;
import com.google.common.collect.Maps;
import com.google.common.testing.NullPointerTester;
import java.io.IOException;
import java.util.Arrays;
import java.util.Iterator;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Set;
Expand Down Expand Up @@ -60,33 +60,18 @@ public class JoinerTest extends TestCase {
private static final Iterable<@Nullable Integer> ITERABLE_FOUR_NULLS =
Arrays.asList((Integer) null, null, null, null);

@SuppressWarnings("JoinIterableIterator") // explicitly testing iterator overload, too
public void testNoSpecialNullBehavior() {
checkNoOutput(J, ITERABLE_);
checkResult(J, ITERABLE_1, "1");
checkResult(J, ITERABLE_12, "1-2");
checkResult(J, ITERABLE_123, "1-2-3");

try {
J.join(ITERABLE_NULL);
fail();
} catch (NullPointerException expected) {
}
try {
J.join(ITERABLE_1_NULL_2);
fail();
} catch (NullPointerException expected) {
}
assertThrows(NullPointerException.class, () -> J.join(ITERABLE_NULL));
assertThrows(NullPointerException.class, () -> J.join(ITERABLE_1_NULL_2));

try {
J.join(ITERABLE_NULL.iterator());
fail();
} catch (NullPointerException expected) {
}
try {
J.join(ITERABLE_1_NULL_2.iterator());
fail();
} catch (NullPointerException expected) {
}
assertThrows(NullPointerException.class, () -> J.join(ITERABLE_NULL.iterator()));
assertThrows(NullPointerException.class, () -> J.join(ITERABLE_1_NULL_2.iterator()));
}

public void testOnCharOverride() {
Expand Down Expand Up @@ -218,29 +203,17 @@ private static void checkResult(Joiner joiner, Iterable<Integer> parts, String e

public void test_useForNull_skipNulls() {
Joiner j = Joiner.on("x").useForNull("y");
try {
j = j.skipNulls();
fail();
} catch (UnsupportedOperationException expected) {
}
assertThrows(UnsupportedOperationException.class, j::skipNulls);
}

public void test_skipNulls_useForNull() {
Joiner j = Joiner.on("x").skipNulls();
try {
j = j.useForNull("y");
fail();
} catch (UnsupportedOperationException expected) {
}
assertThrows(UnsupportedOperationException.class, () -> j.useForNull("y"));
}

public void test_useForNull_twice() {
Joiner j = Joiner.on("x").useForNull("y");
try {
j = j.useForNull("y");
fail();
} catch (UnsupportedOperationException expected) {
}
assertThrows(UnsupportedOperationException.class, () -> j.useForNull("y"));
}

public void testMap() {
Expand All @@ -252,11 +225,7 @@ public void testMap() {
mapWithNulls.put("a", null);
mapWithNulls.put(null, "b");

try {
j.join(mapWithNulls);
fail();
} catch (NullPointerException expected) {
}
assertThrows(NullPointerException.class, () -> j.join(mapWithNulls));

assertEquals("a:00;00:b", j.useForNull("00").join(mapWithNulls));

Expand All @@ -279,17 +248,9 @@ public void testEntries() {
mapWithNulls.put(null, "b");
Set<Entry<String, String>> entriesWithNulls = mapWithNulls.entrySet();

try {
j.join(entriesWithNulls);
fail();
} catch (NullPointerException expected) {
}
assertThrows(NullPointerException.class, () -> j.join(entriesWithNulls));

try {
j.join(entriesWithNulls.iterator());
fail();
} catch (NullPointerException expected) {
}
assertThrows(NullPointerException.class, () -> j.join(entriesWithNulls.iterator()));

assertEquals("a:00;00:b", j.useForNull("00").join(entriesWithNulls));
assertEquals("a:00;00:b", j.useForNull("00").join(entriesWithNulls.iterator()));
Expand All @@ -305,11 +266,7 @@ public void testEntries() {

public void test_skipNulls_onMap() {
Joiner j = Joiner.on(",").skipNulls();
try {
j.withKeyValueSeparator("/");
fail();
} catch (UnsupportedOperationException expected) {
}
assertThrows(UnsupportedOperationException.class, () -> j.withKeyValueSeparator("/"));
}

private static class DontStringMeBro implements CharSequence {
Expand All @@ -334,36 +291,6 @@ public String toString() {
}
}

// Don't do this.
private static class IterableIterator implements Iterable<Integer>, Iterator<Integer> {
private static final ImmutableSet<Integer> INTEGERS = ImmutableSet.of(1, 2, 3, 4);
private final Iterator<Integer> iterator;

public IterableIterator() {
this.iterator = iterator();
}

@Override
public Iterator<Integer> iterator() {
return INTEGERS.iterator();
}

@Override
public boolean hasNext() {
return iterator.hasNext();
}

@Override
public Integer next() {
return iterator.next();
}

@Override
public void remove() {
iterator.remove();
}
}

@GwtIncompatible // StringBuilder.append in GWT invokes Object.toString(), unlike the JRE version.
public void testDontConvertCharSequenceToString() {
assertEquals("foo,foo", Joiner.on(",").join(new DontStringMeBro(), new DontStringMeBro()));
Expand Down
Loading

0 comments on commit e5b58e2

Please sign in to comment.