Concurrency and exception message refinements for test transactions

(cherry picked from commit a0cc800)

spring-test/src/main/java/org/springframework/test/context/transaction/AfterTransaction.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,15 +28,15 @@ import java.lang.annotation.Target;
  * configured to run within a transaction via Spring's {@code @Transactional}
  * annotation.
  *
- * <p>As of Spring Framework 4.3, {@code @AfterTransaction} may be declared on
- * Java 8 based interface default methods.
- *
  * <p>{@code @AfterTransaction} methods declared in superclasses or as interface
  * default methods will be executed after those of the current test class.
  *
  * <p>As of Spring Framework 4.0, this annotation may be used as a
  * <em>meta-annotation</em> to create custom <em>composed annotations</em>.
  *
+ * <p>As of Spring Framework 4.3, {@code @AfterTransaction} may also be
+ * declared on Java 8 based interface default methods.
+ *
  * @author Sam Brannen
  * @since 2.5
  * @see org.springframework.transaction.annotation.Transactional

spring-test/src/main/java/org/springframework/test/context/transaction/BeforeTransaction.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -28,15 +28,15 @@ import java.lang.annotation.Target;
  * configured to run within a transaction via Spring's {@code @Transactional}
  * annotation.
  *
- * <p>As of Spring Framework 4.3, {@code @BeforeTransaction} may be declared on
- * Java 8 based interface default methods.
- *
  * <p>{@code @BeforeTransaction} methods declared in superclasses or as interface
  * default methods will be executed before those of the current test class.
  *
  * <p>As of Spring Framework 4.0, this annotation may be used as a
  * <em>meta-annotation</em> to create custom <em>composed annotations</em>.
  *
+ * <p>As of Spring Framework 4.3, {@code @BeforeTransaction} may also be
+ * declared on Java 8 based interface default methods.
+ *
  * @author Sam Brannen
  * @since 2.5
  * @see org.springframework.transaction.annotation.Transactional

spring-test/src/main/java/org/springframework/test/context/transaction/TestContextTransactionUtils.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -38,6 +38,7 @@ import org.springframework.util.StringUtils;
 /**
  * Utility methods for working with transactions and data access related beans
  * within the <em>Spring TestContext Framework</em>.
+ *
  * <p>Mainly for internal use within the framework.
  *
  * @author Sam Brannen
@@ -46,8 +47,6 @@ import org.springframework.util.StringUtils;
  */
 public abstract class TestContextTransactionUtils {
 
-	private static final Log logger = LogFactory.getLog(TestContextTransactionUtils.class);
-
 	/**
 	 * Default bean name for a {@link DataSource}: {@code "dataSource"}.
 	 */
@@ -60,9 +59,8 @@ public abstract class TestContextTransactionUtils {
 	public static final String DEFAULT_TRANSACTION_MANAGER_NAME = "transactionManager";
 
 
-	private TestContextTransactionUtils() {
-		/* prevent instantiation */
-	}
+	private static final Log logger = LogFactory.getLog(TestContextTransactionUtils.class);
+
 
 	/**
 	 * Retrieve the {@link DataSource} to use for the supplied {@linkplain TestContext
@@ -80,8 +78,8 @@ public abstract class TestContextTransactionUtils {
 	 * {@linkplain #DEFAULT_DATA_SOURCE_NAME default data source name}.
 	 * @param testContext the test context for which the {@code DataSource}
 	 * should be retrieved; never {@code null}
-	 * @param name the name of the {@code DataSource} to retrieve; may be {@code null}
-	 * or <em>empty</em>
+	 * @param name the name of the {@code DataSource} to retrieve
+	 * (may be {@code null} or <em>empty</em>)
 	 * @return the {@code DataSource} to use, or {@code null} if not found
 	 * @throws BeansException if an error occurs while retrieving an explicitly
 	 * named {@code DataSource}
@@ -91,14 +89,14 @@ public abstract class TestContextTransactionUtils {
 		BeanFactory bf = testContext.getApplicationContext().getAutowireCapableBeanFactory();
 
 		try {
-			// look up by type and explicit name
+			// Look up by type and explicit name
 			if (StringUtils.hasText(name)) {
 				return bf.getBean(name, DataSource.class);
 			}
 		}
 		catch (BeansException ex) {
-			logger.error(
-				String.format("Failed to retrieve DataSource named '%s' for test context %s", name, testContext), ex);
+			logger.error(String.format("Failed to retrieve DataSource named '%s' for test context %s",
+					name, testContext), ex);
 			throw ex;
 		}
 
@@ -106,9 +104,9 @@ public abstract class TestContextTransactionUtils {
 			if (bf instanceof ListableBeanFactory) {
 				ListableBeanFactory lbf = (ListableBeanFactory) bf;
 
-				// look up single bean by type
-				Map<String, DataSource> dataSources = BeanFactoryUtils.beansOfTypeIncludingAncestors(lbf,
-					DataSource.class);
+				// Look up single bean by type
+				Map<String, DataSource> dataSources =
+						BeanFactoryUtils.beansOfTypeIncludingAncestors(lbf, DataSource.class);
 				if (dataSources.size() == 1) {
 					return dataSources.values().iterator().next();
 				}
@@ -150,8 +148,8 @@ public abstract class TestContextTransactionUtils {
 	 * name}.
 	 * @param testContext the test context for which the transaction manager
 	 * should be retrieved; never {@code null}
-	 * @param name the name of the transaction manager to retrieve; may be
-	 * {@code null} or <em>empty</em>
+	 * @param name the name of the transaction manager to retrieve
+	 * (may be {@code null} or <em>empty</em>)
 	 * @return the transaction manager to use, or {@code null} if not found
 	 * @throws BeansException if an error occurs while retrieving an explicitly
 	 * named transaction manager
@@ -163,14 +161,14 @@ public abstract class TestContextTransactionUtils {
 		BeanFactory bf = testContext.getApplicationContext().getAutowireCapableBeanFactory();
 
 		try {
-			// look up by type and explicit name
+			// Look up by type and explicit name
 			if (StringUtils.hasText(name)) {
 				return bf.getBean(name, PlatformTransactionManager.class);
 			}
 		}
 		catch (BeansException ex) {
-			logger.error(String.format("Failed to retrieve transaction manager named '%s' for test context %s", name,
-				testContext), ex);
+			logger.error(String.format("Failed to retrieve transaction manager named '%s' for test context %s",
+					name, testContext), ex);
 			throw ex;
 		}
 
@@ -178,28 +176,26 @@ public abstract class TestContextTransactionUtils {
 			if (bf instanceof ListableBeanFactory) {
 				ListableBeanFactory lbf = (ListableBeanFactory) bf;
 
-				// look up single bean by type
-				Map<String, PlatformTransactionManager> txMgrs = BeanFactoryUtils.beansOfTypeIncludingAncestors(lbf,
-					PlatformTransactionManager.class);
+				// Look up single bean by type
+				Map<String, PlatformTransactionManager> txMgrs =
+						BeanFactoryUtils.beansOfTypeIncludingAncestors(lbf, PlatformTransactionManager.class);
 				if (txMgrs.size() == 1) {
 					return txMgrs.values().iterator().next();
 				}
 
 				try {
-					// look up single bean by type, with support for 'primary' beans
+					// Look up single bean by type, with support for 'primary' beans
 					return bf.getBean(PlatformTransactionManager.class);
 				}
 				catch (BeansException ex) {
 					logBeansException(testContext, ex, PlatformTransactionManager.class);
 				}
 
-				// look up single TransactionManagementConfigurer
-				Map<String, TransactionManagementConfigurer> configurers = BeanFactoryUtils.beansOfTypeIncludingAncestors(
-					lbf, TransactionManagementConfigurer.class);
-				if (configurers.size() > 1) {
-					throw new IllegalStateException(
+				// Look up single TransactionManagementConfigurer
+				Map<String, TransactionManagementConfigurer> configurers =
+						BeanFactoryUtils.beansOfTypeIncludingAncestors(lbf, TransactionManagementConfigurer.class);
+				Assert.state(configurers.size() <= 1,
 						"Only one TransactionManagementConfigurer may exist in the ApplicationContext");
-				}
 				if (configurers.size() == 1) {
 					return configurers.values().iterator().next().annotationDrivenTransactionManager();
 				}
@@ -225,13 +221,13 @@ public abstract class TestContextTransactionUtils {
 	 * Create a delegating {@link TransactionAttribute} for the supplied target
 	 * {@link TransactionAttribute} and {@link TestContext}, using the names of
 	 * the test class and test method to build the name of the transaction.
-	 *
-	 * @param testContext the {@code TestContext} upon which to base the name; never {@code null}
-	 * @param targetAttribute the {@code TransactionAttribute} to delegate to; never {@code null}
+	 * @param testContext the {@code TestContext} upon which to base the name
+	 * @param targetAttribute the {@code TransactionAttribute} to delegate to
 	 * @return the delegating {@code TransactionAttribute}
 	 */
-	public static TransactionAttribute createDelegatingTransactionAttribute(TestContext testContext,
-			TransactionAttribute targetAttribute) {
+	public static TransactionAttribute createDelegatingTransactionAttribute(
+			TestContext testContext, TransactionAttribute targetAttribute) {
+
 		Assert.notNull(testContext, "TestContext must not be null");
 		Assert.notNull(targetAttribute, "Target TransactionAttribute must not be null");
 		return new TestContextTransactionAttribute(targetAttribute, testContext);

spring-test/src/main/java/org/springframework/test/context/transaction/TestTransaction.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2014 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,6 +18,7 @@ package org.springframework.test.context.transaction;
 
 import org.springframework.test.context.TestExecutionListeners;
 import org.springframework.transaction.TransactionStatus;
+import org.springframework.util.Assert;
 
 /**
  * {@code TestTransaction} provides a collection of static utility methods for
@@ -49,10 +50,8 @@ public class TestTransaction {
 		TransactionContext transactionContext = TransactionContextHolder.getCurrentTransactionContext();
 		if (transactionContext != null) {
 			TransactionStatus transactionStatus = transactionContext.getTransactionStatus();
-			return (transactionStatus != null) && (!transactionStatus.isCompleted());
+			return (transactionStatus != null && !transactionStatus.isCompleted());
 		}
-
-		// else
 		return false;
 	}
 
@@ -79,8 +78,7 @@ public class TestTransaction {
 	 * Rather, the value of this flag will be used to determine whether or not
 	 * the current test-managed transaction should be rolled back or committed
 	 * once it is {@linkplain #end ended}.
-	 * @throws IllegalStateException if a transaction is not active for the
-	 * current test
+	 * @throws IllegalStateException if no transaction is active for the current test
 	 * @see #isActive()
 	 * @see #isFlaggedForRollback()
 	 * @see #start()
@@ -96,8 +94,7 @@ public class TestTransaction {
 	 * Rather, the value of this flag will be used to determine whether or not
 	 * the current test-managed transaction should be rolled back or committed
 	 * once it is {@linkplain #end ended}.
-	 * @throws IllegalStateException if a transaction is not active for the
-	 * current test
+	 * @throws IllegalStateException if no transaction is active for the current test
 	 * @see #isActive()
 	 * @see #isFlaggedForRollback()
 	 * @see #start()
@@ -121,9 +118,9 @@ public class TestTransaction {
 	}
 
 	/**
-	 * Immediately force a <em>commit</em> or <em>rollback</em> of the current
-	 * test-managed transaction, according to the {@linkplain #isFlaggedForRollback
-	 * rollback flag}.
+	 * Immediately force a <em>commit</em> or <em>rollback</em> of the
+	 * current test-managed transaction, according to the
+	 * {@linkplain #isFlaggedForRollback rollback flag}.
 	 * @throws IllegalStateException if the transaction context could not be
 	 * retrieved or if a transaction is not active for the current test
 	 * @see #isActive()
@@ -133,11 +130,10 @@ public class TestTransaction {
 		requireCurrentTransactionContext().endTransaction();
 	}
 
+
 	private static TransactionContext requireCurrentTransactionContext() {
 		TransactionContext txContext = TransactionContextHolder.getCurrentTransactionContext();
-		if (txContext == null) {
-			throw new IllegalStateException("TransactionContext is not active");
-		}
+		Assert.state(txContext != null, "TransactionContext is not active");
 		return txContext;
 	}
 
@@ -145,4 +141,4 @@ public class TestTransaction {
 		requireCurrentTransactionContext().setFlaggedForRollback(flag);
 	}
 
-}
+}

spring-test/src/main/java/org/springframework/test/context/transaction/TransactionContext.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -16,6 +16,8 @@
 
 package org.springframework.test.context.transaction;
 
+import java.util.concurrent.atomic.AtomicInteger;
+
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 
@@ -24,6 +26,7 @@ import org.springframework.transaction.PlatformTransactionManager;
 import org.springframework.transaction.TransactionDefinition;
 import org.springframework.transaction.TransactionException;
 import org.springframework.transaction.TransactionStatus;
+import org.springframework.util.Assert;
 
 /**
  * Transaction context for a specific {@link TestContext}.
@@ -50,7 +53,7 @@ class TransactionContext {
 
 	private TransactionStatus transactionStatus;
 
-	private volatile int transactionsStarted = 0;
+	private final AtomicInteger transactionsStarted = new AtomicInteger(0);
 
 
 	TransactionContext(TestContext testContext, PlatformTransactionManager transactionManager,
@@ -79,32 +82,30 @@ class TransactionContext {
 
 	void setFlaggedForRollback(boolean flaggedForRollback) {
 		if (this.transactionStatus == null) {
-			throw new IllegalStateException(String.format(
-				"Failed to set rollback flag for test context %s: transaction does not exist.", this.testContext));
+			throw new IllegalStateException(
+					"Failed to set rollback flag - transaction does not exist: " + this.testContext);
 		}
 		this.flaggedForRollback = flaggedForRollback;
 	}
 
 	/**
-	 * Start a new transaction for the configured {@linkplain #getTestContext test context}.
+	 * Start a new transaction for the configured test context.
 	 * <p>Only call this method if {@link #endTransaction} has been called or if no
 	 * transaction has been previously started.
 	 * @throws TransactionException if starting the transaction fails
 	 */
 	void startTransaction() {
-		if (this.transactionStatus != null) {
-			throw new IllegalStateException(
-				"Cannot start a new transaction without ending the existing transaction first.");
-		}
+		Assert.state(this.transactionStatus == null,
+				"Cannot start a new transaction without ending the existing transaction first");
 
 		this.flaggedForRollback = this.defaultRollback;
 		this.transactionStatus = this.transactionManager.getTransaction(this.transactionDefinition);
-		++this.transactionsStarted;
+		int transactionsStarted = this.transactionsStarted.incrementAndGet();
 
 		if (logger.isInfoEnabled()) {
 			logger.info(String.format(
 					"Began transaction (%s) for test context %s; transaction manager [%s]; rollback [%s]",
-					this.transactionsStarted, this.testContext, this.transactionManager, flaggedForRollback));
+					transactionsStarted, this.testContext, this.transactionManager, flaggedForRollback));
 		}
 	}
 
@@ -119,8 +120,8 @@ class TransactionContext {
 					this.testContext, this.transactionStatus, this.flaggedForRollback));
 		}
 		if (this.transactionStatus == null) {
-			throw new IllegalStateException(String.format(
-					"Failed to end transaction for test context %s: transaction does not exist.", this.testContext));
+			throw new IllegalStateException(
+					"Failed to end transaction - transaction does not exist: " + this.testContext);
 		}
 
 		try {
@@ -136,8 +137,8 @@ class TransactionContext {
 		}
 
 		if (logger.isInfoEnabled()) {
-			logger.info(String.format("%s transaction for test context %s.",
-					(this.flaggedForRollback ? "Rolled back" : "Committed"), this.testContext));
+			logger.info((this.flaggedForRollback ? "Rolled back" : "Committed") +
+					" transaction for test: " + this.testContext);
 		}
 	}
 

spring-test/src/main/java/org/springframework/test/context/transaction/TransactionContextHolder.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2017 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -39,11 +39,9 @@ class TransactionContextHolder {
 	}
 
 	static TransactionContext removeCurrentTransactionContext() {
-		synchronized (currentTransactionContext) {
-			TransactionContext transactionContext = currentTransactionContext.get();
-			currentTransactionContext.remove();
-			return transactionContext;
-		}
+		TransactionContext transactionContext = currentTransactionContext.get();
+		currentTransactionContext.remove();
+		return transactionContext;
 	}
 
 }

spring-test/src/main/java/org/springframework/test/context/transaction/TransactionalTestExecutionListener.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2016 the original author or authors.
+ * Copyright 2002-2018 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -163,14 +163,12 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 	 */
 	@Override
 	public void beforeTestMethod(final TestContext testContext) throws Exception {
-		final Method testMethod = testContext.getTestMethod();
-		final Class<?> testClass = testContext.getTestClass();
-		Assert.notNull(testMethod, "The test method of the supplied TestContext must not be null");
+		Method testMethod = testContext.getTestMethod();
+		Class<?> testClass = testContext.getTestClass();
+		Assert.notNull(testMethod, "Test method of supplied TestContext must not be null");
 
 		TransactionContext txContext = TransactionContextHolder.removeCurrentTransactionContext();
-		if (txContext != null) {
-			throw new IllegalStateException("Cannot start a new transaction without ending the existing transaction.");
-		}
+		Assert.state(txContext == null, "Cannot start new transaction without ending existing transaction");
 
 		PlatformTransactionManager tm = null;
 		TransactionAttribute transactionAttribute = this.attributeSource.getTransactionAttribute(testMethod, testClass);
@@ -180,8 +178,8 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 				transactionAttribute);
 
 			if (logger.isDebugEnabled()) {
-				logger.debug("Explicit transaction definition [" + transactionAttribute + "] found for test context " +
-						testContext);
+				logger.debug("Explicit transaction definition [" + transactionAttribute +
+						"] found for test context " + testContext);
 			}
 
 			if (transactionAttribute.getPropagationBehavior() == TransactionDefinition.PROPAGATION_NOT_SUPPORTED) {
@@ -191,9 +189,8 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 			tm = getTransactionManager(testContext, transactionAttribute.getQualifier());
 
 			if (tm == null) {
-				throw new IllegalStateException(String.format(
-						"Failed to retrieve PlatformTransactionManager for @Transactional test for test context %s.",
-						testContext));
+				throw new IllegalStateException(
+						"Failed to retrieve PlatformTransactionManager for @Transactional test: " + testContext);
 			}
 		}
 
@@ -223,7 +220,7 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 			TransactionStatus transactionStatus = txContext.getTransactionStatus();
 			try {
 				// If the transaction is still active...
-				if ((transactionStatus != null) && !transactionStatus.isCompleted()) {
+				if (transactionStatus != null && !transactionStatus.isCompleted()) {
 					txContext.endTransaction();
 				}
 			}
@@ -380,15 +377,15 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 		TransactionConfigurationAttributes txConfigAttributes = retrieveConfigurationAttributes(testContext);
 
 		if (rollbackPresent && txConfigAttributes != defaultTxConfigAttributes) {
-			throw new IllegalStateException(String.format("Test class [%s] is annotated with both @Rollback "
-					+ "and @TransactionConfiguration, but only one is permitted.", testClass.getName()));
+			throw new IllegalStateException(String.format("Test class [%s] is annotated with both @Rollback " +
+					"and @TransactionConfiguration, but only one is permitted.", testClass.getName()));
 		}
 
 		if (rollbackPresent) {
 			boolean defaultRollback = rollback.value();
 			if (logger.isDebugEnabled()) {
-				logger.debug(String.format("Retrieved default @Rollback(%s) for test class [%s].", defaultRollback,
-					testClass.getName()));
+				logger.debug(String.format("Retrieved default @Rollback(%s) for test class [%s].",
+						defaultRollback, testClass.getName()));
 			}
 			return defaultRollback;
 		}