From 0afcb4dfed4044b202b9efd55767fbd1939e6d5d Mon Sep 17 00:00:00 2001 From: Sam Brannen Date: Wed, 4 Oct 2023 16:36:27 +0200 Subject: [PATCH] =?UTF-8?q?Revise=20"Introduce=20class-level=20execution?= =?UTF-8?q?=20phases=20for=20@=E2=81=A0Sql"?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit revises the previous commit as follows. - Remove hasTestMethod() from TestContext and instead introduce a new variant of createDelegatingTransactionAttribute() in TestContextTransactionUtils which accepts a boolean `includeMethodName` flag. - Add missing @⁠TestMethodOrder declaration to AfterTestClassSqlScriptsTests to ensure that the test methods are always executed in the required order. - Polish Javadoc and add missing @⁠since tags. Closes gh-27285 --- .../test/context/TestContext.java | 20 ------- .../test/context/jdbc/Sql.java | 17 +++--- .../jdbc/SqlScriptsTestExecutionListener.java | 34 ++++++------ .../context/support/DefaultTestContext.java | 6 --- .../TestContextTransactionUtils.java | 29 +++++++++-- .../jdbc/AfterTestClassSqlScriptsTests.java | 52 +++++++++++-------- .../jdbc/BeforeTestClassSqlScriptsTests.java | 13 +++-- .../SqlScriptsTestExecutionListenerTests.java | 14 ++--- 8 files changed, 96 insertions(+), 89 deletions(-) diff --git a/spring-test/src/main/java/org/springframework/test/context/TestContext.java b/spring-test/src/main/java/org/springframework/test/context/TestContext.java index 3f63253417..cce8366ece 100644 --- a/spring-test/src/main/java/org/springframework/test/context/TestContext.java +++ b/spring-test/src/main/java/org/springframework/test/context/TestContext.java @@ -42,7 +42,6 @@ import org.springframework.test.annotation.DirtiesContext.HierarchyMode; * override {@link #setMethodInvoker(MethodInvoker)} and {@link #getMethodInvoker()}. * * @author Sam Brannen - * @author Andreas Ahlenstorf * @since 2.5 * @see TestContextManager * @see TestExecutionListener @@ -111,25 +110,6 @@ public interface TestContext extends AttributeAccessor, Serializable { */ Object getTestInstance(); - /** - * Tests whether a test method is part of this test context. Returns - * {@code true} if this context has a current test method, {@code false} - * otherwise. - * - *

The default implementation of this method always returns {@code false}. - * Custom {@code TestContext} implementations are therefore highly encouraged - * to override this method with a more meaningful implementation. Note that - * the standard {@code TestContext} implementation in Spring overrides this - * method appropriately. - * @return {@code true} if the test execution has already entered a test - * method - * @since 6.1 - * @see #getTestMethod() - */ - default boolean hasTestMethod() { - return false; - } - /** * Get the current {@linkplain Method test method} for this test context. *

Note: this is a mutable property. diff --git a/spring-test/src/main/java/org/springframework/test/context/jdbc/Sql.java b/spring-test/src/main/java/org/springframework/test/context/jdbc/Sql.java index 6ab67c8cfa..c52810742a 100644 --- a/spring-test/src/main/java/org/springframework/test/context/jdbc/Sql.java +++ b/spring-test/src/main/java/org/springframework/test/context/jdbc/Sql.java @@ -33,11 +33,12 @@ import org.springframework.core.annotation.AliasFor; * *

Method-level declarations override class-level declarations by default, * but this behavior can be configured via {@link SqlMergeMode @SqlMergeMode}. - * However, this does not apply to class-level declarations that use - * {@link ExecutionPhase#BEFORE_TEST_CLASS} or - * {@link ExecutionPhase#AFTER_TEST_CLASS}. Such declarations are retained and - * scripts and statements are executed once per class in addition to any - * method-level annotations. + * However, this does not apply to class-level declarations configured for the + * {@link ExecutionPhase#BEFORE_TEST_CLASS BEFORE_TEST_CLASS} or + * {@link ExecutionPhase#AFTER_TEST_CLASS AFTER_TEST_CLASS} execution phase. Such + * declarations cannot be overridden, and the corresponding scripts and statements + * will be executed once per class in addition to any method-level scripts and + * statements. * *

Script execution is performed by the {@link SqlScriptsTestExecutionListener}, * which is enabled by default. @@ -169,13 +170,15 @@ public @interface Sql { /** * The configured SQL scripts and statements will be executed - * once before any test method is run. + * once per test class before any test method is run. + * @since 6.1 */ BEFORE_TEST_CLASS, /** * The configured SQL scripts and statements will be executed - * once after any test method is run. + * once per test class after all test methods have run. + * @since 6.1 */ AFTER_TEST_CLASS, diff --git a/spring-test/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java b/spring-test/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java index 90d98db5c4..bda23d8fb4 100644 --- a/spring-test/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java +++ b/spring-test/src/main/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListener.java @@ -68,8 +68,8 @@ import static org.springframework.util.ResourceUtils.CLASSPATH_URL_PREFIX; * configured via the {@link Sql @Sql} annotation. * *

Class-level annotations that are constrained to a class-level execution - * phase ({@link ExecutionPhase#BEFORE_TEST_CLASS} or - * {@link ExecutionPhase#AFTER_TEST_CLASS}) will be run + * phase ({@link ExecutionPhase#BEFORE_TEST_CLASS BEFORE_TEST_CLASS} or + * {@link ExecutionPhase#AFTER_TEST_CLASS AFTER_TEST_CLASS}) will be run * {@linkplain #beforeTestClass(TestContext) once before all test methods} or * {@linkplain #afterTestMethod(TestContext) once after all test methods}, * respectively. All other scripts and inlined statements will be executed @@ -138,20 +138,22 @@ public class SqlScriptsTestExecutionListener extends AbstractTestExecutionListen * Execute SQL scripts configured via {@link Sql @Sql} for the supplied * {@link TestContext} once per test class before any test method * is run. + * @since 6.1 */ @Override public void beforeTestClass(TestContext testContext) throws Exception { - executeBeforeOrAfterClassSqlScripts(testContext, ExecutionPhase.BEFORE_TEST_CLASS); + executeClassLevelSqlScripts(testContext, ExecutionPhase.BEFORE_TEST_CLASS); } /** * Execute SQL scripts configured via {@link Sql @Sql} for the supplied * {@link TestContext} once per test class after all test methods * have been run. + * @since 6.1 */ @Override public void afterTestClass(TestContext testContext) throws Exception { - executeBeforeOrAfterClassSqlScripts(testContext, ExecutionPhase.AFTER_TEST_CLASS); + executeClassLevelSqlScripts(testContext, ExecutionPhase.AFTER_TEST_CLASS); } /** @@ -189,11 +191,12 @@ public class SqlScriptsTestExecutionListener extends AbstractTestExecutionListen /** * Execute class-level SQL scripts configured via {@link Sql @Sql} for the - * supplied {@link TestContext} and the execution phases - * {@link ExecutionPhase#BEFORE_TEST_CLASS} and - * {@link ExecutionPhase#AFTER_TEST_CLASS}. + * supplied {@link TestContext} and the supplied + * {@link ExecutionPhase#BEFORE_TEST_CLASS BEFORE_TEST_CLASS} or + * {@link ExecutionPhase#AFTER_TEST_CLASS AFTER_TEST_CLASS} execution phase. + * @since 6.1 */ - private void executeBeforeOrAfterClassSqlScripts(TestContext testContext, ExecutionPhase executionPhase) { + private void executeClassLevelSqlScripts(TestContext testContext, ExecutionPhase executionPhase) { Class testClass = testContext.getTestClass(); executeSqlScripts(getSqlAnnotationsFor(testClass), testContext, executionPhase, true); } @@ -286,7 +289,7 @@ public class SqlScriptsTestExecutionListener extends AbstractTestExecutionListen Sql sql, ExecutionPhase executionPhase, TestContext testContext, boolean classLevel) { Assert.isTrue(classLevel || isValidMethodLevelPhase(sql.executionPhase()), - () -> "%s cannot be used on methods".formatted(sql.executionPhase())); + () -> "@SQL execution phase %s cannot be used on methods".formatted(sql.executionPhase())); if (executionPhase != sql.executionPhase()) { return; @@ -302,10 +305,8 @@ public class SqlScriptsTestExecutionListener extends AbstractTestExecutionListen .formatted(executionPhase, testContext.getTestClass().getName())); } - Method testMethod = null; - if (testContext.hasTestMethod()) { - testMethod = testContext.getTestMethod(); - } + boolean methodLevel = !classLevel; + Method testMethod = (methodLevel ? testContext.getTestMethod() : null); String[] scripts = getScripts(sql, testContext.getTestClass(), testMethod, classLevel); List scriptResources = TestContextResourceUtils.convertToResourceList( @@ -357,7 +358,7 @@ public class SqlScriptsTestExecutionListener extends AbstractTestExecutionListen int propagation = (newTxRequired ? TransactionDefinition.PROPAGATION_REQUIRES_NEW : TransactionDefinition.PROPAGATION_REQUIRED); TransactionAttribute txAttr = TestContextTransactionUtils.createDelegatingTransactionAttribute( - testContext, new DefaultTransactionAttribute(propagation)); + testContext, new DefaultTransactionAttribute(propagation), methodLevel); new TransactionTemplate(txMgr, txAttr).executeWithoutResult(s -> populator.execute(finalDataSource)); } } @@ -458,7 +459,8 @@ public class SqlScriptsTestExecutionListener extends AbstractTestExecutionListen private static boolean isValidMethodLevelPhase(ExecutionPhase executionPhase) { // Class-level phases cannot be used on methods. - return executionPhase == ExecutionPhase.BEFORE_TEST_METHOD || - executionPhase == ExecutionPhase.AFTER_TEST_METHOD; + return (executionPhase == ExecutionPhase.BEFORE_TEST_METHOD || + executionPhase == ExecutionPhase.AFTER_TEST_METHOD); } + } diff --git a/spring-test/src/main/java/org/springframework/test/context/support/DefaultTestContext.java b/spring-test/src/main/java/org/springframework/test/context/support/DefaultTestContext.java index d95b2457e6..03b6a25827 100644 --- a/spring-test/src/main/java/org/springframework/test/context/support/DefaultTestContext.java +++ b/spring-test/src/main/java/org/springframework/test/context/support/DefaultTestContext.java @@ -41,7 +41,6 @@ import org.springframework.util.StringUtils; * @author Sam Brannen * @author Juergen Hoeller * @author Rob Harrop - * @author Andreas Ahlenstorf * @since 4.0 */ @SuppressWarnings("serial") @@ -167,11 +166,6 @@ public class DefaultTestContext implements TestContext { return testInstance; } - @Override - public boolean hasTestMethod() { - return this.testMethod != null; - } - @Override public final Method getTestMethod() { Method testMethod = this.testMethod; diff --git a/spring-test/src/main/java/org/springframework/test/context/transaction/TestContextTransactionUtils.java b/spring-test/src/main/java/org/springframework/test/context/transaction/TestContextTransactionUtils.java index addfd8ded2..2945ea14b0 100644 --- a/spring-test/src/main/java/org/springframework/test/context/transaction/TestContextTransactionUtils.java +++ b/spring-test/src/main/java/org/springframework/test/context/transaction/TestContextTransactionUtils.java @@ -228,8 +228,7 @@ 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 (if available) to build the name of the - * transaction. + * the test class and test method to build the name of the transaction. * @param testContext the {@code TestContext} upon which to base the name * @param targetAttribute the {@code TransactionAttribute} to delegate to * @return the delegating {@code TransactionAttribute} @@ -237,9 +236,27 @@ public abstract class TestContextTransactionUtils { public static TransactionAttribute createDelegatingTransactionAttribute( TestContext testContext, TransactionAttribute targetAttribute) { + return createDelegatingTransactionAttribute(testContext, targetAttribute, true); + } + + /** + * Create a delegating {@link TransactionAttribute} for the supplied target + * {@link TransactionAttribute} and {@link TestContext}, using the names of + * the test class and test method (if requested) to build the name of the + * transaction. + * @param testContext the {@code TestContext} upon which to base the name + * @param targetAttribute the {@code TransactionAttribute} to delegate to + * @param includeMethodName {@code true} if the test method's name should be + * included in the name of the transaction + * @return the delegating {@code TransactionAttribute} + * @since 6.1 + */ + public static TransactionAttribute createDelegatingTransactionAttribute( + TestContext testContext, TransactionAttribute targetAttribute, boolean includeMethodName) { + Assert.notNull(testContext, "TestContext must not be null"); Assert.notNull(targetAttribute, "Target TransactionAttribute must not be null"); - return new TestContextTransactionAttribute(targetAttribute, testContext); + return new TestContextTransactionAttribute(targetAttribute, testContext, includeMethodName); } @@ -248,10 +265,12 @@ public abstract class TestContextTransactionUtils { private final String name; - public TestContextTransactionAttribute(TransactionAttribute targetAttribute, TestContext testContext) { + public TestContextTransactionAttribute( + TransactionAttribute targetAttribute, TestContext testContext, boolean includeMethodName) { + super(targetAttribute); - if (testContext.hasTestMethod()) { + if (includeMethodName) { this.name = ClassUtils.getQualifiedMethodName(testContext.getTestMethod(), testContext.getTestClass()); } else { diff --git a/spring-test/src/test/java/org/springframework/test/context/jdbc/AfterTestClassSqlScriptsTests.java b/spring-test/src/test/java/org/springframework/test/context/jdbc/AfterTestClassSqlScriptsTests.java index 1d6633288e..394f7d4540 100644 --- a/spring-test/src/test/java/org/springframework/test/context/jdbc/AfterTestClassSqlScriptsTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/jdbc/AfterTestClassSqlScriptsTests.java @@ -18,55 +18,66 @@ package org.springframework.test.context.jdbc; import javax.sql.DataSource; +import org.junit.jupiter.api.MethodOrderer.OrderAnnotation; import org.junit.jupiter.api.Order; import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.TestMethodOrder; -import org.springframework.core.Ordered; import org.springframework.jdbc.BadSqlGrammarException; import org.springframework.jdbc.core.JdbcTemplate; import org.springframework.test.annotation.Commit; import org.springframework.test.annotation.DirtiesContext; import org.springframework.test.context.TestContext; -import org.springframework.test.context.TestExecutionListener; import org.springframework.test.context.TestExecutionListeners; +import org.springframework.test.context.jdbc.AfterTestClassSqlScriptsTests.VerifySchemaDroppedListener; +import org.springframework.test.context.jdbc.Sql.ExecutionPhase; import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; +import org.springframework.test.context.support.AbstractTestExecutionListener; import org.springframework.test.context.transaction.TestContextTransactionUtils; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.springframework.test.context.TestExecutionListeners.MergeMode.MERGE_WITH_DEFAULTS; +import static org.springframework.test.context.jdbc.Sql.ExecutionPhase.AFTER_TEST_CLASS; /** - * Verifies that {@link Sql @Sql} with {@link Sql.ExecutionPhase#AFTER_TEST_CLASS} is run after all tests in the class - * have been run. + * Verifies that {@link Sql @Sql} with {@link ExecutionPhase#AFTER_TEST_CLASS} + * is run after all tests in the class have been run. * * @author Andreas Ahlenstorf + * @author Sam Brannen * @since 6.1 */ @SpringJUnitConfig(PopulatedSchemaDatabaseConfig.class) -@DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) -@Sql(value = {"drop-schema.sql"}, executionPhase = Sql.ExecutionPhase.AFTER_TEST_CLASS) -@TestExecutionListeners( - value = AfterTestClassSqlScriptsTests.VerifyTestExecutionListener.class, - mergeMode = TestExecutionListeners.MergeMode.MERGE_WITH_DEFAULTS -) +@TestMethodOrder(OrderAnnotation.class) +@DirtiesContext +@Sql(scripts = "drop-schema.sql", executionPhase = AFTER_TEST_CLASS) +@Commit +@TestExecutionListeners(listeners = VerifySchemaDroppedListener.class, mergeMode = MERGE_WITH_DEFAULTS) class AfterTestClassSqlScriptsTests extends AbstractTransactionalTests { @Test @Order(1) - @Sql(scripts = "data-add-catbert.sql") - @Commit + @Sql("data-add-catbert.sql") void databaseHasBeenInitialized() { assertUsers("Catbert"); } @Test @Order(2) - @Sql(scripts = "data-add-dogbert.sql") - @Commit + @Sql("data-add-dogbert.sql") void databaseIsNotWipedBetweenTests() { assertUsers("Catbert", "Dogbert"); } - static class VerifyTestExecutionListener implements TestExecutionListener, Ordered { + + static class VerifySchemaDroppedListener extends AbstractTestExecutionListener { + + @Override + public int getOrder() { + // Must run before DirtiesContextTestExecutionListener. Otherwise, the + // old data source will be removed and replaced with a new one. + return 3001; + } @Override public void afterTestClass(TestContext testContext) throws Exception { @@ -74,14 +85,9 @@ class AfterTestClassSqlScriptsTests extends AbstractTransactionalTests { JdbcTemplate jdbcTemplate = new JdbcTemplate(dataSource); assertThatExceptionOfType(BadSqlGrammarException.class) - .isThrownBy(() -> jdbcTemplate.queryForList("SELECT name FROM user", String.class)); - } - - @Override - public int getOrder() { - // Must run before DirtiesContextTestExecutionListener. Otherwise, the old data source will be removed and - // replaced with a new one. - return 3001; + .isThrownBy(() -> jdbcTemplate.queryForList("SELECT name FROM user", String.class)) + .withMessageContaining("user"); } } + } diff --git a/spring-test/src/test/java/org/springframework/test/context/jdbc/BeforeTestClassSqlScriptsTests.java b/spring-test/src/test/java/org/springframework/test/context/jdbc/BeforeTestClassSqlScriptsTests.java index acaae772cd..faded5ce5a 100644 --- a/spring-test/src/test/java/org/springframework/test/context/jdbc/BeforeTestClassSqlScriptsTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/jdbc/BeforeTestClassSqlScriptsTests.java @@ -19,21 +19,24 @@ package org.springframework.test.context.jdbc; import org.junit.jupiter.api.Test; import org.springframework.test.annotation.DirtiesContext; +import org.springframework.test.context.jdbc.Sql.ExecutionPhase; import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; +import static org.springframework.test.context.jdbc.Sql.ExecutionPhase.BEFORE_TEST_CLASS; import static org.springframework.test.context.jdbc.SqlMergeMode.MergeMode.MERGE; import static org.springframework.test.context.jdbc.SqlMergeMode.MergeMode.OVERRIDE; /** - * Verifies that {@link Sql @Sql} with {@link Sql.ExecutionPhase#BEFORE_TEST_CLASS} is run before all tests in the class - * have been run. + * Verifies that {@link Sql @Sql} with {@link ExecutionPhase#BEFORE_TEST_CLASS} + * is run before all tests in the class have been run. * * @author Andreas Ahlenstorf + * @author Sam Brannen * @since 6.1 */ -@SpringJUnitConfig(classes = EmptyDatabaseConfig.class) +@SpringJUnitConfig(EmptyDatabaseConfig.class) @DirtiesContext -@Sql(value = {"schema.sql", "data-add-catbert.sql"}, executionPhase = Sql.ExecutionPhase.BEFORE_TEST_CLASS) +@Sql(scripts = {"schema.sql", "data-add-catbert.sql"}, executionPhase = BEFORE_TEST_CLASS) class BeforeTestClassSqlScriptsTests extends AbstractTransactionalTests { @Test @@ -52,7 +55,7 @@ class BeforeTestClassSqlScriptsTests extends AbstractTransactionalTests { @Sql({"data-add-dogbert.sql"}) @SqlMergeMode(OVERRIDE) void overrideDoesNotAffectClassLevelPhase() { - assertUsers("Dogbert", "Catbert"); + assertUsers("Catbert", "Dogbert"); } } diff --git a/spring-test/src/test/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListenerTests.java b/spring-test/src/test/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListenerTests.java index ff9f3b953f..c71898e966 100644 --- a/spring-test/src/test/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListenerTests.java +++ b/spring-test/src/test/java/org/springframework/test/context/jdbc/SqlScriptsTestExecutionListenerTests.java @@ -30,6 +30,8 @@ import static org.assertj.core.api.Assertions.assertThatIllegalStateException; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.BDDMockito.given; import static org.mockito.Mockito.mock; +import static org.springframework.test.context.jdbc.Sql.ExecutionPhase.AFTER_TEST_CLASS; +import static org.springframework.test.context.jdbc.Sql.ExecutionPhase.BEFORE_TEST_CLASS; /** * Unit tests for {@link SqlScriptsTestExecutionListener}. @@ -58,7 +60,6 @@ class SqlScriptsTestExecutionListenerTests { void missingValueAndScriptsAndStatementsAtMethodLevel() throws Exception { Class clazz = MissingValueAndScriptsAndStatementsAtMethodLevel.class; BDDMockito.> given(testContext.getTestClass()).willReturn(clazz); - given(testContext.hasTestMethod()).willReturn(true); given(testContext.getTestMethod()).willReturn(clazz.getDeclaredMethod("foo")); assertExceptionContains(clazz.getSimpleName() + ".foo" + ".sql"); @@ -109,24 +110,22 @@ class SqlScriptsTestExecutionListenerTests { void beforeTestClassOnMethod() throws Exception { Class clazz = ClassLevelExecutionPhaseOnMethod.class; BDDMockito.> given(testContext.getTestClass()).willReturn(clazz); - given(testContext.hasTestMethod()).willReturn(true); given(testContext.getTestMethod()).willReturn(clazz.getDeclaredMethod("beforeTestClass")); assertThatIllegalArgumentException() .isThrownBy(() -> listener.beforeTestMethod(testContext)) - .withMessage("BEFORE_TEST_CLASS cannot be used on methods"); + .withMessage("@SQL execution phase BEFORE_TEST_CLASS cannot be used on methods"); } @Test void afterTestClassOnMethod() throws Exception { Class clazz = ClassLevelExecutionPhaseOnMethod.class; BDDMockito.> given(testContext.getTestClass()).willReturn(clazz); - given(testContext.hasTestMethod()).willReturn(true); given(testContext.getTestMethod()).willReturn(clazz.getDeclaredMethod("afterTestClass")); assertThatIllegalArgumentException() .isThrownBy(() -> listener.beforeTestMethod(testContext)) - .withMessage("AFTER_TEST_CLASS cannot be used on methods"); + .withMessage("@SQL execution phase AFTER_TEST_CLASS cannot be used on methods"); } private void assertExceptionContains(String msg) throws Exception { @@ -175,12 +174,13 @@ class SqlScriptsTestExecutionListenerTests { static class ClassLevelExecutionPhaseOnMethod { - @Sql(scripts = "foo.sql", executionPhase = Sql.ExecutionPhase.BEFORE_TEST_CLASS) + @Sql(scripts = "foo.sql", executionPhase = BEFORE_TEST_CLASS) public void beforeTestClass() { } - @Sql(scripts = "foo.sql", executionPhase = Sql.ExecutionPhase.AFTER_TEST_CLASS) + @Sql(scripts = "foo.sql", executionPhase = AFTER_TEST_CLASS) public void afterTestClass() { } } + }