Merge pull request #123 from TNG/fix-assertionerror-mem-leak

Fixed memory leak in ArchRule.AssertionError in combination with JUnit 4
TNG · Nov 20, 2018 · 86cae1f · 86cae1f
2 parents 71c2705 + 8e62601
commit 86cae1f
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;
-        }
-    }
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1 @@
		com.tngtech.archunit.testutils.ResultStoringExtension