Concurrency and exception message refinements for test transactions

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.
@@ -17,7 +17,6 @@
 package org.springframework.test.context.transaction;
 
 import java.util.Map;
-
 import javax.sql.DataSource;
 
 import org.apache.commons.logging.Log;
@@ -40,6 +39,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
@@ -48,8 +48,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"}.
 	 */
@@ -62,9 +60,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
@@ -82,8 +79,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}
@@ -94,14 +91,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;
 		}
 
@@ -109,9 +106,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();
 				}
@@ -153,8 +150,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
@@ -167,14 +164,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;
 		}
 
@@ -182,24 +179,24 @@ 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);
+				// 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) {
@@ -227,13 +224,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-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.
@@ -50,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;
 	}
 
@@ -80,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()
@@ -97,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()
@@ -122,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()
@@ -134,6 +130,7 @@ public class TestTransaction {
 		requireCurrentTransactionContext().endTransaction();
 	}
 
+
 	private static TransactionContext requireCurrentTransactionContext() {
 		TransactionContext txContext = TransactionContextHolder.getCurrentTransactionContext();
 		Assert.state(txContext != null, "TransactionContext is not active");
@@ -144,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;
 
@@ -53,7 +55,7 @@ class TransactionContext {
 	@Nullable
 	private TransactionStatus transactionStatus;
 
-	private volatile int transactionsStarted = 0;
+	private final AtomicInteger transactionsStarted = new AtomicInteger(0);
 
 
 	TransactionContext(TestContext testContext, PlatformTransactionManager transactionManager,
@@ -82,8 +84,8 @@ class TransactionContext {
 	}
 
 	void setFlaggedForRollback(boolean flaggedForRollback) {
-		Assert.state(this.transactionStatus != null, () -> String.format(
-				"Failed to set rollback flag for test context %s: transaction does not exist.", this.testContext));
+		Assert.state(this.transactionStatus != null, () ->
+				"Failed to set rollback flag - transaction does not exist: " + this.testContext);
 		this.flaggedForRollback = flaggedForRollback;
 	}
 
@@ -95,16 +97,16 @@ class TransactionContext {
 	 */
 	void startTransaction() {
 		Assert.state(this.transactionStatus == null,
-				"Cannot start a new transaction without ending the existing transaction first.");
+				"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));
 		}
 	}
 
@@ -118,8 +120,8 @@ class TransactionContext {
 					"Ending transaction for test context %s; transaction status [%s]; rollback [%s]",
 					this.testContext, this.transactionStatus, this.flaggedForRollback));
 		}
-		Assert.state(this.transactionStatus != null, () -> String.format(
-				"Failed to end transaction for test context %s: transaction does not exist.", this.testContext));
+		Assert.state(this.transactionStatus != null,
+				() -> "Failed to end transaction - transaction does not exist: " + this.testContext);
 
 		try {
 			if (this.flaggedForRollback) {
@@ -134,8 +136,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.
@@ -31,7 +31,7 @@ class TransactionContextHolder {
 			new NamedInheritableThreadLocal<>("Test Transaction Context");
 
 
-	static void setCurrentTransactionContext(@Nullable TransactionContext transactionContext) {
+	static void setCurrentTransactionContext(TransactionContext transactionContext) {
 		currentTransactionContext.set(transactionContext);
 	}
 
@@ -42,11 +42,9 @@ class TransactionContextHolder {
 
 	@Nullable
 	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-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.
@@ -136,6 +136,7 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 	// Do not require @Transactional test methods to be public.
 	protected final TransactionAttributeSource attributeSource = new AnnotationTransactionAttributeSource(false);
 
+
 	/**
 	 * Returns {@code 4000}.
 	 */
@@ -159,10 +160,10 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 	public void beforeTestMethod(final TestContext testContext) throws Exception {
 		Method testMethod = testContext.getTestMethod();
 		Class<?> testClass = testContext.getTestClass();
-		Assert.notNull(testMethod, "The test method of the supplied TestContext must not be null");
+		Assert.notNull(testMethod, "Test method of supplied TestContext must not be null");
 
 		TransactionContext txContext = TransactionContextHolder.removeCurrentTransactionContext();
-		Assert.state(txContext == null, "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);
@@ -172,8 +173,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) {
@@ -181,9 +182,8 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 			}
 
 			tm = getTransactionManager(testContext, transactionAttribute.getQualifier());
-			Assert.state(tm != null, () -> String.format(
-					"Failed to retrieve PlatformTransactionManager for @Transactional test for test context %s.",
-					testContext));
+			Assert.state(tm != null,
+					() -> "Failed to retrieve PlatformTransactionManager for @Transactional test: " + testContext);
 		}
 
 		if (tm != null) {
@@ -368,8 +368,8 @@ public class TransactionalTestExecutionListener extends AbstractTestExecutionLis
 		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;
 		}