diff --git a/doc/dependency-graph.puml b/doc/dependency-graph.puml index 5dd8085bd..baf944084 100644 --- a/doc/dependency-graph.puml +++ b/doc/dependency-graph.puml @@ -6,7 +6,7 @@ skinparam rectangle { BackgroundColor<> lightBlue BackgroundColor<> lightGray } -rectangle "analysis-model\n\n11.16.0" as edu_hm_hafner_analysis_model_jar +rectangle "analysis-model\n\n11.18.0-SNAPSHOT" as edu_hm_hafner_analysis_model_jar rectangle "jsoup\n\n1.17.2" as org_jsoup_jsoup_jar rectangle "commons-io\n\n2.15.1" as commons_io_commons_io_jar rectangle "commons-digester3\n\n3.2" as org_apache_commons_commons_digester3_jar diff --git a/src/main/java/edu/hm/hafner/analysis/Issue.java b/src/main/java/edu/hm/hafner/analysis/Issue.java index b1ce3273c..2ce2cb772 100644 --- a/src/main/java/edu/hm/hafner/analysis/Issue.java +++ b/src/main/java/edu/hm/hafner/analysis/Issue.java @@ -187,6 +187,7 @@ public static Predicate byType(final String type) { private String description; // fixed private String fingerprint; // mutable, not part of equals + private boolean partOfModifiedCode; // mutable /** * Creates a new instance of {@link Issue} using the properties of the other issue instance. The new issue has the @@ -734,7 +735,7 @@ public String getModuleName() { } /** - * Sets the the name of the module or project (or similar concept) that contains this issue. + * Sets the name of the module or project (or similar concept) that contains this issue. * * @param moduleName * the module name to set @@ -781,7 +782,7 @@ public String getOriginName() { * @param origin * the origin */ - public void setOrigin(final String origin) { + void setOrigin(final String origin) { Ensure.that(origin).isNotBlank("Issue origin ID '%s' must be not blank (%s)", id, toString()); this.origin = origin.intern(); @@ -795,7 +796,7 @@ public void setOrigin(final String origin) { * @param name * the name of the origin */ - public void setOrigin(final String originId, final String name) { + void setOrigin(final String originId, final String name) { setOrigin(originId); Ensure.that(name).isNotBlank("Issue origin name '%s' must be not blank (%s)", name, toString()); @@ -820,13 +821,13 @@ public String getReference() { * @param reference * the reference */ - public void setReference(@CheckForNull final String reference) { + void setReference(@CheckForNull final String reference) { this.reference = stripToEmpty(reference); } /** * Returns the fingerprint for this issue. Used to decide if two issues are equal even if the equals method returns - * {@code false} since some of the properties differ due to code refactorings. The fingerprint is created by + * {@code false} since some properties differ due to code refactorings. The fingerprint is created by * analyzing the content of the affected file. *

* Note: the fingerprint is not part of the equals method since the fingerprint might change due to an unrelated @@ -860,6 +861,23 @@ public boolean hasFingerprint() { return !UNDEFINED.equals(fingerprint); } + /** + * Returns whether this issue affects a code line that has been modified recently. + * + * @return {@code true} if this issue affects a code line that has been modified recently. + * @see IssuesInModifiedCodeMarker + */ + public boolean isPartOfModifiedCode() { + return partOfModifiedCode; + } + + /** + * Marks the issue as part of a source control diff. + */ + void markAsPartOfModifiedCode() { + partOfModifiedCode = true; + } + /** * Returns additional properties for this issue. A static analysis tool may store additional properties in this * untyped object. This object will be serialized and is used in {@code equals} and {@code hashCode}. @@ -883,6 +901,9 @@ public boolean equals(@CheckForNull final Object o) { Issue issue = (Issue) o; + if (partOfModifiedCode != issue.partOfModifiedCode) { + return false; + } if (lineStart != issue.lineStart) { return false; } @@ -950,11 +971,12 @@ public int hashCode() { result = 31 * result + moduleName.hashCode(); result = 31 * result + packageName.hashCode(); result = 31 * result + fileName.hashCode(); + result = 31 * result + (partOfModifiedCode ? 1 : 0); return result; } @Override public String toString() { - return String.format("%s(%d,%d): %s: %s: %s", getBaseName(), lineStart, columnStart, type, category, message); + return String.format("%s%s(%d,%d): %s: %s: %s", isPartOfModifiedCode() ? "*" : StringUtils.EMPTY, getBaseName(), lineStart, columnStart, type, category, message); } } diff --git a/src/main/java/edu/hm/hafner/analysis/IssueDifference.java b/src/main/java/edu/hm/hafner/analysis/IssueDifference.java index e4011d968..69747f6c8 100644 --- a/src/main/java/edu/hm/hafner/analysis/IssueDifference.java +++ b/src/main/java/edu/hm/hafner/analysis/IssueDifference.java @@ -187,20 +187,6 @@ public Report getNewIssues() { return newIssues; } - /** - * Returns the new issues that are part of the changed code lines. I.e., all issues that are part of the current - * report but that have not been shown up in the previous report. If the difference is computed for a specific - * set of changed files, then this set contains only the new issues that are part of the changes. Otherwise, this - * set will be empty. - * - * @return the new issues - * @see IssueDifference#getNewIssues() - */ - @SuppressFBWarnings("EI") - public Report getNewIssuesInChangedCode() { - return newIssuesInChangedCode; - } - /** * Returns the fixed issues. I.e., all issues that are part of the previous report but that are not present in the * current report anymore. diff --git a/src/main/java/edu/hm/hafner/analysis/IssuesInModifiedCodeMarker.java b/src/main/java/edu/hm/hafner/analysis/IssuesInModifiedCodeMarker.java new file mode 100644 index 000000000..1880f373f --- /dev/null +++ b/src/main/java/edu/hm/hafner/analysis/IssuesInModifiedCodeMarker.java @@ -0,0 +1,43 @@ +package edu.hm.hafner.analysis; + +import java.util.Map; +import java.util.Map.Entry; +import java.util.Set; + +import edu.hm.hafner.util.PathUtil; + +/** + * Computes old, new, and fixed issues based on the reports of two consecutive static analysis runs for the same + * software artifact. + * + * @author Ullrich Hafner + */ +@SuppressWarnings("PMD.DataClass") +public class IssuesInModifiedCodeMarker { + private static final PathUtil PATH_UTIL = new PathUtil(); + + /** + * Finds and marks all issues that are part the changes in a source control diff. + * + * @param report + * the report with the issues to scan + * @param modifiedLinesInFilesMapping + * a mapping modified lines within files + */ + public void markIssuesInModifiedCode(final Report report, final Map> modifiedLinesInFilesMapping) { + for (Entry> include : modifiedLinesInFilesMapping.entrySet()) { + report.filter(issue -> affectsChangedLineInFile(issue, include.getKey(), include.getValue())) + .stream() + .forEach(Issue::markAsPartOfModifiedCode); + } + } + + private boolean affectsChangedLineInFile(final Issue issue, final String fileName, final Set lines) { + var normalizedPath = PATH_UTIL.getRelativePath(fileName); + + if (!issue.getFileName().endsWith(normalizedPath)) { // check only the suffix + return false; + } + return lines.stream().anyMatch(issue::affectsLine); + } +} diff --git a/src/main/java/edu/hm/hafner/analysis/LineRange.java b/src/main/java/edu/hm/hafner/analysis/LineRange.java index 2799f31b0..e75419053 100644 --- a/src/main/java/edu/hm/hafner/analysis/LineRange.java +++ b/src/main/java/edu/hm/hafner/analysis/LineRange.java @@ -1,6 +1,8 @@ package edu.hm.hafner.analysis; import java.io.Serializable; +import java.util.NavigableSet; +import java.util.TreeSet; import edu.umd.cs.findbugs.annotations.CheckForNull; @@ -66,6 +68,19 @@ public int getEnd() { return end; } + /** + * Returns the lines of this line lange in a sorted set. + * + * @return the containing lines, one by one + */ + public NavigableSet getLines() { + var lines = new TreeSet(); + for (int line = getStart(); line <= getEnd(); line++) { + lines.add(line); + } + return lines; + } + /** * Returns whether the specified line is contained in this range. * @@ -76,6 +91,15 @@ public boolean contains(final int line) { return line >= start && line <= end; } + /** + * Returns whether this range is just a single line. + * + * @return {@code true} if this range is just a single line, {@code false} otherwise + */ + public boolean isSingleLine() { + return start == end; + } + @Override public boolean equals(@CheckForNull final Object obj) { if (this == obj) { diff --git a/src/test/java/edu/hm/hafner/analysis/IssueDifferenceTest.java b/src/test/java/edu/hm/hafner/analysis/IssueDifferenceTest.java index 5c2dd34e1..7d4882643 100644 --- a/src/test/java/edu/hm/hafner/analysis/IssueDifferenceTest.java +++ b/src/test/java/edu/hm/hafner/analysis/IssueDifferenceTest.java @@ -1,7 +1,6 @@ package edu.hm.hafner.analysis; import java.nio.charset.StandardCharsets; -import java.util.Map; import org.junit.jupiter.api.Test; @@ -20,97 +19,6 @@ class IssueDifferenceTest extends ResourceTest { private static final String REFERENCE_BUILD = "100"; private static final String CURRENT_BUILD = "2"; - @Test - void shouldFilterByPath() { - Report referenceIssues = new Report().addAll( - createIssue("OUTSTANDING 1", "OUT 1"), - createIssue("OUTSTANDING 2", "OUT 2"), - createIssue("OUTSTANDING 3", "OUT 3"), - createIssue("TO FIX 1", "FIX 1"), - createIssue("TO FIX 2", "FIX 2")); - - Report currentIssues = new Report().addAll( - createIssue("UPD OUTSTANDING 1", "OUT 1"), - createIssue("OUTSTANDING 2", "UPD OUT 2"), - createIssue("OUTSTANDING 3", "OUT 3"), - createIssue("NEW 1", "NEW 1", "/include/me"), - createIssue("NEW 2", "NEW 2", "/remove/me"), - createIssue("NEW 3", "NEW 3", "/include/me")); - - IssueDifference everything = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues); - assertThatReportContainsExactly(everything.getFixedIssues(), "TO FIX 1", "TO FIX 2"); - assertThatReferenceIsSet(everything.getFixedIssues(), REFERENCE_BUILD); - assertThatReportContainsExactly(everything.getNewIssues(), "NEW 1", "NEW 2", "NEW 3"); - assertThatReferenceIsSet(everything.getNewIssues(), CURRENT_BUILD); - assertThatReportContainsExactly(everything.getNewIssuesInChangedCode()); - assertThatReportContainsExactly(everything.getOutstandingIssues(), - "OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1"); - assertThatReferenceIsSet(everything.getOutstandingIssues(), REFERENCE_BUILD); - - IssueDifference included = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues, - Map.of("/include/me", 0)); - assertThatReportContainsExactly(included.getFixedIssues(), "TO FIX 1", "TO FIX 2"); - assertThatReferenceIsSet(included.getFixedIssues(), REFERENCE_BUILD); - assertThatReportContainsExactly(included.getNewIssuesInChangedCode(), "NEW 1", "NEW 3"); - assertThatReferenceIsSet(included.getNewIssuesInChangedCode(), CURRENT_BUILD); - assertThatReportContainsExactly(included.getNewIssues(), "NEW 2"); - assertThatReferenceIsSet(included.getNewIssues(), CURRENT_BUILD); - assertThatReportContainsExactly(included.getOutstandingIssues(), - "OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1"); - assertThatReferenceIsSet(included.getOutstandingIssues(), REFERENCE_BUILD); - - IssueDifference sameSuffixForAll = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues, - Map.of("me", 0)); - assertThatReportContainsExactly(sameSuffixForAll.getFixedIssues(), "TO FIX 1", "TO FIX 2"); - assertThatReferenceIsSet(sameSuffixForAll.getFixedIssues(), REFERENCE_BUILD); - assertThatReportContainsExactly(sameSuffixForAll.getNewIssuesInChangedCode(), - "NEW 1", "NEW 2", "NEW 3"); - assertThatReferenceIsSet(sameSuffixForAll.getNewIssuesInChangedCode(), CURRENT_BUILD); - assertThatReportContainsExactly(sameSuffixForAll.getNewIssues()); - assertThatReportContainsExactly(sameSuffixForAll.getOutstandingIssues(), - "OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1"); - assertThatReferenceIsSet(sameSuffixForAll.getOutstandingIssues(), REFERENCE_BUILD); - - IssueDifference allFiles = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues, - Map.of("/include/me", 0, "/remove/me", 0)); - assertThatReportContainsExactly(allFiles.getFixedIssues(), "TO FIX 1", "TO FIX 2"); - assertThatReferenceIsSet(allFiles.getFixedIssues(), REFERENCE_BUILD); - assertThatReportContainsExactly(allFiles.getNewIssuesInChangedCode(), - "NEW 1", "NEW 2", "NEW 3"); - assertThatReferenceIsSet(allFiles.getNewIssuesInChangedCode(), CURRENT_BUILD); - assertThatReportContainsExactly(allFiles.getNewIssues()); - assertThatReportContainsExactly(allFiles.getOutstandingIssues(), - "OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1"); - assertThatReferenceIsSet(allFiles.getOutstandingIssues(), REFERENCE_BUILD); - - IssueDifference nothingNew = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues, - Map.of("other", 0)); - assertThatReportContainsExactly(nothingNew.getFixedIssues(), - "TO FIX 1", "TO FIX 2"); - assertThatReferenceIsSet(nothingNew.getFixedIssues(), REFERENCE_BUILD); - assertThatReportContainsExactly(nothingNew.getNewIssuesInChangedCode()); - assertThatReportContainsExactly(nothingNew.getNewIssues(), "NEW 1", "NEW 2", "NEW 3"); - assertThatReferenceIsSet(nothingNew.getNewIssues(), CURRENT_BUILD); - assertThatReportContainsExactly(nothingNew.getOutstandingIssues(), - "OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1"); - assertThatReferenceIsSet(nothingNew.getOutstandingIssues(), REFERENCE_BUILD); - } - - private void assertThatReferenceIsSet(final Report report, final String referenceBuild) { - assertThat(report.get()).extracting(Issue::getReference).containsOnly(referenceBuild); - } - - private void assertThatReportContainsExactly(final Report report, final String... messages) { - if (messages.length == 0) { - assertThat(report).isEmpty(); - } - else { - assertThat(report.get()) - .extracting(Issue::getMessage) - .containsExactlyInAnyOrder(messages); - } - } - @Test void shouldCreateIssueDifferenceBasedOnPropertiesAndThenOnFingerprint() { Report referenceIssues = new Report().addAll( diff --git a/src/test/java/edu/hm/hafner/analysis/IssuesInModifiedCodeMarkerTest.java b/src/test/java/edu/hm/hafner/analysis/IssuesInModifiedCodeMarkerTest.java new file mode 100644 index 000000000..c16332202 --- /dev/null +++ b/src/test/java/edu/hm/hafner/analysis/IssuesInModifiedCodeMarkerTest.java @@ -0,0 +1,117 @@ +package edu.hm.hafner.analysis; + +import java.util.List; +import java.util.Map; +import java.util.Set; + +import org.assertj.core.api.AbstractListAssert; +import org.assertj.core.api.ObjectAssert; +import org.junit.jupiter.api.Test; + +import static edu.hm.hafner.analysis.assertions.Assertions.*; + +class IssuesInModifiedCodeMarkerTest { + private static final String TO_STRING_UNMODIFIED = "code.txt(12,0): -: : "; + private static final String TO_STRING_MODIFIED = "*code.txt(12,0): -: : "; + + @Test + void shouldNotMarkAnythingForEmptyMap() { + var report = createReportWithTwoIssues(); + + assertThatModifiedCodeMarkers(report).containsExactly(false, false); + + var marker = new IssuesInModifiedCodeMarker(); + marker.markIssuesInModifiedCode(report, Map.of()); + + assertThatModifiedCodeMarkers(report).containsExactly(false, false); + assertThatIssuesToString(report).containsExactly(TO_STRING_UNMODIFIED, TO_STRING_UNMODIFIED); + } + + @Test + void shouldNotMarkIfFileMatchesButLinesNot() { + var report = createReportWithTwoIssues(); + + var marker = new IssuesInModifiedCodeMarker(); + marker.markIssuesInModifiedCode(report, Map.of("/part/of/modified/code.txt", Set.of(10))); + + assertThatModifiedCodeMarkers(report).containsExactly(false, false); + assertThatIssuesToString(report).containsExactly(TO_STRING_UNMODIFIED, TO_STRING_UNMODIFIED); + } + + @Test + void shouldNotMarkIfLineMatchesButFileNot() { + var report = createReportWithTwoIssues(); + + var marker = new IssuesInModifiedCodeMarker(); + marker.markIssuesInModifiedCode(report, Map.of("/wrong/path/code.txt", Set.of(12, 20))); + + assertThatModifiedCodeMarkers(report).containsExactly(false, false); + assertThatIssuesToString(report).containsExactly(TO_STRING_UNMODIFIED, TO_STRING_UNMODIFIED); + } + + @Test + void shouldMarkIssuesIfOneFileAndLinesMatch() { + var report = createReportWithTwoIssues(); + + var marker = new IssuesInModifiedCodeMarker(); + marker.markIssuesInModifiedCode(report, Map.of("/part/of/modified/code.txt", Set.of(10, 20))); + + assertThatModifiedCodeMarkers(report).containsExactly(true, false); + assertThatIssuesToString(report).containsExactly(TO_STRING_MODIFIED, TO_STRING_UNMODIFIED); + } + + @Test + void shouldMarkIssuesIfAllFilesAndLinesMatch() { + var report = createReportWithTwoIssues(); + + var marker = new IssuesInModifiedCodeMarker(); + marker.markIssuesInModifiedCode(report, Map.of( + "/part/of/modified/code.txt", Set.of(12), + "/part/of/additional/modified/code.txt", Set.of(20))); + + assertThatModifiedCodeMarkers(report).containsExactly(true, true); + assertThatIssuesToString(report).containsExactly(TO_STRING_MODIFIED, TO_STRING_MODIFIED); + } + + @Test + void shouldMarkIssuesIfFilesPrefixMatches() { + var report = createReportWithTwoIssues(); + + var marker = new IssuesInModifiedCodeMarker(); + marker.markIssuesInModifiedCode(report, Map.of("modified/code.txt", Set.of(12, 20))); + + assertThatModifiedCodeMarkers(report).containsExactly(true, true); + assertThatIssuesToString(report).containsExactly(TO_STRING_MODIFIED, TO_STRING_MODIFIED); + } + + @Test + void shouldMarkIssuesIfFileNameUsesWindowsPath() { + var report = createReportWithTwoIssues(); + + var marker = new IssuesInModifiedCodeMarker(); + marker.markIssuesInModifiedCode(report, Map.of("modified\\code.txt", Set.of(12, 20))); + + assertThatModifiedCodeMarkers(report).containsExactly(true, true); + assertThatIssuesToString(report).containsExactly(TO_STRING_MODIFIED, TO_STRING_MODIFIED); + } + + private AbstractListAssert, String, ObjectAssert> assertThatIssuesToString( + final Report report) { + return assertThat(report.get()).extracting(Issue::toString); + } + + private AbstractListAssert, Boolean, ObjectAssert> assertThatModifiedCodeMarkers( + final Report report) { + return assertThat(report.get()).extracting(Issue::isPartOfModifiedCode); + } + + private Report createReportWithTwoIssues() { + var report = new Report(); + try (var builder = new IssueBuilder()) { + builder.setLineStart(12).setLineEnd(20); + report.add(builder.setFileName("/part/of/modified/code.txt").build()); + report.add(builder.setFileName("/part/of/additional/modified/code.txt").build()); + } + return report; + } +} diff --git a/src/test/java/edu/hm/hafner/analysis/LineRangeTest.java b/src/test/java/edu/hm/hafner/analysis/LineRangeTest.java index 845f17de2..0f31d7ca5 100644 --- a/src/test/java/edu/hm/hafner/analysis/LineRangeTest.java +++ b/src/test/java/edu/hm/hafner/analysis/LineRangeTest.java @@ -4,24 +4,36 @@ import nl.jqno.equalsverifier.EqualsVerifier; -import static org.assertj.core.api.Assertions.*; +import static edu.hm.hafner.analysis.assertions.Assertions.*; class LineRangeTest { + @Test + void shouldRefuseIllegalValue() { + assertThat(new LineRange(-5, 1)).hasStart(0).hasEnd(0).isSingleLine(); + } + @Test void shouldFindLinesInsideAndOutsideOfLineRange() { - assertThat(new LineRange(1, 2).contains(0)).isFalse(); - assertThat(new LineRange(1, 2).contains(1)).isTrue(); - assertThat(new LineRange(1, 2).contains(2)).isTrue(); - assertThat(new LineRange(1, 2).contains(3)).isFalse(); - - assertThat(new LineRange(2, 1).contains(0)).isFalse(); - assertThat(new LineRange(2, 1).contains(1)).isTrue(); - assertThat(new LineRange(2, 1).contains(2)).isTrue(); - assertThat(new LineRange(2, 1).contains(3)).isFalse(); - - assertThat(new LineRange(2).contains(1)).isFalse(); - assertThat(new LineRange(2).contains(2)).isTrue(); - assertThat(new LineRange(2).contains(3)).isFalse(); + var lineRange = new LineRange(1, 2); + + assertThat(lineRange.contains(0)).isFalse(); + assertThat(lineRange.contains(1)).isTrue(); + assertThat(lineRange.contains(2)).isTrue(); + assertThat(lineRange.contains(3)).isFalse(); + assertThat(lineRange).hasStart(1).hasEnd(2).hasLines(1, 2).hasToString("[1-2]"); + + var wrongOrder = new LineRange(2, 1); + assertThat(wrongOrder.contains(0)).isFalse(); + assertThat(wrongOrder.contains(1)).isTrue(); + assertThat(wrongOrder.contains(2)).isTrue(); + assertThat(wrongOrder.contains(3)).isFalse(); + assertThat(wrongOrder).hasStart(1).hasEnd(2).hasLines(1, 2).hasToString("[1-2]"); + + var point = new LineRange(2); + assertThat(point.contains(1)).isFalse(); + assertThat(point.contains(2)).isTrue(); + assertThat(point.contains(3)).isFalse(); + assertThat(point).hasStart(2).hasEnd(2).hasLines(2).hasToString("[2-2]"); } @Test