From 8b859a424d812b37cb06926e3be88328509e254e Mon Sep 17 00:00:00 2001 From: Peter Gafert Date: Tue, 20 Nov 2018 16:31:16 +0100 Subject: [PATCH] Fixed memory leak: JUnit4Core keeps reference to all Failures, Failures 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 --- .../integration/ExamplesIntegrationTest.java | 19 ++++++ .../integration/ExtensionIntegrationTest.java | 68 ++++++++++--------- .../testutils/ExpectedTestFailures.java | 15 ++-- .../testutils/ResultStoringExtension.java | 47 +++++++++++++ ....archunit.lang.extension.ArchUnitExtension | 1 + .../com/tngtech/archunit/lang/ArchRule.java | 17 +---- 6 files changed, 112 insertions(+), 55 deletions(-) create mode 100644 archunit-integration-test/src/test/java/com/tngtech/archunit/testutils/ResultStoringExtension.java create mode 100644 archunit-integration-test/src/test/resources/META-INF/services/com.tngtech.archunit.lang.extension.ArchUnitExtension diff --git a/archunit-integration-test/src/test/java/com/tngtech/archunit/integration/ExamplesIntegrationTest.java b/archunit-integration-test/src/test/java/com/tngtech/archunit/integration/ExamplesIntegrationTest.java index 0a3a5c103d..ceff8bcd5b 100644 --- a/archunit-integration-test/src/test/java/com/tngtech/archunit/integration/ExamplesIntegrationTest.java +++ b/archunit-integration-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 CodingRulesTest() { return ExpectedTestFailures diff --git a/archunit-integration-test/src/test/java/com/tngtech/archunit/integration/ExtensionIntegrationTest.java b/archunit-integration-test/src/test/java/com/tngtech/archunit/integration/ExtensionIntegrationTest.java index 4c71a645da..5956fa1232 100644 --- a/archunit-integration-test/src/test/java/com/tngtech/archunit/integration/ExtensionIntegrationTest.java +++ b/archunit-integration-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 containingEntry(final String propKey, final String propValue) { diff --git a/archunit-integration-test/src/test/java/com/tngtech/archunit/testutils/ExpectedTestFailures.java b/archunit-integration-test/src/test/java/com/tngtech/archunit/testutils/ExpectedTestFailures.java index 7486722d56..6ef05759ad 100644 --- a/archunit-integration-test/src/test/java/com/tngtech/archunit/testutils/ExpectedTestFailures.java +++ b/archunit-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 failures) { void assertNoUnexpectedErrorType() { List 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/archunit-integration-test/src/test/java/com/tngtech/archunit/testutils/ResultStoringExtension.java b/archunit-integration-test/src/test/java/com/tngtech/archunit/testutils/ResultStoringExtension.java new file mode 100644 index 0000000000..ae4a733651 --- /dev/null +++ b/archunit-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 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/archunit-integration-test/src/test/resources/META-INF/services/com.tngtech.archunit.lang.extension.ArchUnitExtension b/archunit-integration-test/src/test/resources/META-INF/services/com.tngtech.archunit.lang.extension.ArchUnitExtension new file mode 100644 index 0000000000..fc462642ea --- /dev/null +++ b/archunit-integration-test/src/test/resources/META-INF/services/com.tngtech.archunit.lang.extension.ArchUnitExtension @@ -0,0 +1 @@ +com.tngtech.archunit.testutils.ResultStoringExtension \ No newline at end of file diff --git a/archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java b/archunit/src/main/java/com/tngtech/archunit/lang/ArchRule.java index 258169c82e..9280ab6cbd 100644 --- 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 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; - } - } }