Fix fragile tests for asynchronous events

This commit introduces a dependency on the Awaitility assertion
framework and makes use of asynchronous assertions in order to make
tests for asynchronous events more robust.

Issue: SPR-17211

spring-context/spring-context.gradle

@@ -28,6 +28,7 @@ dependencies {
 	testCompile("org.codehaus.groovy:groovy-test:${groovyVersion}")
 	testCompile("org.apache.commons:commons-pool2:2.6.0")
 	testCompile("javax.inject:javax.inject-tck:1")
+	testCompile("org.awaitility:awaitility:3.1.2")
 	testRuntime("javax.xml.bind:jaxb-api:2.3.0")
 	testRuntime("org.glassfish:javax.el:3.0.1-b08")
 	testRuntime("org.javamoney:moneta:1.3")

spring-context/src/test/java/org/springframework/context/support/SimpleThreadScopeTests.java

@@ -16,6 +16,9 @@
 
 package org.springframework.context.support;
 
+import java.util.concurrent.TimeUnit;
+
+import org.awaitility.Awaitility;
 import org.junit.Test;
 
 import org.springframework.context.ApplicationContext;
@@ -45,26 +48,18 @@ public class SimpleThreadScopeTests {
 
 	@Test
 	public void getMultipleInstances() throws Exception {
+		// Arrange
 		final TestBean[] beans = new TestBean[2];
-		Thread thread1 = new Thread(new Runnable() {
-			@Override
-			public void run() {
-				beans[0] = applicationContext.getBean("threadScopedObject", TestBean.class);
-			}
-		});
-		Thread thread2 = new Thread(new Runnable() {
-			@Override
-			public void run() {
-				beans[1] = applicationContext.getBean("threadScopedObject", TestBean.class);
-			}
-		});
+		Thread thread1 = new Thread(() -> beans[0] = applicationContext.getBean("threadScopedObject", TestBean.class));
+		Thread thread2 = new Thread(() -> beans[1] = applicationContext.getBean("threadScopedObject", TestBean.class));
+		// Act
 		thread1.start();
 		thread2.start();
-
-		Thread.sleep(200);
-
-		assertNotNull(beans[0]);
-		assertNotNull(beans[1]);
+		// Assert
+		Awaitility.await()
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .atMost(500, TimeUnit.MILLISECONDS)
+				  .until(() -> beans[0] != null & beans[1] != null);
 		assertNotSame(beans[0], beans[1]);
 	}
 

spring-context/src/test/java/org/springframework/scheduling/annotation/AsyncExecutionTests.java

@@ -24,9 +24,11 @@ import java.util.HashMap;
 import java.util.concurrent.CompletableFuture;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
 
 import org.aopalliance.intercept.MethodInterceptor;
 import org.aopalliance.intercept.MethodInvocation;
+import org.awaitility.Awaitility;
 import org.junit.Test;
 
 import org.springframework.aop.framework.ProxyFactory;
@@ -365,19 +367,26 @@ public class AsyncExecutionTests {
 
 	@Test
 	public void asyncMethodListener() throws Exception {
+		// Arrange
 		originalThreadName = Thread.currentThread().getName();
 		listenerCalled = 0;
 		GenericApplicationContext context = new GenericApplicationContext();
 		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(AsyncMethodListener.class));
 		context.registerBeanDefinition("autoProxyCreator", new RootBeanDefinition(DefaultAdvisorAutoProxyCreator.class));
 		context.registerBeanDefinition("asyncAdvisor", new RootBeanDefinition(AsyncAnnotationAdvisor.class));
+		// Act
 		context.refresh();
-		Thread.sleep(1000);
-		assertEquals(1, listenerCalled);
+		// Assert
+		Awaitility.await()
+				  .atMost(1, TimeUnit.SECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> listenerCalled == 1);
+		assertEquals(listenerCalled, 1);
 	}
 
 	@Test
 	public void asyncClassListener() throws Exception {
+		// Arrange
 		originalThreadName = Thread.currentThread().getName();
 		listenerCalled = 0;
 		listenerConstructed = 0;
@@ -385,15 +394,21 @@ public class AsyncExecutionTests {
 		context.registerBeanDefinition("asyncTest", new RootBeanDefinition(AsyncClassListener.class));
 		context.registerBeanDefinition("autoProxyCreator", new RootBeanDefinition(DefaultAdvisorAutoProxyCreator.class));
 		context.registerBeanDefinition("asyncAdvisor", new RootBeanDefinition(AsyncAnnotationAdvisor.class));
+		// Act
 		context.refresh();
 		context.close();
-		Thread.sleep(1000);
+		// Assert
+		Awaitility.await()
+				  .atMost(1, TimeUnit.SECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> listenerCalled == 2);
 		assertEquals(2, listenerCalled);
 		assertEquals(1, listenerConstructed);
 	}
 
 	@Test
 	public void asyncPrototypeClassListener() throws Exception {
+		// Arrange
 		originalThreadName = Thread.currentThread().getName();
 		listenerCalled = 0;
 		listenerConstructed = 0;
@@ -403,9 +418,14 @@ public class AsyncExecutionTests {
 		context.registerBeanDefinition("asyncTest", listenerDef);
 		context.registerBeanDefinition("autoProxyCreator", new RootBeanDefinition(DefaultAdvisorAutoProxyCreator.class));
 		context.registerBeanDefinition("asyncAdvisor", new RootBeanDefinition(AsyncAnnotationAdvisor.class));
+		// Act
 		context.refresh();
 		context.close();
-		Thread.sleep(1000);
+		// Assert
+		Awaitility.await()
+				  .atMost(1, TimeUnit.SECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> listenerCalled == 2);
 		assertEquals(2, listenerCalled);
 		assertEquals(2, listenerConstructed);
 	}

spring-context/src/test/java/org/springframework/scheduling/annotation/EnableAsyncTests.java

@@ -25,7 +25,9 @@ import java.util.concurrent.ExecutionException;
 import java.util.concurrent.Executor;
 import java.util.concurrent.Executors;
 import java.util.concurrent.Future;
+import java.util.concurrent.TimeUnit;
 
+import org.awaitility.Awaitility;
 import org.junit.Test;
 import org.mockito.Mockito;
 
@@ -163,7 +165,7 @@ public class EnableAsyncTests {
 		Object bean = ctx.getBean(CustomAsyncBean.class);
 		assertTrue(AopUtils.isAopProxy(bean));
 		boolean isAsyncAdvised = false;
-		for (Advisor advisor : ((Advised)bean).getAdvisors()) {
+		for (Advisor advisor : ((Advised) bean).getAdvisors()) {
 			if (advisor instanceof AsyncAnnotationAdvisor) {
 				isAsyncAdvised = true;
 				break;
@@ -184,85 +186,129 @@ public class EnableAsyncTests {
 
 	@Test
 	public void customExecutorBean() throws InterruptedException {
+		// Arrange
 		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
 		ctx.register(CustomExecutorBean.class);
 		ctx.refresh();
-
 		AsyncBean asyncBean = ctx.getBean(AsyncBean.class);
+		// Act
 		asyncBean.work();
-		Thread.sleep(500);
+		// Assert
+		Awaitility.await()
+				  .atMost(500, TimeUnit.MILLISECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> asyncBean.getThreadOfExecution() != null);
 		assertThat(asyncBean.getThreadOfExecution().getName(), startsWith("Custom-"));
-
 		ctx.close();
 	}
 
 	@Test
 	public void customExecutorConfig() throws InterruptedException {
+		// Arrange
 		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
 		ctx.register(CustomExecutorConfig.class);
 		ctx.refresh();
-
 		AsyncBean asyncBean = ctx.getBean(AsyncBean.class);
+		// Act
 		asyncBean.work();
-		Thread.sleep(500);
+		// Assert
+		Awaitility.await()
+				  .atMost(500, TimeUnit.MILLISECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> asyncBean.getThreadOfExecution() != null);
 		assertThat(asyncBean.getThreadOfExecution().getName(), startsWith("Custom-"));
+		ctx.close();
+	}
 
-		TestableAsyncUncaughtExceptionHandler exceptionHandler = (TestableAsyncUncaughtExceptionHandler)
-				ctx.getBean("exceptionHandler");
-		assertFalse("handler should not have been called yet", exceptionHandler.isCalled());
-
-		asyncBean.fail();
-		Thread.sleep(500);
+	@Test
+	public void customExecutorConfigWithThrowsException() {
+		// Arrange
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(CustomExecutorConfig.class);
+		ctx.refresh();
+		AsyncBean asyncBean = ctx.getBean(AsyncBean.class);
 		Method method = ReflectionUtils.findMethod(AsyncBean.class, "fail");
-		exceptionHandler.assertCalledWith(method, UnsupportedOperationException.class);
-
+		TestableAsyncUncaughtExceptionHandler exceptionHandler =
+				(TestableAsyncUncaughtExceptionHandler) ctx.getBean("exceptionHandler");
+		assertFalse("handler should not have been called yet", exceptionHandler.isCalled());
+		// Act
+		asyncBean.fail();
+		// Assert
+		Awaitility.await()
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .atMost(500, TimeUnit.MILLISECONDS)
+				  .untilAsserted(() -> exceptionHandler.assertCalledWith(method, UnsupportedOperationException.class));
 		ctx.close();
 	}
 
 	@Test
 	public void customExecutorBeanConfig() throws InterruptedException {
+		// Arrange
 		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
 		ctx.register(CustomExecutorBeanConfig.class, ExecutorPostProcessor.class);
 		ctx.refresh();
-
 		AsyncBean asyncBean = ctx.getBean(AsyncBean.class);
+		// Act
 		asyncBean.work();
-		Thread.sleep(500);
+		// Assert
+		Awaitility.await()
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .atMost(500, TimeUnit.MILLISECONDS)
+				  .until(() -> asyncBean.getThreadOfExecution() != null);
 		assertThat(asyncBean.getThreadOfExecution().getName(), startsWith("Post-"));
+		ctx.close();
+	}
 
-		TestableAsyncUncaughtExceptionHandler exceptionHandler = (TestableAsyncUncaughtExceptionHandler)
-				ctx.getBean("exceptionHandler");
+	@Test
+	public void customExecutorBeanConfigWithThrowsException() {
+		// Arrange
+		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext();
+		ctx.register(CustomExecutorBeanConfig.class, ExecutorPostProcessor.class);
+		ctx.refresh();
+		AsyncBean asyncBean = ctx.getBean(AsyncBean.class);
+		TestableAsyncUncaughtExceptionHandler exceptionHandler =
+				(TestableAsyncUncaughtExceptionHandler) ctx.getBean("exceptionHandler");
 		assertFalse("handler should not have been called yet", exceptionHandler.isCalled());
-
-		asyncBean.fail();
-		Thread.sleep(500);
 		Method method = ReflectionUtils.findMethod(AsyncBean.class, "fail");
-		exceptionHandler.assertCalledWith(method, UnsupportedOperationException.class);
-
+		// Act
+		asyncBean.fail();
+		// Assert
+		Awaitility.await()
+				  .atMost(500, TimeUnit.MILLISECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .untilAsserted(() -> exceptionHandler.assertCalledWith(method, UnsupportedOperationException.class));
 		ctx.close();
 	}
 
 	@Test  // SPR-14949
 	public void findOnInterfaceWithInterfaceProxy() throws InterruptedException {
+		// Arrange
 		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(Spr14949ConfigA.class);
-
 		AsyncInterface asyncBean = ctx.getBean(AsyncInterface.class);
+		// Act
 		asyncBean.work();
-		Thread.sleep(500);
+		// Assert
+		Awaitility.await()
+				  .atMost(500, TimeUnit.MILLISECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> asyncBean.getThreadOfExecution() != null);
 		assertThat(asyncBean.getThreadOfExecution().getName(), startsWith("Custom-"));
-
 		ctx.close();
 	}
 
 	@Test  // SPR-14949
 	public void findOnInterfaceWithCglibProxy() throws InterruptedException {
+		// Arrange
 		AnnotationConfigApplicationContext ctx = new AnnotationConfigApplicationContext(Spr14949ConfigB.class);
-
 		AsyncInterface asyncBean = ctx.getBean(AsyncInterface.class);
+		// Act
 		asyncBean.work();
-		Thread.sleep(500);
+		// Assert
+		Awaitility.await()
+				  .atMost(500, TimeUnit.MILLISECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(()-> asyncBean.getThreadOfExecution() != null);
 		assertThat(asyncBean.getThreadOfExecution().getName(), startsWith("Custom-"));
-
 		ctx.close();
 	}
 
@@ -390,7 +436,8 @@ public class EnableAsyncTests {
 	@EnableAsync
 	static class AsyncConfigWithMockito {
 
-		@Bean @Lazy
+		@Bean
+		@Lazy
 		public AsyncBean asyncBean() {
 			return Mockito.mock(AsyncBean.class);
 		}

spring-context/src/test/java/org/springframework/scheduling/concurrent/AbstractSchedulingTaskExecutorTests.java

@@ -24,6 +24,7 @@ import java.util.concurrent.Future;
 import java.util.concurrent.TimeUnit;
 import java.util.concurrent.atomic.AtomicInteger;
 
+import org.awaitility.Awaitility;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
@@ -113,10 +114,14 @@ public abstract class AbstractSchedulingTaskExecutorTests {
 	@Test
 	public void submitListenableRunnable() throws Exception {
 		TestTask task = new TestTask(1);
+		// Act
 		ListenableFuture<?> future = executor.submitListenable(task);
 		future.addCallback(result -> outcome = result, ex -> outcome = ex);
-		Thread.sleep(1000);
-		assertTrue(future.isDone());
+		// Assert
+		Awaitility.await()
+				  .atMost(1, TimeUnit.SECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(future::isDone);
 		assertNull(outcome);
 		assertThreadNamePrefix(task);
 	}
@@ -126,8 +131,12 @@ public abstract class AbstractSchedulingTaskExecutorTests {
 		TestTask task = new TestTask(0);
 		ListenableFuture<?> future = executor.submitListenable(task);
 		future.addCallback(result -> outcome = result, ex -> outcome = ex);
-		Thread.sleep(1000);
-		assertTrue(future.isDone());
+
+		Awaitility.await()
+				  .dontCatchUncaughtExceptions()
+				  .atMost(1, TimeUnit.SECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> future.isDone() && outcome != null);
 		assertSame(RuntimeException.class, outcome.getClass());
 	}
 
@@ -165,27 +174,36 @@ public abstract class AbstractSchedulingTaskExecutorTests {
 		TestCallable task2 = new TestCallable(-1);
 		Future<?> future2 = executor.submit(task2);
 		shutdownExecutor();
-		future1.get(100, TimeUnit.MILLISECONDS);
-		future2.get(100, TimeUnit.MILLISECONDS);
+		future1.get(1000, TimeUnit.MILLISECONDS);
+		future2.get(1000, TimeUnit.MILLISECONDS);
 	}
 
 	@Test
 	public void submitListenableCallable() throws Exception {
 		TestCallable task = new TestCallable(1);
+		// Act
 		ListenableFuture<String> future = executor.submitListenable(task);
 		future.addCallback(result -> outcome = result, ex -> outcome = ex);
-		Thread.sleep(100);
-		assertTrue(future.isDone());
+		// Assert
+		Awaitility.await()
+				  .atMost(1, TimeUnit.SECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> future.isDone() && outcome != null);
 		assertEquals(THREAD_NAME_PREFIX, outcome.toString().substring(0, THREAD_NAME_PREFIX.length()));
 	}
 
 	@Test
 	public void submitFailingListenableCallable() throws Exception {
 		TestCallable task = new TestCallable(0);
+		// Act
 		ListenableFuture<String> future = executor.submitListenable(task);
 		future.addCallback(result -> outcome = result, ex -> outcome = ex);
-		Thread.sleep(100);
-		assertTrue(future.isDone());
+		// Assert
+		Awaitility.await()
+				  .dontCatchUncaughtExceptions()
+				  .atMost(1, TimeUnit.SECONDS)
+				  .pollInterval(10, TimeUnit.MILLISECONDS)
+				  .until(() -> future.isDone() && outcome != null);
 		assertSame(RuntimeException.class, outcome.getClass());
 	}
 
@@ -196,8 +214,8 @@ public abstract class AbstractSchedulingTaskExecutorTests {
 		TestCallable task2 = new TestCallable(-1);
 		ListenableFuture<?> future2 = executor.submitListenable(task2);
 		shutdownExecutor();
-		future1.get(100, TimeUnit.MILLISECONDS);
-		future2.get(100, TimeUnit.MILLISECONDS);
+		future1.get(1000, TimeUnit.MILLISECONDS);
+		future2.get(1000, TimeUnit.MILLISECONDS);
 	}