Skip to content

Commit

Permalink
Address [CatchFail](https://errorprone.info/bugpattern/CatchFail), [S…
Browse files Browse the repository at this point in the history
…taticQualifiedUsingExpression](https://errorprone.info/bugpattern/StaticQualifiedUsingExpression), [UnusedMethod](https://errorprone.info/bugpattern/UnusedMethod), and [CacheLoaderNull](https://errorprone.info/bugpattern/CacheLoaderNull) warnings.

And use `assertThrows` in some more places where it is now obvious that that is possible.

PiperOrigin-RevId: 686721569
  • Loading branch information
cpovirk authored and Google Java Core Libraries committed Oct 17, 2024
1 parent 702e4b2 commit ca12c33
Show file tree
Hide file tree
Showing 9 changed files with 80 additions and 125 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ private static void assertGoodTesterAnnotation(Class<? extends Annotation> annot
try {
method = annotationClass.getMethod(propertyName);
} catch (NoSuchMethodException e) {
fail(
rootLocaleFormat("%s must have a property named '%s'.", annotationClass, propertyName));
throw new AssertionError("Annotation is missing required method", e);
}
final Class<?> returnType = method.getReturnType();
assertTrue(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import static com.google.common.cache.TestingCacheLoaders.identityLoader;
import static com.google.common.cache.TestingRemovalListeners.countingRemovalListener;
import static com.google.common.truth.Truth.assertThat;
import static java.lang.Thread.currentThread;
import static java.util.Arrays.asList;
import static java.util.concurrent.TimeUnit.MILLISECONDS;
import static org.junit.Assert.assertThrows;
Expand Down Expand Up @@ -75,7 +74,7 @@ public void setUp() throws Exception {
public void tearDown() throws Exception {
super.tearDown();
// TODO(cpovirk): run tests in other thread instead of messing with main thread interrupt status
currentThread().interrupted();
Thread.interrupted();
LocalCache.logger.removeHandler(logHandler);
}

Expand Down Expand Up @@ -1148,11 +1147,11 @@ public void testLoadInterruptedException() {
assertEquals(0, stats.hitCount());

// Sanity check:
assertFalse(currentThread().interrupted());
assertFalse(Thread.interrupted());

Exception expected = assertThrows(ExecutionException.class, () -> cache.get(new Object()));
assertThat(expected).hasCauseThat().isSameInstanceAs(e);
assertTrue(currentThread().interrupted());
assertTrue(Thread.interrupted());
stats = cache.stats();
assertEquals(1, stats.missCount());
assertEquals(0, stats.loadSuccessCount());
Expand All @@ -1162,15 +1161,15 @@ public void testLoadInterruptedException() {
expected =
assertThrows(UncheckedExecutionException.class, () -> cache.getUnchecked(new Object()));
assertThat(expected).hasCauseThat().isSameInstanceAs(e);
assertTrue(currentThread().interrupted());
assertTrue(Thread.interrupted());
stats = cache.stats();
assertEquals(2, stats.missCount());
assertEquals(0, stats.loadSuccessCount());
assertEquals(2, stats.loadExceptionCount());
assertEquals(0, stats.hitCount());

cache.refresh(new Object());
assertTrue(currentThread().interrupted());
assertTrue(Thread.interrupted());
checkLoggedCause(e);
stats = cache.stats();
assertEquals(2, stats.missCount());
Expand All @@ -1183,7 +1182,7 @@ public void testLoadInterruptedException() {
assertThrows(
ExecutionException.class, () -> cache.get(new Object(), throwing(callableException)));
assertThat(expected).hasCauseThat().isSameInstanceAs(callableException);
assertTrue(currentThread().interrupted());
assertTrue(Thread.interrupted());
stats = cache.stats();
assertEquals(3, stats.missCount());
assertEquals(0, stats.loadSuccessCount());
Expand All @@ -1192,7 +1191,7 @@ public void testLoadInterruptedException() {

expected = assertThrows(ExecutionException.class, () -> cache.getAll(asList(new Object())));
assertThat(expected).hasCauseThat().isSameInstanceAs(e);
assertTrue(currentThread().interrupted());
assertTrue(Thread.interrupted());
stats = cache.stats();
assertEquals(4, stats.missCount());
assertEquals(0, stats.loadSuccessCount());
Expand Down Expand Up @@ -1392,7 +1391,7 @@ public void testBulkLoadInterruptedException() {
ExecutionException expected =
assertThrows(ExecutionException.class, () -> cache.getAll(asList(new Object())));
assertThat(expected).hasCauseThat().isSameInstanceAs(e);
assertTrue(currentThread().interrupted());
assertTrue(Thread.interrupted());
stats = cache.stats();
assertEquals(1, stats.missCount());
assertEquals(0, stats.loadSuccessCount());
Expand Down Expand Up @@ -1765,31 +1764,22 @@ public void testLoadingExceptionWithCause() {
LoadingCache<Object, Object> cacheChecked =
CacheBuilder.newBuilder().build(exceptionLoader(ee));

try {
cacheUnchecked.get(new Object());
fail();
} catch (ExecutionException e) {
fail();
} catch (UncheckedExecutionException caughtEe) {
assertThat(caughtEe).hasCauseThat().isSameInstanceAs(uee);
}

UncheckedExecutionException caughtUee =
assertThrows(UncheckedExecutionException.class, () -> cacheUnchecked.get(new Object()));
assertThat(caughtUee).hasCauseThat().isSameInstanceAs(uee);

caughtUee =
assertThrows(
UncheckedExecutionException.class, () -> cacheUnchecked.getUnchecked(new Object()));
assertThat(caughtUee).hasCauseThat().isSameInstanceAs(uee);

cacheUnchecked.refresh(new Object());
checkLoggedCause(uee);

try {
cacheUnchecked.getAll(asList(new Object()));
fail();
} catch (ExecutionException e) {
fail();
} catch (UncheckedExecutionException caughtEe) {
assertThat(caughtEe).hasCauseThat().isSameInstanceAs(uee);
}
caughtUee =
assertThrows(
UncheckedExecutionException.class, () -> cacheUnchecked.getAll(asList(new Object())));
assertThat(caughtUee).hasCauseThat().isSameInstanceAs(uee);

ExecutionException caughtEe =
assertThrows(ExecutionException.class, () -> cacheChecked.get(new Object()));
Expand Down Expand Up @@ -1818,14 +1808,10 @@ public void testBulkLoadingExceptionWithCause() {
LoadingCache<Object, Object> cacheChecked =
CacheBuilder.newBuilder().build(bulkLoader(exceptionLoader(ee)));

try {
cacheUnchecked.getAll(asList(new Object()));
fail();
} catch (ExecutionException e) {
fail();
} catch (UncheckedExecutionException caughtEe) {
assertThat(caughtEe).hasCauseThat().isSameInstanceAs(uee);
}
UncheckedExecutionException caughtUee =
assertThrows(
UncheckedExecutionException.class, () -> cacheUnchecked.getAll(asList(new Object())));
assertThat(caughtUee).hasCauseThat().isSameInstanceAs(uee);

ExecutionException caughtEe =
assertThrows(ExecutionException.class, () -> cacheChecked.getAll(asList(new Object())));
Expand Down Expand Up @@ -1897,6 +1883,7 @@ private static void testConcurrentLoadingNull(CacheBuilder<Object, Object> build
builder.build(
new CacheLoader<String, String>() {
@Override
@SuppressWarnings("CacheLoaderNull") // test of broken user implementation
public String load(String key) throws InterruptedException {
callCount.incrementAndGet();
startSignal.await();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,11 +159,7 @@ public void throwExceptionOn(String message) {
}
};
eventBus.register(subscriber);
try {
eventBus.post(EVENT);
} catch (RuntimeException e) {
fail("Exception should not be thrown.");
}
eventBus.post(EVENT);
}

public void testDeadEventForwarding() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@
* tests.
* </ul>
*/
// We call threadUnexpectedException, which does the right thing for errors.
@SuppressWarnings("AssertionFailureIgnored")
abstract class JSR166TestCase extends TestCase {
private static final boolean useSecurityManager = Boolean.getBoolean("jsr166.useSecurityManager");

Expand Down Expand Up @@ -449,54 +451,46 @@ static void delay(long millis) throws InterruptedException {
}

/** Waits out termination of a thread pool or fails doing so. */
void joinPool(ExecutorService exec) {
void joinPool(ExecutorService exec) throws InterruptedException {
try {
exec.shutdown();
assertTrue(
"ExecutorService did not terminate in a timely manner",
exec.awaitTermination(2 * LONG_DELAY_MS, MILLISECONDS));
} catch (SecurityException ok) {
// Allowed in case test doesn't have privs
} catch (InterruptedException ie) {
fail("Unexpected InterruptedException");
}
}

/**
* Checks that thread does not terminate within the default millisecond delay of {@code
* timeoutMillis()}.
*/
void assertThreadStaysAlive(Thread thread) {
void assertThreadStaysAlive(Thread thread) throws InterruptedException {
assertThreadStaysAlive(thread, timeoutMillis());
}

/** Checks that thread does not terminate within the given millisecond delay. */
void assertThreadStaysAlive(Thread thread, long millis) {
try {
// No need to optimize the failing case via Thread.join.
delay(millis);
assertTrue(thread.isAlive());
} catch (InterruptedException ie) {
fail("Unexpected InterruptedException");
}
void assertThreadStaysAlive(Thread thread, long millis) throws InterruptedException {
// No need to optimize the failing case via Thread.join.
delay(millis);
assertTrue(thread.isAlive());
}

/**
* Checks that the threads do not terminate within the default millisecond delay of {@code
* timeoutMillis()}.
*/
void assertThreadsStayAlive(Thread... threads) {
void assertThreadsStayAlive(Thread... threads) throws InterruptedException {
assertThreadsStayAlive(timeoutMillis(), threads);
}

/** Checks that the threads do not terminate within the given millisecond delay. */
void assertThreadsStayAlive(long millis, Thread... threads) {
try {
// No need to optimize the failing case via Thread.join.
delay(millis);
for (Thread thread : threads) assertTrue(thread.isAlive());
} catch (InterruptedException ie) {
fail("Unexpected InterruptedException");
void assertThreadsStayAlive(long millis, Thread... threads) throws InterruptedException {
// No need to optimize the failing case via Thread.join.
delay(millis);
for (Thread thread : threads) {
assertTrue(thread.isAlive());
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,7 @@ private static void assertGoodTesterAnnotation(Class<? extends Annotation> annot
try {
method = annotationClass.getMethod(propertyName);
} catch (NoSuchMethodException e) {
fail(
rootLocaleFormat("%s must have a property named '%s'.", annotationClass, propertyName));
throw new AssertionError("Annotation is missing required method", e);
}
final Class<?> returnType = method.getReturnType();
assertTrue(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,10 @@
* @author Kevin Bourrillion
* @author Mick Killianey
*/
@SuppressWarnings("CheckReturnValue")
@SuppressWarnings({
"CheckReturnValue",
"unused", // many methods tested reflectively -- maybe prefer local @Keep annotations?
})
public class NullPointerTesterTest extends TestCase {

/** Non-NPE RuntimeException. */
Expand Down Expand Up @@ -268,7 +271,7 @@ public void testStaticOneArgMethodsThatShouldPass() throws Exception {
try {
new NullPointerTester().testMethodParameter(new OneArg(), method, 0);
} catch (AssertionError unexpected) {
fail("Should not have flagged method " + methodName);
throw new AssertionError("Should not have flagged method " + methodName, unexpected);
}
}
}
Expand All @@ -293,7 +296,7 @@ public void testNonStaticOneArgMethodsThatShouldPass() throws Exception {
try {
new NullPointerTester().testMethodParameter(foo, method, 0);
} catch (AssertionError unexpected) {
fail("Should not have flagged method " + methodName);
throw new AssertionError("Should not have flagged method " + methodName, unexpected);
}
}
}
Expand Down
Loading

0 comments on commit ca12c33

Please sign in to comment.