Skip to content

Commit

Permalink
[JENKINS-61392] Add an include filter for new issues.
Browse files Browse the repository at this point in the history 
To filter issues by files that are actually changed in a pull request
the issue difference needs to filter new issues that are part of a
source control delta.
  • Loading branch information
@uhafner
uhafner committed Jan 18, 2024
1 parent 68a8d40 commit 81ab1a6
Show file tree
Hide file tree
Showing 6 changed files with 207 additions and 49 deletions.
22 changes: 22 additions & 0 deletions src/main/java/edu/hm/hafner/analysis/Issue.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -643,6 +643,28 @@ public Iterable<? extends LineRange> getLineRanges() {
return new LineRangeList(lineRanges);
}

/**
* Returns whether this issue line ranges contain the specified line. If this issue has no lines defined, then this
* method will return {@code true}.
*
* @param line the line to check
* @return {@code true} if the specified line is within the line ranges of this issue, {@code false} otherwise
*/
public boolean affectsLine(final int line) {
if (lineStart == 0 || line == 0) {
return true; // the whole file is marked, so every line is affected
}
if (lineStart <= line && line <= lineEnd) {
return true; // the line is within the primary range of this issue
}
for (LineRange lineRange : lineRanges) {
if (lineRange.contains(line)) {
return true; // the line is within an additional line range of this issue
}
}
return getLineStart() == 0; // an issue without a line number is always relevant
}

/**
* Returns the first column of this issue (columns start at 1, 0 indicates the whole line).
*
Expand Down
48 changes: 44 additions & 4 deletions src/main/java/edu/hm/hafner/analysis/IssueDifference.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,10 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
import java.util.Optional;
import java.util.UUID;
import java.util.stream.Collectors;

import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;

Expand Down Expand Up @@ -34,7 +36,25 @@ public class IssueDifference {
* @param referenceIssues
* the issues of a previous report (reference)
*/
public IssueDifference(final Report currentIssues, final String referenceId, final Report referenceIssues) {
public IssueDifference(final Report currentIssues, final String referenceId,
final Report referenceIssues) {
this(currentIssues, referenceId, referenceIssues, Map.of());
}

/**
* Creates a new instance of {@link IssueDifference}.
*
* @param currentIssues
* the issues of the current report
* @param referenceId
* ID identifying the reference report
* @param referenceIssues
* the issues of a previous report (reference)
* @param includes
* lines in files mapping to include as new issues, all other issues are considered as outstanding
*/
public IssueDifference(final Report currentIssues, final String referenceId,
final Report referenceIssues, final Map<String, Integer> includes) {
newIssues = currentIssues.copy();
fixedIssues = referenceIssues.copy();
outstandingIssues = new Report();
Expand All @@ -52,9 +72,29 @@ public IssueDifference(final Report currentIssues, final String referenceId, fin
removed.forEach(secondPass::remove);
matchIssuesByFingerprint(secondPass);

if (!includes.isEmpty()) {
keepOnlyIncludedIssues(includes);
}
newIssues.forEach(issue -> issue.setReference(referenceId));
}

private void keepOnlyIncludedIssues(final Map<String, Integer> includes) {
var idsToRemove = newIssues.stream().map(Issue::getId).collect(Collectors.toSet());
for (Entry<String, Integer> include : includes.entrySet()) {
newIssues.filter(issue -> filter(issue, include.getKey(), include.getValue()))
.stream().map(Issue::getId)
.forEach(idsToRemove::remove);
}
idsToRemove.stream().map(newIssues::remove).forEach(outstandingIssues::add);
}

private boolean filter(final Issue issue, final String fileName, final int line) {
if (!issue.getFileName().endsWith(fileName)) {
return false;
}
return issue.affectsLine(line);
}

private List<UUID> matchIssuesByEquals(final Report currentIssues) {
List<UUID> removedIds = new ArrayList<>();
for (Issue current : currentIssues) {
Expand Down Expand Up @@ -121,7 +161,7 @@ private List<Issue> findReferenceByEquals(final Issue current) {
}

/**
* Returns the outstanding issues. I.e. all issues, that are part of the previous report and that are still part of
* Returns the outstanding issues. I.e., all issues that are part of the previous report and that are still part of
* the current report.
*
* @return the outstanding issues
Expand All @@ -132,7 +172,7 @@ public Report getOutstandingIssues() {
}

/**
* Returns the new issues. I.e. all issues, that are part of the current report but that have not been shown up in
* Returns the new issues. I.e., all issues that are part of the current report but that have not been shown up in
* the previous report.
*
* @return the new issues
Expand All @@ -143,7 +183,7 @@ public Report getNewIssues() {
}

/**
* Returns the fixed issues. I.e. all issues, that are part of the previous report but that are not present in the
* 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.
*
* @return the fixed issues
Expand Down
12 changes: 11 additions & 1 deletion src/main/java/edu/hm/hafner/analysis/LineRange.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
*
* @author Ullrich Hafner
*/
public class LineRange implements Serializable {
public final class LineRange implements Serializable {
private static final long serialVersionUID = -4124143085672930110L;

private final int start;
Expand Down Expand Up @@ -66,6 +66,16 @@ public int getEnd() {
return end;
}

/**
* Returns whether the specified line is contained in this range.
*
* @param line the line to check
* @return {@code true} if the line is contained in this range, {@code false} otherwise
*/
public boolean contains(final int line) {
return line >= start && line <= end;
}

@Override
public boolean equals(@CheckForNull final Object obj) {
if (this == obj) {
Expand Down
122 changes: 84 additions & 38 deletions src/test/java/edu/hm/hafner/analysis/IssueDifferenceTest.java
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package edu.hm.hafner.analysis;

import java.nio.charset.StandardCharsets;
import java.util.Map;

import org.junit.jupiter.api.Test;

Expand All @@ -19,11 +20,75 @@ class IssueDifferenceTest extends ResourceTest {
private static final String REFERENCE_BUILD = "100";
private static final String CURRENT_BUILD = "2";

/**
* Verifies that issue difference report is created correctly.
*/
@Test
void shouldCreateIssueDifference() {
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");
assertThatReportContainsExactly(everything.getNewIssues(),
"NEW 1", "NEW 2", "NEW 3");
assertThatReportContainsExactly(everything.getOutstandingIssues(),
"OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1");

IssueDifference included = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues,
Map.of("/include/me", 0));
assertThatReportContainsExactly(included.getFixedIssues(),
"TO FIX 1", "TO FIX 2");
assertThatReportContainsExactly(included.getNewIssues(),
"NEW 1", "NEW 3");
assertThatReportContainsExactly(included.getOutstandingIssues(),
"OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1", "NEW 2");

IssueDifference sameSuffixForAll = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues,
Map.of("me", 0));
assertThatReportContainsExactly(sameSuffixForAll.getFixedIssues(),
"TO FIX 1", "TO FIX 2");
assertThatReportContainsExactly(sameSuffixForAll.getNewIssues(),
"NEW 1", "NEW 2", "NEW 3");
assertThatReportContainsExactly(sameSuffixForAll.getOutstandingIssues(),
"OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1");

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");
assertThatReportContainsExactly(allFiles.getNewIssues(),
"NEW 1", "NEW 2", "NEW 3");
assertThatReportContainsExactly(allFiles.getOutstandingIssues(),
"OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1");

IssueDifference nothingNew = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues,
Map.of("other", 0));
assertThatReportContainsExactly(nothingNew.getFixedIssues(),
"TO FIX 1", "TO FIX 2");
assertThatReportContainsExactly(nothingNew.getNewIssues());
assertThatReportContainsExactly(nothingNew.getOutstandingIssues(),
"OUTSTANDING 2", "OUTSTANDING 3", "UPD OUTSTANDING 1", "NEW 1", "NEW 2", "NEW 3");
}

private void assertThatReportContainsExactly(final Report report, final String... messages) {
assertThat(report.get())
.extracting(Issue::getMessage)
.containsExactlyInAnyOrder(messages);
}

@Test
void shouldCreateIssueDifferenceBasedOnPropertiesAndThenOnFingerprint() {
Report referenceIssues = new Report().addAll(
createIssue("OUTSTANDING 1", "OUT 1"),
createIssue("OUTSTANDING 2", "OUT 2"),
Expand All @@ -37,7 +102,7 @@ void shouldCreateIssueDifference() {
createIssue("OUTSTANDING 3", "OUT 3"),
createIssue("NEW 1", "NEW 1"));

IssueDifference issueDifference = createDifference(referenceIssues, currentIssues);
IssueDifference issueDifference = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues);

Report outstanding = issueDifference.getOutstandingIssues();
assertThat(outstanding).hasSize(3);
Expand All @@ -54,14 +119,8 @@ void shouldCreateIssueDifference() {
assertThat(issueDifference.getNewIssues().get(0)).hasMessage("NEW 1").hasReference("2");
}

private IssueDifference createDifference(final Report referenceIssues, final Report currentIssues) {
Report aggregation = new Report("aggregation", "Aggregation");
aggregation.addAll(currentIssues);
return new IssueDifference(aggregation, CURRENT_BUILD, referenceIssues);
}

/**
* Verifies that issue difference report has only outstanding issues when current report and reference report have
* Verifies that issue difference report has only outstanding issues when the current report and reference report have
* same issues.
*/
@Test
Expand All @@ -76,7 +135,7 @@ private void shouldFindOutstandingFromEqualsOrFingerprint(
Report referenceIssues = new Report().add(createIssue("OLD", "OLD"));
Report currentIssues = new Report().add(createIssue(currentMessage, currentFingerprint));

IssueDifference issueDifference = createDifference(referenceIssues, currentIssues);
IssueDifference issueDifference = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues);

assertThat(issueDifference.getFixedIssues()).isEmpty();
assertThat(issueDifference.getNewIssues()).isEmpty();
Expand All @@ -89,16 +148,13 @@ private void shouldFindOutstandingFromEqualsOrFingerprint(
.hasReference(REFERENCE_BUILD);
}

/**
* Verifies that issue difference report has only fixed issues when current report is empty.
*/
@Test
void shouldCreateIssueDifferenceWithEmptyCurrent() {
Report referenceIssues = new Report().addAll(createIssue("OLD 1", "FA"),
createIssue("OLD 2", "FB"));
Report currentIssues = new Report();

IssueDifference issueDifference = createDifference(referenceIssues, currentIssues);
IssueDifference issueDifference = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues);

assertThat(issueDifference.getNewIssues()).isEmpty();
assertThat(issueDifference.getOutstandingIssues()).isEmpty();
Expand All @@ -110,16 +166,13 @@ void shouldCreateIssueDifferenceWithEmptyCurrent() {
assertThat(fixed.get(1)).hasMessage("OLD 2").hasReference(REFERENCE_BUILD);
}

/**
* Verifies that issue difference report has only new issues when reference report is empty.
*/
@Test
void shouldCreateIssueDifferenceWithEmptyReference() {
Report referenceIssues = new Report();
Report currentIssues = new Report().addAll(createIssue("NEW 1", "FA"),
createIssue("NEW 2", "FB"));

IssueDifference issueDifference = createDifference(referenceIssues, currentIssues);
IssueDifference issueDifference = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues);

assertThat(issueDifference.getFixedIssues()).isEmpty();
assertThat(issueDifference.getOutstandingIssues()).isEmpty();
Expand All @@ -130,21 +183,15 @@ void shouldCreateIssueDifferenceWithEmptyReference() {
assertThat(newIssues.get(1)).hasMessage("NEW 2").hasReference(CURRENT_BUILD);
}

/**
* Verifies that if two issues have the same fingerprint then equals is used to select the matching issue in the
* reference build.
*
* @see <a href="https://issues.jenkins-ci.org/browse/JENKINS-56324">Issue 56324</a>
*/
@Test
@Test @org.junitpioneer.jupiter.Issue("JENKINS-56324")
void shouldAlsoUseFingerprintIfIssuesAreEqual() {
Report referenceIssues = new Report().addAll(
createIssue("OLD 1", "FP"));
Report currentIssues = new Report().addAll(
createIssue("NEW 1", "FP"),
createIssue("OLD 1", "FP"));

IssueDifference issueDifference = createDifference(referenceIssues, currentIssues);
IssueDifference issueDifference = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues);

assertThat(issueDifference.getFixedIssues()).isEmpty();

Expand All @@ -166,16 +213,20 @@ void shouldRemoveForSecondPass() {
createIssue("NEW 1", "FP1"),
createIssue("NEW 3", "FP2"));

IssueDifference issueDifference = createDifference(referenceIssues, currentIssues);
IssueDifference issueDifference = new IssueDifference(currentIssues, CURRENT_BUILD, referenceIssues);

assertThat(issueDifference.getFixedIssues()).hasSize(1);
assertThat(issueDifference.getNewIssues()).hasSize(1);
assertThat(issueDifference.getOutstandingIssues()).hasSize(1);
}

private Issue createIssue(final String message, final String fingerprint) {
return createIssue(message, fingerprint, "file-name");
}

private Issue createIssue(final String message, final String fingerprint, final String fileName) {
try (IssueBuilder builder = new IssueBuilder()) {
builder.setFileName("file-name")
builder.setFileName(fileName)
.setLineStart(1)
.setLineEnd(2)
.setColumnStart(3)
Expand All @@ -195,12 +246,7 @@ private Issue createIssue(final String message, final String fingerprint) {
}
}

/**
* Verifies that an aggregation of duplicate issues will be retained in the outstanding issues property.
*
* @see <a href="https://issues.jenkins-ci.org/browse/JENKINS-65482">Issue 65482</a>
*/
@Test
@Test @org.junitpioneer.jupiter.Issue("JENKINS-65482")
void shouldHandleAggregatedResults() {
Report firstAxis = readSpotBugsWarnings();
assertThat(firstAxis).hasSize(2);
Expand All @@ -215,7 +261,7 @@ void shouldHandleAggregatedResults() {
Report reference = new Report();
reference.addAll(firstAxis, secondAxis);

IssueDifference issueDifference = createDifference(reference, aggregation);
IssueDifference issueDifference = new IssueDifference(aggregation, CURRENT_BUILD, reference);
assertThat(issueDifference).hasNoFixedIssues().hasNoNewIssues();
assertThat(issueDifference.getOutstandingIssues()).hasSize(2);
}
Expand Down
Loading

0 comments on commit 81ab1a6

Please sign in to comment.