Fixed memory leak: JUnit4Core keeps reference to all Failures, Failur…

…es keep reference to the respective AssertionError, ArchRule.AssertionError kept reference to EvaluationResult, thus making it impossible to GC big results over the whole JUnit run. Signed-off-by: Peter Gafert <[email protected]>
TNG · Nov 20, 2018 · 8b859a4 · 8b859a4
1 parent c8085cb
commit 8b859a4
Show file tree

Hide file tree

Showing 6 changed files with 112 additions and 55 deletions.
diff --git a/...egration-test/src/test/java/com/tngtech/archunit/integration/ExamplesIntegrationTest.java b/...egration-test/src/test/java/com/tngtech/archunit/integration/ExamplesIntegrationTest.java
@@ -106,6 +106,10 @@
 import com.tngtech.archunit.testutils.ExpectedMethod;
 import com.tngtech.archunit.testutils.ExpectedTestFailures;
 import com.tngtech.archunit.testutils.MessageAssertionChain;
+import com.tngtech.archunit.testutils.ResultStoringExtension;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeAll;
 import org.junit.jupiter.api.DynamicTest;
 import org.junit.jupiter.api.TestFactory;
 
@@ -139,6 +143,21 @@
 
 class ExamplesIntegrationTest {
 
+    @BeforeAll
+    static void initExtension() {
+        ResultStoringExtension.enable();
+    }
+
+    @AfterEach
+    void tearDown() {
+        ResultStoringExtension.reset();
+    }
+
+    @AfterAll
+    static void disableExtension() {
+        ResultStoringExtension.disable();
+    }
+
     @TestFactory
     Stream<DynamicTest> CodingRulesTest() {
         return ExpectedTestFailures

diff --git a/...gration-test/src/test/java/com/tngtech/archunit/integration/ExtensionIntegrationTest.java b/...gration-test/src/test/java/com/tngtech/archunit/integration/ExtensionIntegrationTest.java
@@ -11,54 +11,58 @@
 import com.tngtech.archunit.exampletest.extension.ExampleExtension;
 import com.tngtech.archunit.lang.ArchRule;
 import org.assertj.core.api.Condition;
-import org.junit.Test;
+import org.junit.jupiter.api.AfterEach;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
 
 import static com.google.common.collect.Iterables.getOnlyElement;
 import static com.tngtech.archunit.lang.syntax.ArchRuleDefinition.noClasses;
 import static org.assertj.core.api.Assertions.assertThat;
 
-public class ExtensionIntegrationTest {
+class ExtensionIntegrationTest {
     private final JavaClasses classes = new ClassFileImporter().importPackagesOf(ClassViolatingCodingRules.class);
 
-    @Test
-    public void evaluation_results_are_dispatched_to_extensions() {
+    @BeforeEach
+    void setUp() {
         ExampleExtension.reset();
-        try {
-            ArchConfiguration.get().configureExtension(ExampleExtension.UNIQUE_IDENTIFIER)
-                    .setProperty("enabled", true);
+    }
+
+    @AfterEach
+    void tearDown() {
+        ExampleExtension.reset();
+        ArchConfiguration.get().configureExtension(ExampleExtension.UNIQUE_IDENTIFIER)
+                .setProperty("enabled", false);
+    }
 
-            ArchRule rule = noClasses().should().haveFullyQualifiedName(ClassViolatingCodingRules.class.getName());
-            checkRuleAndIgnoreFailure(classes, rule);
+    @Test
+    void evaluation_results_are_dispatched_to_extensions() {
+        ArchConfiguration.get().configureExtension(ExampleExtension.UNIQUE_IDENTIFIER)
+                .setProperty("enabled", true);
 
-            assertThat(ExampleExtension.getConfigurationEvents()).hasSize(1);
-            assertThat(ExampleExtension.getConfigurationEvents())
-                    .extracting("properties").are(containingEntry("example-prop", "exampleValue"));
+        ArchRule rule = noClasses().should().haveFullyQualifiedName(ClassViolatingCodingRules.class.getName());
+        checkRuleAndIgnoreFailure(classes, rule);
 
-            EvaluatedRuleEvent event = getOnlyElement(ExampleExtension.getEvaluatedRuleEvents());
-            assertThat(event.contains(rule)).as("Rule was passed").isTrue();
-            assertThat(event.contains(classes)).as("Classes were passed").isTrue();
-            assertThat(event.hasViolationFor(ClassViolatingCodingRules.class))
-                    .as("Has violation for " + ClassViolatingCodingRules.class.getSimpleName()).isTrue();
-        } finally {
-            ArchConfiguration.get().configureExtension(ExampleExtension.UNIQUE_IDENTIFIER).setProperty("enabled", false);
-        }
+        assertThat(ExampleExtension.getConfigurationEvents()).hasSize(1);
+        assertThat(ExampleExtension.getConfigurationEvents())
+                .extracting("properties").are(containingEntry("example-prop", "exampleValue"));
+
+        EvaluatedRuleEvent event = getOnlyElement(ExampleExtension.getEvaluatedRuleEvents());
+        assertThat(event.contains(rule)).as("Rule was passed").isTrue();
+        assertThat(event.contains(classes)).as("Classes were passed").isTrue();
+        assertThat(event.hasViolationFor(ClassViolatingCodingRules.class))
+                .as("Has violation for " + ClassViolatingCodingRules.class.getSimpleName()).isTrue();
     }
 
     @Test
-    public void evaluation_results_are_only_dispatched_to_enabled_extensions() {
-        ExampleExtension.reset();
-        try {
-            ArchConfiguration.get().configureExtension(ExampleExtension.UNIQUE_IDENTIFIER)
-                    .setProperty("enabled", false);
+    void evaluation_results_are_only_dispatched_to_enabled_extensions() {
+        ArchConfiguration.get().configureExtension(ExampleExtension.UNIQUE_IDENTIFIER)
+                .setProperty("enabled", false);
 
-            ArchRule rule = noClasses().should().haveFullyQualifiedName(ClassViolatingCodingRules.class.getName());
-            checkRuleAndIgnoreFailure(classes, rule);
+        ArchRule rule = noClasses().should().haveFullyQualifiedName(ClassViolatingCodingRules.class.getName());
+        checkRuleAndIgnoreFailure(classes, rule);
 
-            assertThat(ExampleExtension.getConfigurationEvents()).isEmpty();
-            assertThat(ExampleExtension.getEvaluatedRuleEvents()).isEmpty();
-        } finally {
-            ArchConfiguration.get().configureExtension(ExampleExtension.UNIQUE_IDENTIFIER).setProperty("enabled", false);
-        }
+        assertThat(ExampleExtension.getConfigurationEvents()).isEmpty();
+        assertThat(ExampleExtension.getEvaluatedRuleEvents()).isEmpty();
     }
 
     private static Condition<Object> containingEntry(final String propKey, final String propValue) {

diff --git a/...t-integration-test/src/test/java/com/tngtech/archunit/testutils/ExpectedTestFailures.java b/...t-integration-test/src/test/java/com/tngtech/archunit/testutils/ExpectedTestFailures.java
@@ -14,7 +14,7 @@
 
 import com.google.common.base.Joiner;
 import com.google.common.collect.ImmutableList;
-import com.tngtech.archunit.lang.ArchRule;
+import com.tngtech.archunit.lang.EvaluationResult;
 import org.junit.jupiter.api.DynamicTest;
 import org.junit.platform.runner.JUnitPlatform;
 import org.junit.runner.JUnitCore;
@@ -259,8 +259,8 @@ private TestFailure(Failure junitFailure, Throwable error) {
             this.error = error;
         }
 
-        ArchRule.AssertionError getAssertionError() {
-            return (ArchRule.AssertionError) error;
+        AssertionError getAssertionError() {
+            return (AssertionError) error;
         }
 
         @Override
@@ -281,13 +281,13 @@ private TestFailures(Iterable<TestFailure> failures) {
 
         void assertNoUnexpectedErrorType() {
             List<TestFailure> unexpectedFailureTypes = failures.stream()
-                    .filter(f -> !(f.error instanceof ArchRule.AssertionError))
+                    .filter(f -> !(f.error instanceof AssertionError))
                     .collect(toList());
 
             if (!unexpectedFailureTypes.isEmpty()) {
                 throw new AssertionError(String.format(
                         "Some failures were due to an unexpected error (i.e. not %s): %s",
-                        ArchRule.AssertionError.class.getName(),
+                        AssertionError.class.getName(),
                         unexpectedFailureTypes));
             }
         }
@@ -358,9 +358,10 @@ ExpectedViolationToAssign copy() {
             return new ExpectedViolationToAssign(failurePredicate, expectedViolation.copy(), handlingAssertion.copy());
         }
 
-        Result evaluate(ArchRule.AssertionError error) {
+        Result evaluate(AssertionError error) {
             ViolationComparisonResult violationResult = expectedViolation.evaluate(error);
-            HandlingAssertion.Result handlingResult = handlingAssertion.evaluate(error.getResult());
+            EvaluationResult evaluationResult = ResultStoringExtension.getEvaluationResultFor(error.getMessage());
+            HandlingAssertion.Result handlingResult = handlingAssertion.evaluate(evaluationResult);
             return new Result(violationResult, handlingResult);
         }
 

diff --git a/...integration-test/src/test/java/com/tngtech/archunit/testutils/ResultStoringExtension.java b/...integration-test/src/test/java/com/tngtech/archunit/testutils/ResultStoringExtension.java
@@ -0,0 +1,47 @@
+package com.tngtech.archunit.testutils;
+
+import java.util.Properties;
+import java.util.concurrent.ConcurrentHashMap;
+
+import com.tngtech.archunit.ArchConfiguration;
+import com.tngtech.archunit.lang.EvaluationResult;
+import com.tngtech.archunit.lang.extension.ArchUnitExtension;
+import com.tngtech.archunit.lang.extension.EvaluatedRule;
+
+import static com.google.common.base.Preconditions.checkNotNull;
+
+public class ResultStoringExtension implements ArchUnitExtension {
+    private static final String UNIQUE_IDENTIFIER = "archunit-result-storing-extension";
+
+    private static final ConcurrentHashMap<String, EvaluationResult> storedResults = new ConcurrentHashMap<>();
+
+    @Override
+    public String getUniqueIdentifier() {
+        return UNIQUE_IDENTIFIER;
+    }
+
+    @Override
+    public void configure(Properties properties) {
+    }
+
+    @Override
+    public void handle(EvaluatedRule evaluatedRule) {
+        storedResults.put(evaluatedRule.getResult().getFailureReport().toString(), evaluatedRule.getResult());
+    }
+
+    public static void reset() {
+        storedResults.clear();
+    }
+
+    static EvaluationResult getEvaluationResultFor(String errorMessage) {
+        return checkNotNull(storedResults.get(errorMessage), "No result was recorded for error message: %s", errorMessage);
+    }
+
+    public static void enable() {
+        ArchConfiguration.get().configureExtension(UNIQUE_IDENTIFIER).setProperty("enabled", true);
+    }
+
+    public static void disable() {
+        ArchConfiguration.get().configureExtension(UNIQUE_IDENTIFIER).setProperty("enabled", false);
+    }
+}
diff --git a/...rc/test/resources/META-INF/services/com.tngtech.archunit.lang.extension.ArchUnitExtension b/...rc/test/resources/META-INF/services/com.tngtech.archunit.lang.extension.ArchUnitExtension
@@ -0,0 +1 @@
+com.tngtech.archunit.testutils.ResultStoringExtension
diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java b/archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java
@@ -89,8 +89,7 @@ public static void assertNoViolation(EvaluationResult result) {
             Set<Pattern> patterns = readPatternsFrom(ARCHUNIT_IGNORE_PATTERNS_FILE_NAME);
             report = report.filter(notMatchedByAny(patterns));
             if (!report.isEmpty()) {
-                String message = report.toString();
-                throw new AssertionError(message, result);
+                throw new AssertionError(report.toString());
             }
         }
 
@@ -261,18 +260,4 @@ public String toString() {
             }
         }
     }
-
-    final class AssertionError extends java.lang.AssertionError {
-        private final EvaluationResult result;
-
-        private AssertionError(String message, EvaluationResult result) {
-            super(message);
-            this.result = result;
-        }
-
-        @PublicAPI(usage = ACCESS)
-        public EvaluationResult getResult() {
-            return result;
-        }
-    }
 }