Merge pull request #64 from Terasology/fix/storeContext

fix: more consistent disposal of resources at the end of the test

src/main/java/org/terasology/moduletestingenvironment/IsolatedMTEExtension.java

@@ -1,43 +1,19 @@
-// Copyright 2020 The Terasology Foundation
+// Copyright 2021 The Terasology Foundation
 // SPDX-License-Identifier: Apache-2.0
 
 package org.terasology.moduletestingenvironment;
 
-import org.junit.jupiter.api.extension.AfterAllCallback;
-import org.junit.jupiter.api.extension.AfterEachCallback;
-import org.junit.jupiter.api.extension.BeforeAllCallback;
-import org.junit.jupiter.api.extension.ExtensionContext;
-import org.junit.jupiter.api.extension.ParameterResolver;
-import org.junit.jupiter.api.extension.TestInstancePostProcessor;
-
 /**
  * Subclass of {@link MTEExtension} which isolates all test cases by creating a new engine for each test. This is much
  * slower since it runs the startup and shutdown process for all tests. You should use {@link MTEExtension} unless
  * you're certain that you need to use this class.
  * <p>
  * Use this within {@link org.junit.jupiter.api.extension.ExtendWith}
  */
-public class IsolatedMTEExtension extends MTEExtension implements BeforeAllCallback, AfterAllCallback,
-        AfterEachCallback, ParameterResolver, TestInstancePostProcessor {
-    @Override
-    public void afterAll(ExtensionContext context) throws Exception {
-        // don't call super
-    }
-
-    @Override
-    public void beforeAll(ExtensionContext context) throws Exception {
-        // don't call super
-    }
-
-    @Override
-    public void afterEach(ExtensionContext context) throws Exception {
-        super.afterAll(context);
-    }
-
-    @Override
-    public void postProcessTestInstance(Object testInstance, ExtensionContext extensionContext) throws Exception {
-        // beforeEach would be run after postProcess so postProcess would NPE, so we initialize the MTE here beforehand
-        super.beforeAll(extensionContext);
-        super.postProcessTestInstance(testInstance, extensionContext);
+public class IsolatedMTEExtension extends MTEExtension {
+    {
+        // Resources are not shared between namespaces. We increase isolation by using a different
+        // namespace for every test method.
+        helperLifecycle = Scopes.PER_METHOD;
     }
 }

src/main/java/org/terasology/moduletestingenvironment/MTEExtension.java

@@ -9,7 +9,6 @@
 import ch.qos.logback.core.util.StatusPrinter;
 import com.google.common.collect.Sets;
 import org.junit.jupiter.api.Nested;
-import org.junit.jupiter.api.extension.AfterAllCallback;
 import org.junit.jupiter.api.extension.BeforeAllCallback;
 import org.junit.jupiter.api.extension.ExtensionContext;
 import org.junit.jupiter.api.extension.ParameterContext;
@@ -25,10 +24,11 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.Arrays;
-import java.util.HashMap;
+import java.util.Collections;
 import java.util.LinkedList;
 import java.util.List;
-import java.util.Map;
+import java.util.Set;
+import java.util.function.Function;
 
 /**
  * Junit 5 Extension for using {@link ModuleTestingHelper} in your test.
@@ -47,61 +47,31 @@
  * <p>
  * Use this within {@link org.junit.jupiter.api.extension.ExtendWith}
  */
-public class MTEExtension implements BeforeAllCallback, AfterAllCallback, ParameterResolver, TestInstancePostProcessor {
+public class MTEExtension implements BeforeAllCallback, ParameterResolver, TestInstancePostProcessor {
 
     static final String LOGBACK_RESOURCE = "default-logback.xml";
-
-    protected final Map<Class<?>, ModuleTestingHelper> helperContexts = new HashMap<>();
+    protected Function<ExtensionContext, ExtensionContext.Namespace> helperLifecycle = Scopes.PER_CLASS;
+    protected Function<ExtensionContext, Class<?>> getTestClass = Scopes::getTopTestClass;
 
     @Override
-    public void afterAll(ExtensionContext context) throws Exception {
-        Class<?> testClass = context.getRequiredTestClass();
-        if (testClass.isAnnotationPresent(Nested.class)) {
-            // nested classes get torn down in the parent
-            return;
-        }
-
-        // Could be null if an exception interrupts before setup is complete.
-        if (helperContexts.get(testClass) != null) {
-            helperContexts.get(testClass).tearDown();
-        }
-    }
-
-    @Override
-    public void beforeAll(ExtensionContext context) throws Exception {
+    public void beforeAll(ExtensionContext context) {
         if (context.getRequiredTestClass().isAnnotationPresent(Nested.class)) {
-            // nested classes get set up in the parent
-            ModuleTestingHelper parentHelper = helperContexts.get(context.getRequiredTestClass().getEnclosingClass());
-            helperContexts.put(context.getRequiredTestClass(), parentHelper);
-            return;
+            return;  // nested classes get set up in the parent
         }
-
         setupLogging();
-
-        Dependencies dependencies = context.getRequiredTestClass().getAnnotation(Dependencies.class);
-        UseWorldGenerator useWorldGenerator = context.getRequiredTestClass().getAnnotation(UseWorldGenerator.class);
-        ModuleTestingHelper helperContext = new ModuleTestingHelper();
-        if (dependencies != null) {
-            helperContext.setDependencies(Sets.newHashSet(dependencies.value()));
-        }
-        if (useWorldGenerator != null) {
-            helperContext.setWorldGeneratorUri(useWorldGenerator.value());
-        }
-        helperContext.setup();
-        helperContexts.put(context.getRequiredTestClass(), helperContext);
     }
 
     @Override
     public boolean supportsParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
-        ModuleTestingHelper helper = helperContexts.get(extensionContext.getRequiredTestClass());
+        ModuleTestingHelper helper = getHelper(extensionContext);
         return helper.getHostContext().get(parameterContext.getParameter().getType()) != null
                 || parameterContext.getParameter().getType().equals(ModuleTestingEnvironment.class)
                 || parameterContext.getParameter().getType().equals(ModuleTestingHelper.class);
     }
 
     @Override
     public Object resolveParameter(ParameterContext parameterContext, ExtensionContext extensionContext) throws ParameterResolutionException {
-        ModuleTestingHelper helper = helperContexts.get(extensionContext.getRequiredTestClass());
+        ModuleTestingHelper helper = getHelper(extensionContext);
         Class<?> type = parameterContext.getParameter().getType();
 
         return getDIInstance(helper, type);
@@ -115,8 +85,8 @@ private Object getDIInstance(ModuleTestingHelper helper, Class<?> type) {
     }
 
     @Override
-    public void postProcessTestInstance(Object testInstance, ExtensionContext extensionContext) throws Exception {
-        ModuleTestingHelper helper = helperContexts.get(extensionContext.getRequiredTestClass());
+    public void postProcessTestInstance(Object testInstance, ExtensionContext extensionContext) {
+        ModuleTestingHelper helper = getHelper(extensionContext);
         List<IllegalAccessException> exceptionList = new LinkedList<>();
         Class<?> type = testInstance.getClass();
         while (type != null) {
@@ -140,6 +110,37 @@ public void postProcessTestInstance(Object testInstance, ExtensionContext extens
         }
     }
 
+    public String getWorldGeneratorUri(ExtensionContext context) {
+        UseWorldGenerator useWorldGenerator = getTestClass.apply(context).getAnnotation(UseWorldGenerator.class);
+        return useWorldGenerator != null ? useWorldGenerator.value() : null;
+    }
+
+    public Set<String> getDependencyNames(ExtensionContext context) {
+        Dependencies dependencies = getTestClass.apply(context).getAnnotation(Dependencies.class);
+        return dependencies != null ? Sets.newHashSet(dependencies.value()) : Collections.emptySet();
+    }
+
+    /**
+     * Get the ModuleTestingHelper for this test.
+     * <p>
+     * The new ModuleTestingHelper instance is configured using the {@link Dependencies} and {@link UseWorldGenerator}
+     * annotations for the test class.
+     * <p>
+     * This will create a new instance when necessary. It will be stored in the
+     * {@link ExtensionContext} for reuse between tests that wish to avoid the expense of creating a new
+     * instance every time, and will be disposed of when the context closes.
+     *
+     * @param context for the current test
+     * @return configured for this test
+     */
+    protected ModuleTestingHelper getHelper(ExtensionContext context) {
+        ExtensionContext.Store store = context.getStore(helperLifecycle.apply(context));
+        HelperCleaner autoCleaner = store.getOrComputeIfAbsent(
+                HelperCleaner.class, k -> new HelperCleaner(getDependencyNames(context), getWorldGeneratorUri(context)),
+                HelperCleaner.class);
+        return autoCleaner.helper;
+    }
+
     /**
      * Apply our default logback configuration to the logger.
      * <p>
@@ -174,4 +175,41 @@ void setupLogging() {
             StatusPrinter.printInCaseOfErrorsOrWarnings(context);
         }
     }
+
+    /**
+     * Manages a ModuleTestingHelper for storage in an ExtensionContext.
+     * <p>
+     * Implements {@link ExtensionContext.Store.CloseableResource CloseableResource} to dispose of
+     * the {@link ModuleTestingHelper} when the context is closed.
+     */
+    static class HelperCleaner implements ExtensionContext.Store.CloseableResource {
+        protected ModuleTestingHelper helper;
+
+        HelperCleaner(ModuleTestingHelper helper) {
+            this.helper = helper;
+        }
+
+        HelperCleaner(Set<String> dependencyNames, String worldGeneratorUri) {
+            this(setupHelper(new ModuleTestingHelper(), dependencyNames, worldGeneratorUri));
+        }
+
+        protected static ModuleTestingHelper setupHelper(ModuleTestingHelper helper, Set<String> dependencyNames,
+                                                         String worldGeneratorUri) {
+            // This is a shim to fit the existing ModuleTestingEnvironment interface, but
+            // I expect we can make things cleaner after we drop the old interface that is
+            // also pretending to be a TestCase class itself.
+            helper.setDependencies(dependencyNames);
+            if (worldGeneratorUri != null) {
+                helper.setWorldGeneratorUri(worldGeneratorUri);
+            }
+            helper.setup();
+            return helper;
+        }
+
+        @Override
+        public void close() {
+            helper.tearDown();
+            helper = null;
+        }
+    }
 }

src/main/java/org/terasology/moduletestingenvironment/ModuleTestingEnvironment.java

@@ -54,6 +54,7 @@
 import org.terasology.gestalt.module.ModuleRegistry;
 
 import java.io.IOException;
+import java.io.UncheckedIOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.Collections;
@@ -151,7 +152,7 @@ public class ModuleTestingEnvironment {
     public static final long DEFAULT_SAFETY_TIMEOUT = 60000;
     public static final long DEFAULT_GAME_TIME_TIMEOUT = 30000;
     private static final Logger logger = LoggerFactory.getLogger(ModuleTestingEnvironment.class);
-    private Set<String> dependencies = Sets.newHashSet("engine");
+    private final Set<String> dependencies = Sets.newHashSet("engine");
     private String worldGeneratorUri = "moduletestingenvironment:dummy";
     private boolean doneLoading;
     private TerasologyEngine host;
@@ -166,13 +167,15 @@ public class ModuleTestingEnvironment {
      * Set up and start the engine as configured via this environment.
      * <p>
      * Every instance should be shut down properly by calling {@link #tearDown()}.
-     *
-     * @throws Exception
      */
     @BeforeEach
-    public void setup() throws Exception {
+    public void setup() {
         mockPathManager();
-        host = createHost();
+        try {
+            host = createHost();
+        } catch (IOException e) {
+            throw new UncheckedIOException(e);
+        }
         ScreenGrabber grabber = mock(ScreenGrabber.class);
         hostContext.put(ScreenGrabber.class, grabber);
         CoreRegistry.put(GameEngine.class, host);
@@ -217,7 +220,10 @@ public Set<String> getDependencies() {
      */
     void setDependencies(Set<String> dependencies) {
         Preconditions.checkState(host == null, "You cannot set Dependencies after setup");
-        this.dependencies = dependencies;
+        synchronized (this.dependencies) {
+            this.dependencies.clear();
+            this.dependencies.addAll(dependencies);
+        }
     }
 
     /**

src/main/java/org/terasology/moduletestingenvironment/Scopes.java

@@ -0,0 +1,42 @@
+// Copyright 2021 The Terasology Foundation
+// SPDX-License-Identifier: Apache-2.0
+
+package org.terasology.moduletestingenvironment;
+
+import com.google.common.collect.ObjectArrays;
+import org.junit.jupiter.api.Nested;
+import org.junit.jupiter.api.extension.ExtensionContext;
+
+import java.util.function.Function;
+
+public final class Scopes {
+
+    /** One per top-level test class. */
+    public static final Function<ExtensionContext, ExtensionContext.Namespace> PER_CLASS = context -> mteNamespace(getTopTestClass(context));
+
+    /** One per test method. */
+    public static final Function<ExtensionContext, ExtensionContext.Namespace> PER_METHOD = context -> mteNamespace(context.getTestMethod());
+
+    private Scopes() { };
+
+    static ExtensionContext.Namespace mteNamespace(Object... parts) {
+        // Start with this Extension, so it's clear where this came from.
+        return ExtensionContext.Namespace.create(ObjectArrays.concat(MTEExtension.class, parts));
+    }
+
+    /**
+     * The outermost class defining this test.
+     * <p>
+     * For <a href="https://junit.org/junit5/docs/current/user-guide/#writing-tests-nested">nested tests</a>, this
+     * returns the outermost class in which this test is nested.
+     * <p>
+     * Most tests are not nested, in which case this returns the class defining the test.
+     *
+     * @param context for the current test
+     * @return a test class
+     */
+    public static Class<?> getTopTestClass(ExtensionContext context) {
+        Class<?> testClass = context.getRequiredTestClass();
+        return testClass.isAnnotationPresent(Nested.class) ? testClass.getEnclosingClass() : testClass;
+    }
+}

src/test/java/org/terasology/moduletestingenvironment/IsolatedEngineTest.java

@@ -25,7 +25,7 @@
 
 @Tag("MteTest")
 @ExtendWith(IsolatedMTEExtension.class)
-@Dependencies({"engine", "ModuleTestingEnvironment"})
+@Dependencies("ModuleTestingEnvironment")
 @TestMethodOrder(MethodOrderer.OrderAnnotation.class)
 public class IsolatedEngineTest {
     private final Set<EntityManager> entityManagerSet = Sets.newHashSet();
@@ -55,8 +55,8 @@ public void someTest() {
     public void someOtherTest() {
         // make sure we don't reuse the EntityManager
         assertFalse(entityManagerSet.contains(entityManager));
-        entityManagerSet.add(entityManager);
 
-        assertFalse(entity.getComponent(DummyComponent.class).eventReceived);
+        assertFalse(entity.getComponent(DummyComponent.class).eventReceived,
+                "This entity should not have its field set yet!");
     }
 }