Fix AbstractEntryBasedExtension backups in nested tests (#480 / #481)

`ExtensionContext$Store#get(Object)` queries ancestors of the current
context if no value is stored under the supplied key. This can mess up
backups of system properties and environment variables in nested
classes if, for instance, the current test method has no corresponding
annotation but the surrounding container. Then, the first test
execution accidentally restores the backup, which leads to incorrect
system property / environment variables settings.

This PR sidesteps this issue by ensuring that the store / backup key
is unique per test or container, respectively, by using the given
display name.

Closes: #480
PR: #481

README.md

@@ -111,7 +111,7 @@ The least we can do is to thank them and list some of their accomplishments here
 #### 2021
 
 * [Cory Thomas](https://github.com/dump247) contributed the `minSuccess` attribute in [retrying tests](https://junit-pioneer.org/docs/retrying-test/) (#408 / #430)
-* [Daniel Kraus](https://github.com/beatngu13) fixed bugs in the environment variable and system property extensions (#432 / #433, #448 / #449, and #473 / #476) and improved the build process (#482 / #483)
+* [Daniel Kraus](https://github.com/beatngu13) fixed bugs in the environment variable and system property extensions (#432 / #433, #448 / #449, and more) and improved the build process (#482 / #483)
 * [Slawomir Jaranowski](https://github.com/slawekjaranowski) Migrate to new Shipkit plugins (#410 / #419)
 * [Stefano Cordio](https://github.com/scordio) contributed [the Cartesian Enum source](https://junit-pioneer.org/docs/cartesian-product/#cartesianenumsource) (#379 / #409 and #414 / #453)
 

src/main/java/org/junitpioneer/jupiter/AbstractEntryBasedExtension.java

@@ -36,14 +36,6 @@
 abstract class AbstractEntryBasedExtension<K, V>
 		implements BeforeAllCallback, BeforeEachCallback, AfterAllCallback, AfterEachCallback {
 
-	private static final String BACKUP = "Backup";
-
-	/**
-	 * @param context The current extension context.
-	 * @return <code>true</code> if one or more of the extension's annotations are present.
-	 */
-	protected abstract boolean isAnnotationPresent(ExtensionContext context);
-
 	/**
 	 * @param context The current extension context.
 	 * @return The entry keys to be cleared.
@@ -124,7 +116,7 @@ private void preventClearAndSetSameEntries(Collection<K> entriesToClear, Collect
 
 	private void storeOriginalEntries(ExtensionContext context, Collection<K> entriesToClear,
 			Collection<K> entriesToSet) {
-		getStore(context).put(BACKUP, new EntriesBackup(entriesToClear, entriesToSet));
+		getStore(context).put(getStoreKey(context), new EntriesBackup(entriesToClear, entriesToSet));
 	}
 
 	private void clearEntries(Collection<K> entriesToClear) {
@@ -137,8 +129,7 @@ private void setEntries(Map<K, V> entriesToSet) {
 
 	@Override
 	public void afterEach(ExtensionContext context) throws Exception {
-		if (isAnnotationPresent(context))
-			restoreOriginalEntries(context);
+		restoreOriginalEntries(context);
 	}
 
 	@Override
@@ -147,18 +138,26 @@ public void afterAll(ExtensionContext context) throws Exception {
 	}
 
 	private void restoreOriginalEntries(ExtensionContext context) {
-		getStore(context).get(BACKUP, EntriesBackup.class).restoreBackup();
+		getStore(context).getOrDefault(getStoreKey(context), EntriesBackup.class, new EntriesBackup()).restoreBackup();
 	}
 
 	private Store getStore(ExtensionContext context) {
 		return context.getStore(Namespace.create(getClass()));
 	}
 
+	private Object getStoreKey(ExtensionContext context) {
+		return context.getUniqueId();
+	}
+
 	private class EntriesBackup {
 
 		private final Set<K> entriesToClear = new HashSet<>();
 		private final Map<K, V> entriesToSet = new HashMap<>();
 
+		public EntriesBackup() {
+			// empty backup
+		}
+
 		public EntriesBackup(Collection<K> entriesToClear, Collection<K> entriesToSet) {
 			Stream.concat(entriesToClear.stream(), entriesToSet.stream()).forEach(entry -> {
 				V backup = AbstractEntryBasedExtension.this.getEntry(entry);

src/main/java/org/junitpioneer/jupiter/EnvironmentVariableExtension.java

@@ -18,7 +18,6 @@
 
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.platform.commons.support.AnnotationSupport;
-import org.junitpioneer.internal.PioneerAnnotationUtils;
 import org.junitpioneer.internal.PioneerUtils;
 
 class EnvironmentVariableExtension extends AbstractEntryBasedExtension<String, String> {
@@ -28,13 +27,6 @@ class EnvironmentVariableExtension extends AbstractEntryBasedExtension<String, S
 	static final String WARNING_KEY = EnvironmentVariableExtension.class.getSimpleName();
 	static final String WARNING_VALUE = "This extension uses reflection to mutate JDK-internal state, which is fragile. Check the Javadoc or documentation for more details.";
 
-	@Override
-	protected boolean isAnnotationPresent(ExtensionContext context) {
-		return PioneerAnnotationUtils
-				.isAnyRepeatableAnnotationPresent(context, ClearEnvironmentVariable.class,
-					SetEnvironmentVariable.class);
-	}
-
 	@Override
 	protected Set<String> entriesToClear(ExtensionContext context) {
 		return AnnotationSupport

src/main/java/org/junitpioneer/jupiter/SystemPropertyExtension.java

@@ -17,17 +17,10 @@
 
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.platform.commons.support.AnnotationSupport;
-import org.junitpioneer.internal.PioneerAnnotationUtils;
 import org.junitpioneer.internal.PioneerUtils;
 
 class SystemPropertyExtension extends AbstractEntryBasedExtension<String, String> {
 
-	@Override
-	protected boolean isAnnotationPresent(ExtensionContext context) {
-		return PioneerAnnotationUtils
-				.isAnyRepeatableAnnotationPresent(context, ClearSystemProperty.class, SetSystemProperty.class);
-	}
-
 	@Override
 	protected Set<String> entriesToClear(ExtensionContext context) {
 		return AnnotationSupport

src/test/java/org/junitpioneer/jupiter/AbstractEntryBasedExtensionTest.java

@@ -38,6 +38,7 @@ void setUp() {
 	}
 
 	@Test
+	@Issue("432")
 	@DisplayName("should not mix backups of different extensions on clear environment variable and clear system property")
 	void shouldNotMixBackupsOfDifferentExtensionsOnClearEnvironmentVariableAndClearSystemProperty() {
 		PioneerTestKit.executeTestMethod(MixBackupsTestCases.class, "clearEnvironmentVariableAndClearSystemProperty");
@@ -47,6 +48,7 @@ void shouldNotMixBackupsOfDifferentExtensionsOnClearEnvironmentVariableAndClearS
 	}
 
 	@Test
+	@Issue("432")
 	@DisplayName("should not mix backups of different extensions on set environment variable and set system property")
 	void shouldNotMixBackupsOfDifferentExtensionsOnSetEnvironmentVariableAndSetSystemProperty() {
 		PioneerTestKit.executeTestMethod(MixBackupsTestCases.class, "setEnvironmentVariableAndSetSystemProperty");
@@ -56,6 +58,7 @@ void shouldNotMixBackupsOfDifferentExtensionsOnSetEnvironmentVariableAndSetSyste
 	}
 
 	@Test
+	@Issue("432")
 	@DisplayName("should not mix backups of different extensions on clear environment variable and set system property")
 	void shouldNotMixBackupsOfDifferentExtensionsOnClearEnvironmentVariableAndSetSystemProperty() {
 		PioneerTestKit.executeTestMethod(MixBackupsTestCases.class, "clearEnvironmentVariableAndSetSystemProperty");
@@ -65,6 +68,7 @@ void shouldNotMixBackupsOfDifferentExtensionsOnClearEnvironmentVariableAndSetSys
 	}
 
 	@Test
+	@Issue("432")
 	@DisplayName("should not mix backups of different extensions on set environment variable and clear system property")
 	void shouldNotMixBackupsOfDifferentExtensionsOnSetEnvironmentVariableAndClearSystemProperty() {
 		PioneerTestKit.executeTestMethod(MixBackupsTestCases.class, "setEnvironmentVariableAndClearSystemProperty");

src/test/java/org/junitpioneer/jupiter/EnvironmentVariableExtensionTests.java

@@ -22,8 +22,11 @@
 import org.junit.jupiter.api.BeforeEach;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.MethodOrderer.OrderAnnotation;
 import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestMethodOrder;
 import org.junit.jupiter.api.extension.ExtensionConfigurationException;
 import org.junitpioneer.testkit.ExecutionResults;
 
@@ -159,6 +162,7 @@ void methodLevelShouldOverwriteClassLevel() {
 		}
 
 		@Test
+		@Issue("473")
 		@DisplayName("method level should not clash (in terms of duplicate entries) with class level")
 		@SetEnvironmentVariable(key = "set envvar A", value = "new A")
 		void methodLevelShouldNotClashWithClassLevel() {
@@ -180,13 +184,25 @@ void methodLevelShouldNotClashWithClassLevel() {
 	class NestedEnvironmentVariableTests {
 
 		@Nested
+		@TestMethodOrder(OrderAnnotation.class)
 		@DisplayName("without EnvironmentVariable annotations")
 		class NestedClass {
 
 			@Test
+			@Order(1)
 			@ReadsEnvironmentVariable
 			@DisplayName("environment variables should be set from enclosed class when they are not provided in nested")
-			public void shouldSetEnvironmentVariableFromEnclosedClass() {
+			void shouldSetEnvironmentVariableFromEnclosedClass() {
+				assertThat(systemEnvironmentVariable("set envvar A")).isNull();
+				assertThat(systemEnvironmentVariable("set envvar B")).isEqualTo("new B");
+			}
+
+			@Test
+			@Issue("480")
+			@Order(2)
+			@ReadsEnvironmentVariable
+			@DisplayName("environment variables should be set from enclosed class after restore")
+			void shouldSetEnvironmentVariableFromEnclosedClassAfterRestore() {
 				assertThat(systemEnvironmentVariable("set envvar A")).isNull();
 				assertThat(systemEnvironmentVariable("set envvar B")).isEqualTo("new B");
 			}
@@ -201,14 +217,14 @@ class AnnotatedNestedClass {
 			@Test
 			@ReadsEnvironmentVariable
 			@DisplayName("environment variable should be set from nested class when it is provided")
-			public void shouldSetEnvironmentVariableFromNestedClass() {
+			void shouldSetEnvironmentVariableFromNestedClass() {
 				assertThat(systemEnvironmentVariable("set envvar B")).isEqualTo("newer B");
 			}
 
 			@Test
 			@SetEnvironmentVariable(key = "set envvar B", value = "newest B")
 			@DisplayName("environment variable should be set from method when it is provided")
-			public void shouldSetEnvironmentVariableFromMethodOfNestedClass() {
+			void shouldSetEnvironmentVariableFromMethodOfNestedClass() {
 				assertThat(systemEnvironmentVariable("set envvar B")).isEqualTo("newest B");
 			}
 
@@ -340,6 +356,7 @@ void shouldFailWhenSetSameEnvironmentVariableTwice() {
 	class InheritanceTests extends InheritanceBaseTest {
 
 		@Test
+		@Issue("448")
 		@DisplayName("should inherit clear and set annotations")
 		void shouldInheritClearAndSetProperty() {
 			assertThat(systemEnvironmentVariable("set envvar A")).isNull();

src/test/java/org/junitpioneer/jupiter/SystemPropertyExtensionTests.java

@@ -17,8 +17,11 @@
 import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.Disabled;
 import org.junit.jupiter.api.DisplayName;
+import org.junit.jupiter.api.MethodOrderer.OrderAnnotation;
 import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.Order;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.TestMethodOrder;
 import org.junit.jupiter.api.extension.ExtensionConfigurationException;
 import org.junitpioneer.testkit.ExecutionResults;
 import org.junitpioneer.testkit.PioneerTestKit;
@@ -151,6 +154,7 @@ void methodLevelShouldOverwriteClassLevel() {
 		}
 
 		@Test
+		@Issue("473")
 		@DisplayName("method level should not clash (in terms of duplicate entries) with class level")
 		@SetSystemProperty(key = "set prop A", value = "new A")
 		void methodLevelShouldNotClashWithClassLevel() {
@@ -172,13 +176,25 @@ void methodLevelShouldNotClashWithClassLevel() {
 	class NestedSystemPropertyTests {
 
 		@Nested
+		@TestMethodOrder(OrderAnnotation.class)
 		@DisplayName("without SystemProperty annotations")
 		class NestedClass {
 
 			@Test
+			@Order(1)
 			@ReadsSystemProperty
 			@DisplayName("system properties should be set from enclosed class when they are not provided in nested")
-			public void shouldSetSystemPropertyFromEnclosedClass() {
+			void shouldSetSystemPropertyFromEnclosedClass() {
+				assertThat(System.getProperty("set prop A")).isNull();
+				assertThat(System.getProperty("set prop B")).isEqualTo("new B");
+			}
+
+			@Test
+			@Issue("480")
+			@Order(2)
+			@ReadsSystemProperty
+			@DisplayName("system properties should be set from enclosed class after restore")
+			void shouldSetSystemPropertyFromEnclosedClassAfterRestore() {
 				assertThat(System.getProperty("set prop A")).isNull();
 				assertThat(System.getProperty("set prop B")).isEqualTo("new B");
 			}
@@ -193,14 +209,14 @@ class AnnotatedNestedClass {
 			@Test
 			@ReadsSystemProperty
 			@DisplayName("system property should be set from nested class when it is provided")
-			public void shouldSetSystemPropertyFromNestedClass() {
+			void shouldSetSystemPropertyFromNestedClass() {
 				assertThat(System.getProperty("set prop B")).isEqualTo("newer B");
 			}
 
 			@Test
 			@SetSystemProperty(key = "set prop B", value = "newest B")
 			@DisplayName("system property should be set from method when it is provided")
-			public void shouldSetSystemPropertyFromMethodOfNestedClass() {
+			void shouldSetSystemPropertyFromMethodOfNestedClass() {
 				assertThat(System.getProperty("set prop B")).isEqualTo("newest B");
 			}
 
@@ -274,6 +290,7 @@ void shouldFailWhenSetSameSystemPropertyTwice() {
 	class InheritanceTests extends InheritanceBaseTest {
 
 		@Test
+		@Issue("448")
 		@DisplayName("should inherit clear and set annotations")
 		void shouldInheritClearAndSetProperty() {
 			assertThat(System.getProperty("set prop A")).isNull();