From 503e9390a512e9d06b95f07f951560d95ee3d12e Mon Sep 17 00:00:00 2001 From: Stefan Penndorf Date: Sun, 10 May 2020 16:18:29 +0200 Subject: [PATCH] Allowed more tests to be considered killing a mutant when using historic data (fixes #763) (#765) * made incremental analysis more tolerant of coverage changes fixes #763 Incremental analysis honors all killing tests now instead of limiting the scope of analysis to the first killing test. This is often identical if there is no matrix mutation in place but TestNG plugin calculates test units in terms of test classes thus executes all test methods of a test class and records historic data with method level granularity and class level granularity. We need to take all killing tests into account to match history data and executable test units in the analyzer. This will also help if different test classes cover a certain production class and one of those test classes is deleted or renamed. Improved logging to help spotting analyzer misbehaviour. * remove log catcher after tests Changing the configuration of the Logger is a global setting per JVM. When running tests with surefire the JVM will be reused among tests. Permanently changing the logging configuration might interfere with other tests. --- pitest-entry/pom.xml | 13 + .../org/pitest/coverage/CoverageData.java | 18 +- .../incremental/IncrementalAnalyser.java | 67 ++-- .../incremental/IncrementalAnalyserTest.java | 300 +++++++++++++++--- 4 files changed, 314 insertions(+), 84 deletions(-) diff --git a/pitest-entry/pom.xml b/pitest-entry/pom.xml index d4b7a85f3..bf2260517 100644 --- a/pitest-entry/pom.xml +++ b/pitest-entry/pom.xml @@ -175,6 +175,19 @@ ${testng.version} test + + + org.hamcrest + hamcrest-core + ${hamcrest.version} + test + + + org.hamcrest + hamcrest-library + ${hamcrest.version} + test + diff --git a/pitest-entry/src/main/java/org/pitest/coverage/CoverageData.java b/pitest-entry/src/main/java/org/pitest/coverage/CoverageData.java index f91735caf..a42b56696 100644 --- a/pitest-entry/src/main/java/org/pitest/coverage/CoverageData.java +++ b/pitest-entry/src/main/java/org/pitest/coverage/CoverageData.java @@ -15,6 +15,8 @@ package org.pitest.coverage; +import static java.util.stream.Collectors.toCollection; + import java.math.BigInteger; import java.util.ArrayList; import java.util.Collection; @@ -31,7 +33,6 @@ import java.util.function.Function; import java.util.function.Predicate; import java.util.logging.Logger; -import java.util.stream.Collectors; import java.util.stream.Stream; import org.pitest.classinfo.ClassInfo; @@ -61,7 +62,7 @@ public class CoverageData implements CoverageDatabase { private final List failingTestDescriptions = new ArrayList<>(); public CoverageData(final CodeSource code, final LineMap lm) { - this(code, lm, new LinkedHashMap>()); + this(code, lm, new LinkedHashMap<>()); } @@ -116,13 +117,10 @@ public int getNumberOfCoveredLines(final Collection mutatedClass) { @Override public Collection getTestsForClass(final ClassName clazz) { - final Set tis = new TreeSet<>( - new TestInfoNameComparator()); - tis.addAll(this.instructionCoverage.entrySet().stream().filter(isFor(clazz)) - .flatMap(toTests()) - .collect(Collectors.toList()) - ); - return tis; + return this.instructionCoverage.entrySet().stream() + .filter(isFor(clazz)) + .flatMap(toTests()) + .collect(toCollection(() -> new TreeSet<>(new TestInfoNameComparator()))); } public void calculateClassCoverage(final CoverageResult cr) { @@ -172,7 +170,7 @@ public Collection getClassesForFile(final String sourceFile, final Collection value = this.getClassesForFileCache().get( keyFromSourceAndPackage(sourceFile, packageName)); if (value == null) { - return Collections. emptyList(); + return Collections.emptyList(); } else { return value; } diff --git a/pitest-entry/src/main/java/org/pitest/mutationtest/incremental/IncrementalAnalyser.java b/pitest-entry/src/main/java/org/pitest/mutationtest/incremental/IncrementalAnalyser.java index 7dd152469..872c29a7e 100644 --- a/pitest-entry/src/main/java/org/pitest/mutationtest/incremental/IncrementalAnalyser.java +++ b/pitest-entry/src/main/java/org/pitest/mutationtest/incremental/IncrementalAnalyser.java @@ -28,7 +28,7 @@ public class IncrementalAnalyser implements MutationAnalyser { private final CodeHistory history; private final CoverageDatabase coverage; - private final Map preAnalysed = createStatusMap(); + private final Map preAnalysed = new EnumMap<>(DetectionStatus.class); public IncrementalAnalyser(final CodeHistory history, final CoverageDatabase coverage) { @@ -36,14 +36,6 @@ public IncrementalAnalyser(final CodeHistory history, this.coverage = coverage; } - private static Map createStatusMap() { - final EnumMap map = new EnumMap<>(DetectionStatus.class); - for (final DetectionStatus each : DetectionStatus.values()) { - map.put(each, 0L); - } - return map; - } - @Override public Collection analyse( final Collection mutation) { @@ -67,13 +59,18 @@ public Collection analyse( } private void logTotals() { + int numberOfReducedMutations = 0; for (final Entry each : this.preAnalysed.entrySet()) { - if (each.getValue() != 0) { - LOG.fine("Incremental analysis set " + each.getValue() - + " mutations to a status of " + each.getKey()); + final Long numberOfMutationsInStatus = each.getValue(); + final DetectionStatus mutationStatus = each.getKey(); + LOG.fine("Incremental analysis set " + numberOfMutationsInStatus + + " mutations to a status of " + mutationStatus); + if (mutationStatus != DetectionStatus.NOT_STARTED) { + numberOfReducedMutations += numberOfMutationsInStatus; } } + LOG.info("Incremental analysis reduced number of mutations by " + numberOfReducedMutations ); } private MutationResult analyseFromHistory(final MutationDetails each, @@ -89,10 +86,16 @@ private MutationResult analyseFromHistory(final MutationDetails each, return makeResult(each, DetectionStatus.TIMED_OUT); } - if ((mutationStatusTestPair.getStatus() == DetectionStatus.KILLED) - && killingTestHasNotChanged(each, mutationStatusTestPair)) { - return makeResult(each, DetectionStatus.KILLED, mutationStatusTestPair - .getKillingTests(), mutationStatusTestPair.getSucceedingTests()); + if ((mutationStatusTestPair.getStatus() == DetectionStatus.KILLED)) { + final List killingTestNames = filterUnchangedKillingTests(each, mutationStatusTestPair); + + if (!killingTestNames.isEmpty()) { + return makeResult( + each, + DetectionStatus.KILLED, + killingTestNames, + mutationStatusTestPair.getSucceedingTests()); + } } if ((mutationStatusTestPair.getStatus() == DetectionStatus.SURVIVED) @@ -104,26 +107,23 @@ && killingTestHasNotChanged(each, mutationStatusTestPair)) { return analyseFromScratch(each); } - private boolean killingTestHasNotChanged(final MutationDetails each, - final MutationStatusTestPair mutationStatusTestPair) { - final Collection allTests = this.coverage.getTestsForClass(each - .getClassName()); + private List filterUnchangedKillingTests(final MutationDetails each, + final MutationStatusTestPair mutationStatusTestPair) { - final List testClasses = allTests.stream() - .filter(testIsCalled(mutationStatusTestPair.getKillingTest().get())) - .map(TestInfo.toDefiningClassName()) + return this.coverage.getTestsForClass(each.getClassName()).stream() + .filter(isAKillingTestFor(mutationStatusTestPair)) + .filter(testClassDidNotChange()) + .map(TestInfo::getName) .collect(Collectors.toList()); + } - if (testClasses.isEmpty()) { - return false; - } - - return !this.history.hasClassChanged(testClasses.get(0)); - + private Predicate testClassDidNotChange() { + return a -> !this.history.hasClassChanged(TestInfo.toDefiningClassName().apply(a)); } - private static Predicate testIsCalled(final String testName) { - return a -> a.getName().equals(testName); + private static Predicate isAKillingTestFor(final MutationStatusTestPair mutation) { + final List killingTestNames = mutation.getKillingTests(); + return a -> killingTestNames.contains(a.getName()); } private MutationResult analyseFromScratch(final MutationDetails mutation) { @@ -144,10 +144,7 @@ private MutationResult makeResult(final MutationDetails each, } private void updatePreanalysedTotal(final DetectionStatus status) { - if (status != DetectionStatus.NOT_STARTED) { - final long count = this.preAnalysed.get(status); - this.preAnalysed.put(status, count + 1); - } + this.preAnalysed.merge(status, 1L, Long::sum); } } diff --git a/pitest-entry/src/test/java/org/pitest/mutationtest/incremental/IncrementalAnalyserTest.java b/pitest-entry/src/test/java/org/pitest/mutationtest/incremental/IncrementalAnalyserTest.java index f1f1c3f3e..087e50fa0 100644 --- a/pitest-entry/src/test/java/org/pitest/mutationtest/incremental/IncrementalAnalyserTest.java +++ b/pitest-entry/src/test/java/org/pitest/mutationtest/incremental/IncrementalAnalyserTest.java @@ -1,15 +1,36 @@ package org.pitest.mutationtest.incremental; -import static org.junit.Assert.assertEquals; +import static java.util.Arrays.asList; +import static java.util.Collections.emptyList; +import static java.util.Collections.singletonList; +import static org.hamcrest.Matchers.allOf; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasItems; +import static org.junit.Assert.assertThat; import static org.mockito.Matchers.any; import static org.mockito.Mockito.when; +import static org.pitest.mutationtest.DetectionStatus.KILLED; +import static org.pitest.mutationtest.DetectionStatus.NOT_STARTED; +import static org.pitest.mutationtest.DetectionStatus.SURVIVED; +import static org.pitest.mutationtest.DetectionStatus.TIMED_OUT; import static org.pitest.mutationtest.LocationMother.aLocation; import static org.pitest.mutationtest.LocationMother.aMutationId; import java.math.BigInteger; +import java.util.ArrayList; import java.util.Collection; import java.util.Collections; +import java.util.Optional; +import java.util.logging.Handler; +import java.util.logging.Level; +import java.util.logging.LogRecord; +import java.util.logging.Logger; +import org.hamcrest.Description; +import org.hamcrest.Matcher; +import org.hamcrest.TypeSafeDiagnosingMatcher; +import org.junit.After; import org.junit.Before; import org.junit.Test; import org.mockito.Mock; @@ -17,12 +38,12 @@ import org.pitest.classinfo.ClassName; import org.pitest.coverage.CoverageDatabase; import org.pitest.coverage.TestInfo; -import java.util.Optional; import org.pitest.mutationtest.DetectionStatus; import org.pitest.mutationtest.MutationResult; import org.pitest.mutationtest.MutationStatusTestPair; import org.pitest.mutationtest.engine.MutationDetails; import org.pitest.mutationtest.engine.MutationIdentifier; +import org.pitest.util.Log; public class IncrementalAnalyserTest { @@ -34,23 +55,43 @@ public class IncrementalAnalyserTest { @Mock private CoverageDatabase coverage; + private LogCatcher logCatcher; + + + @Before + public void configureLogCatcher() { + logCatcher = new LogCatcher(); + final Logger logger = Log.getLogger(); + + logger.addHandler(logCatcher); + logger.setLevel(Level.ALL); + } + @Before public void setUp() { MockitoAnnotations.initMocks(this); this.testee = new IncrementalAnalyser(this.history, this.coverage); } + @After + public void removeLogCatcher() { + Log.getLogger().removeHandler(logCatcher); + } + @Test public void shouldStartNewMutationsAtAStatusOfNotStarted() { final MutationDetails md = makeMutation("foo"); when(this.history.getPreviousResult(any(MutationIdentifier.class))) - .thenReturn(Optional. empty()); + .thenReturn(Optional.empty()); - final Collection actual = this.testee.analyse(Collections - .singletonList(md)); + final Collection actual = this.testee.analyse(singletonList(md)); - assertEquals(DetectionStatus.NOT_STARTED, actual.iterator().next() - .getStatus()); + assertThat(actual, hasItem(withStatus(NOT_STARTED))); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of NOT_STARTED", + "Incremental analysis reduced number of mutations by 0" + )); } @Test @@ -60,10 +101,14 @@ public void shouldStartPreviousSurvivedMutationsAtAStatusOfNotStartedWhenCoverag when( this.history.hasCoverageChanged(any(ClassName.class), any(BigInteger.class))).thenReturn(true); - final Collection actual = this.testee.analyse(Collections - .singletonList(md)); - assertEquals(DetectionStatus.NOT_STARTED, actual.iterator().next() - .getStatus()); + final Collection actual = this.testee.analyse(singletonList(md)); + + assertThat(actual, hasItem(withStatus(NOT_STARTED))); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of NOT_STARTED", + "Incremental analysis reduced number of mutations by 0" + )); } @Test @@ -73,9 +118,14 @@ public void shouldStartPreviousSurvivedMutationsAtAStatusOfSurvivedWhenCoverageH when( this.history.hasCoverageChanged(any(ClassName.class), any(BigInteger.class))).thenReturn(false); - final Collection actual = this.testee.analyse(Collections - .singletonList(md)); - assertEquals(DetectionStatus.SURVIVED, actual.iterator().next().getStatus()); + final Collection actual = this.testee.analyse(singletonList(md)); + + assertThat(actual, hasItem(withStatus(SURVIVED))); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of SURVIVED", + "Incremental analysis reduced number of mutations by 1" + )); } @Test @@ -83,11 +133,14 @@ public void shouldStartPreviousTimedOutMutationsAtAStatusOfNotStartedWhenClassHa final MutationDetails md = makeMutation("foo"); setHistoryForAllMutationsTo(DetectionStatus.TIMED_OUT); when(this.history.hasClassChanged(any(ClassName.class))).thenReturn(true); - final Collection actual = this.testee.analyse(Collections - .singletonList(md)); + final Collection actual = this.testee.analyse(singletonList(md)); - assertEquals(DetectionStatus.NOT_STARTED, actual.iterator().next() - .getStatus()); + assertThat(actual, hasItem(withStatus(NOT_STARTED))); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of NOT_STARTED", + "Incremental analysis reduced number of mutations by 0" + )); } @Test @@ -95,49 +148,202 @@ public void shouldStartPreviousTimedOutMutationsAtAStatusOfTimedOutWhenClassHasN final MutationDetails md = makeMutation("foo"); setHistoryForAllMutationsTo(DetectionStatus.TIMED_OUT); when(this.history.hasClassChanged(any(ClassName.class))).thenReturn(false); - final Collection actual = this.testee.analyse(Collections - .singletonList(md)); + final Collection actual = this.testee.analyse(singletonList(md)); - assertEquals(DetectionStatus.TIMED_OUT, actual.iterator().next() - .getStatus()); + assertThat(actual, hasItem(withStatus(TIMED_OUT))); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of TIMED_OUT", + "Incremental analysis reduced number of mutations by 1" + )); } @Test - public void shouldStartPreviousKilledMutationsAtAStatusOfNotStartedWhenNeitherClassOrTestHasChanged() { + public void shouldStartPreviousKilledMutationsAtAStatusOfKilledWhenNeitherClassOrTestHasChanged() { final MutationDetails md = makeMutation("foo"); final String killingTest = "fooTest"; setHistoryForAllMutationsTo(DetectionStatus.KILLED, killingTest); final Collection tests = Collections.singleton(new TestInfo( - "TEST_CLASS", killingTest, 0, Optional. empty(), 0)); + "TEST_CLASS", killingTest, 0, Optional.empty(), 0)); when(this.coverage.getTestsForClass(any(ClassName.class))) .thenReturn(tests); when(this.history.hasClassChanged(any(ClassName.class))).thenReturn(false); - final MutationResult actual = this.testee - .analyse(Collections.singletonList(md)).iterator().next(); + final Collection actual = this.testee + .analyse(singletonList(md)); + + assertThat(actual, hasItem(allOf(withStatus(KILLED), withKillingTest(killingTest)))); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of KILLED", + "Incremental analysis reduced number of mutations by 1" + )); - assertEquals(DetectionStatus.KILLED, actual.getStatus()); - assertEquals(Optional.ofNullable(killingTest), actual.getKillingTest()); } @Test - public void shouldStartPreviousKilledMutationsAtAStatusOfKilledWhenNeitherClassOrTestHasChanged() { + public void shouldStartPreviousKilledMutationsAtAStatusOfKilledWhenNeitherClassHasChangedNorTestHasChangedForAtLeastOneKillingTest() { + final MutationDetails md = makeMutation("foo"); + final String killingTestChanged = "fooTest"; + final String killingTestUnchanged = "killerTest"; + + setHistoryForAllMutationsTo(DetectionStatus.KILLED, killingTestChanged, killingTestUnchanged ); + + final TestInfo testChanged = new TestInfo("TEST_CLASS_CHANGED", killingTestChanged, 0, Optional.empty(), 0); + final TestInfo testUnchanged = new TestInfo("TEST_CLASS_UNCHANGED", killingTestUnchanged, 0, Optional.empty(), 0); + + when(this.coverage.getTestsForClass(ClassName.fromString("clazz"))) + .thenReturn(asList(testChanged,testUnchanged)); + + when(this.history.hasClassChanged(ClassName.fromString("clazz"))) + .thenReturn(false); + + when(this.history.hasClassChanged(ClassName.fromString("TEST_CLASS_CHANGED"))) + .thenReturn(true); + when(this.history.hasClassChanged(ClassName.fromString("TEST_CLASS_UNCHANGED"))) + .thenReturn(false); + + + final Collection actual = this.testee.analyse(singletonList(md)); + + assertThat(actual, hasItem(allOf(withStatus(KILLED), withKillingTest(killingTestUnchanged)))); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of KILLED", + "Incremental analysis reduced number of mutations by 1" + )); + } + + @Test + public void shouldStartPreviousKilledMutationsAtAStatusOfNotStartedWhenTestHasChanged() { final MutationDetails md = makeMutation("foo"); final String killingTest = "fooTest"; setHistoryForAllMutationsTo(DetectionStatus.KILLED, killingTest); final Collection tests = Collections.singleton(new TestInfo( - "TEST_CLASS", killingTest, 0, Optional. empty(), 0)); + "TEST_CLASS", killingTest, 0, Optional.empty(), 0)); when(this.coverage.getTestsForClass(any(ClassName.class))) - .thenReturn(tests); - when(this.history.hasClassChanged(ClassName.fromString("foo"))).thenReturn( - false); + .thenReturn(tests); + when(this.history.hasClassChanged(ClassName.fromString("clazz"))).thenReturn( + false); when(this.history.hasClassChanged(ClassName.fromString("TEST_CLASS"))) - .thenReturn(true); - final MutationResult actual = this.testee - .analyse(Collections.singletonList(md)).iterator().next(); + .thenReturn(true); + + final Collection actual = this.testee.analyse(singletonList(md)); - assertEquals(DetectionStatus.NOT_STARTED, actual.getStatus()); + assertThat(actual, hasItem(withStatus(NOT_STARTED))); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of NOT_STARTED", + "Incremental analysis reduced number of mutations by 0" + )); + } + + @Test + public void assessMultipleMutationsAtATime() { + final MutationDetails md1 = makeMutation("foo"); + final MutationDetails md2 = makeMutation("bar"); + final MutationDetails md3 = makeMutation("baz"); + final MutationDetails md4 = makeMutation("bumm"); + + final String killingTest = "killerTest"; + final TestInfo test = new TestInfo("TEST_CLASS", killingTest, 0, Optional.empty(), 0); + + when(this.history.getPreviousResult(md1.getId())) + .thenReturn(Optional.of( + new MutationStatusTestPair( + 0, + KILLED, + singletonList(killingTest), + emptyList()))); + + when(this.history.getPreviousResult(md2.getId())) + .thenReturn(Optional.of( + new MutationStatusTestPair( + 0, + KILLED, + singletonList(killingTest), + emptyList()))); + + when(this.history.getPreviousResult(md3.getId())) + .thenReturn(Optional.of( + new MutationStatusTestPair(0, SURVIVED, emptyList(), emptyList()))); + + when(this.history.getPreviousResult(md4.getId())) + .thenReturn(Optional.empty()); + + when(this.coverage.getTestsForClass(ClassName.fromString("clazz"))) + .thenReturn(singletonList(test)); + + when(this.history.hasClassChanged(ClassName.fromString("clazz"))) + .thenReturn(false); + when(this.history.hasClassChanged(ClassName.fromString("TEST_CLASS"))) + .thenReturn(false); + + final Collection actual = this.testee.analyse(asList(md1, md2, md3, md4)); + + assertThat(actual, contains( + withStatus(KILLED), + withStatus(KILLED), + withStatus(SURVIVED), + withStatus(NOT_STARTED) + )); + assertThat(logCatcher.logEntries, + hasItems( + "Incremental analysis set 1 mutations to a status of NOT_STARTED", + "Incremental analysis set 2 mutations to a status of KILLED", + "Incremental analysis set 1 mutations to a status of SURVIVED", + "Incremental analysis reduced number of mutations by 3" + )); + } + + private Matcher withStatus(final DetectionStatus status) { + return new TypeSafeDiagnosingMatcher() { + + @Override + public void describeTo(final Description description) { + description.appendText("a mutation result with status ").appendValue(status); + } + + @Override + protected boolean matchesSafely(final MutationResult item, + final Description mismatchDescription) { + mismatchDescription + .appendText("a mutation result with status ") + .appendValue(item.getStatus()); + + return status.equals(item.getStatus()); + } + }; + } + + private Matcher withKillingTest(final String expectedKillingTest) { + return new TypeSafeDiagnosingMatcher() { + @Override + public void describeTo(final Description description) { + description + .appendText("a mutation result with killing test named ") + .appendValue(expectedKillingTest); + } + + @Override + protected boolean matchesSafely(final MutationResult item, + final Description mismatchDescription) { + Optional itemKillingTest = item.getKillingTest(); + if (!itemKillingTest.isPresent()) { + mismatchDescription + .appendText("a mutation result with no killing test"); + return false; + } + + final String killingTestName = itemKillingTest.get(); + mismatchDescription + .appendText("a mutation result with killing test named ") + .appendValue(killingTestName); + + return expectedKillingTest.equals(killingTestName); + } + }; } private MutationDetails makeMutation(final String method) { @@ -151,9 +357,25 @@ private void setHistoryForAllMutationsTo(final DetectionStatus status) { } private void setHistoryForAllMutationsTo(final DetectionStatus status, - final String test) { + final String... test) { when(this.history.getPreviousResult(any(MutationIdentifier.class))) - .thenReturn(Optional.ofNullable(new MutationStatusTestPair(0, status, test))); + .thenReturn(Optional.of( + new MutationStatusTestPair(0, status, asList(test), emptyList()))); } + private static class LogCatcher extends Handler { + + final ArrayList logEntries = new ArrayList<>(); + + @Override + public void publish(final LogRecord record) { + logEntries.add(record.getMessage()); + } + + @Override + public void flush() { } + + @Override + public void close() throws SecurityException { } + } }