Skip to content

Commit

Permalink
Allow to filter score and review issues by path
Browse files Browse the repository at this point in the history
  • Loading branch information
reda-alaoui committed Jul 2, 2022
1 parent 7f34ee8 commit b9c9b38
Show file tree
Hide file tree
Showing 10 changed files with 219 additions and 20 deletions.
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -191,6 +191,7 @@ It is possible to filter issues by:
is not created by processing commit and this issue is not supposed to be included to review.
3. Changed lines only - when only several lines are changed in a commit user may not want other lines to be commented by
Gerrit. With "Add comments to changed lines only" unchanged in the commit lines will not be commented in Gerrit.
4. Included paths' glob pattern - The glob pattern that file paths should match to be included by the filter. Leaving a blank value or setting value `**/*` means any file path will match. Tested file paths always start with `/`.

![Filter settings](doc/filter-settings.png)

Expand Down
Binary file modified doc/filter-settings.png
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
package org.jenkinsci.plugins.sonargerrit.sonar;

import com.google.common.base.Predicate;
import java.nio.file.FileSystems;
import java.nio.file.PathMatcher;
import java.nio.file.Paths;
import org.apache.commons.lang3.StringUtils;

/** @author Réda Housni Alaoui */
public class ByGlobPatternPredicate implements Predicate<Issue> {

private final PathMatcher pathMatcher;

public ByGlobPatternPredicate(String pattern) {
this.pathMatcher = FileSystems.getDefault().getPathMatcher("glob:" + pattern);
}

@Override
public boolean apply(Issue input) {
String filepath = StringUtils.prependIfMissing(input.getFilepath(), "/");
return pathMatcher.matches(Paths.get(filepath));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ public Iterable<Issue> filter() {
toBeApplied.add(ByMinSeverityPredicate.apply(severity));
}

String includedPathsGlobPattern = filterConfig.getIncludedPathsGlobPattern();
if (includedPathsGlobPattern != null) {
toBeApplied.add(new ByGlobPatternPredicate(includedPathsGlobPattern));
}

return Iterables.filter(issues, Predicates.and(toBeApplied));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import static org.jenkinsci.plugins.sonargerrit.util.Localization.getLocalized;

import com.google.common.base.MoreObjects;
import edu.umd.cs.findbugs.annotations.Nullable;
import hudson.Extension;
import hudson.model.AbstractDescribableImpl;
import hudson.model.Descriptor;
Expand All @@ -18,6 +19,7 @@ public class IssueFilterConfig extends AbstractDescribableImpl<IssueFilterConfig
private String severity; // todo make enum
private boolean newIssuesOnly;
private boolean changedLinesOnly;
private String includedPathsGlobPattern;

public IssueFilterConfig(String severity, boolean newIssuesOnly, boolean changedLinesOnly) {
setSeverity(severity);
Expand All @@ -35,30 +37,40 @@ public String getSeverity() {
return severity;
}

public boolean isChangedLinesOnly() {
return changedLinesOnly;
}

public boolean isNewIssuesOnly() {
return newIssuesOnly;
}

@DataBoundSetter
public void setSeverity(String severity) {
severity = DataHelper.checkEnumValueCorrect(Severity.class, severity);
this.severity = MoreObjects.firstNonNull(severity, DescriptorImpl.SEVERITY);
}

public boolean isNewIssuesOnly() {
return newIssuesOnly;
}

@DataBoundSetter
public void setNewIssuesOnly(boolean newIssuesOnly) {
this.newIssuesOnly = newIssuesOnly;
}

public boolean isChangedLinesOnly() {
return changedLinesOnly;
}

@DataBoundSetter
public void setChangedLinesOnly(boolean changedLinesOnly) {
this.changedLinesOnly = changedLinesOnly;
}

@Nullable
public String getIncludedPathsGlobPattern() {
return includedPathsGlobPattern;
}

@DataBoundSetter
public void setIncludedPathsGlobPattern(String includedPathsGlobPattern) {
this.includedPathsGlobPattern = includedPathsGlobPattern;
}

@Override
public DescriptorImpl getDescriptor() {
return new DescriptorImpl();
Expand All @@ -82,9 +94,11 @@ public static class DescriptorImpl extends Descriptor<IssueFilterConfig> {
*/
@SuppressWarnings(value = "unused")
public FormValidation doCheckSeverity(@QueryParameter String value) {
if (value == null || Severity.valueOf(value) == null) {
if (value == null) {
return FormValidation.error(
getLocalized("jenkins.plugin.error.review.filter.severity.unknown"));
} else {
Severity.valueOf(value);
}
return FormValidation.ok();
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,11 @@
description="${%jenkins.plugin.settings.gerrit.filter.lines.changed.description}">
<f:checkbox selected="${changedLinesOnly}" default="descriptor.CHANGED_LINES_ONLY"/>
</f:entry>
</j:jelly>

<f:entry
title="${%jenkins.plugin.settings.gerrit.filter.included-paths-glob-pattern}"
field="includedPathsGlobPattern"
description="${%jenkins.plugin.settings.gerrit.filter.included-paths-glob-pattern.description}">
<f:textbox/>
</f:entry>
</j:jelly>
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ jenkins.plugin.settings.gerrit.filter.new=Report new issues only?
jenkins.plugin.settings.gerrit.filter.new.description=Reports new as well as existing issues when unchecked. Note that pull request analysis only reports new issues and is therefore unaffected by this parameter.
jenkins.plugin.settings.gerrit.filter.lines.changed=Affect changed lines only
jenkins.plugin.settings.gerrit.filter.lines.changed.description=Reports issues for changed as well as not changed lines in files affected by patchset when unchecked
jenkins.plugin.settings.gerrit.filter.included-paths-glob-pattern=Included paths'' glob pattern
jenkins.plugin.settings.gerrit.filter.included-paths-glob-pattern.description=The glob pattern that file paths should match to be included by the filter. Leaving a blank value or setting value ''**/*'' means any file path will match. Tested file paths always start with ''/''.

INFO=Info
MINOR=Minor
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,12 @@
import java.io.IOException;
import java.nio.file.Path;
import java.util.Collections;
import java.util.Optional;
import jenkins.model.Jenkins;
import jenkins.model.ParameterizedJobMixIn;
import me.redaalaoui.gerrit_rest_java_client.thirdparty.com.google.gerrit.extensions.common.ChangeInfo;
import me.redaalaoui.gerrit_rest_java_client.thirdparty.com.google.gerrit.extensions.restapi.RestApiException;
import org.apache.commons.lang3.StringUtils;
import org.eclipse.jgit.api.errors.GitAPIException;
import org.jenkinsci.plugins.sonargerrit.test_infrastructure.cluster.Cluster;
import org.jenkinsci.plugins.sonargerrit.test_infrastructure.cluster.EnableCluster;
Expand All @@ -38,6 +40,8 @@ class ReviewTest {
+ "-Dsonar.pullrequest.key=${env.GERRIT_CHANGE_NUMBER}-${env.GERRIT_PATCHSET_NUMBER} "
+ "-Dsonar.pullrequest.base=${env.GERRIT_BRANCH} "
+ "-Dsonar.pullrequest.branch=${env.GERRIT_REFSPEC}";
private static final String FILEPATH =
"src/main/java/org/example/UselessConstructorDeclaration.java";

private static Cluster cluster;
private static GerritGit git;
Expand Down Expand Up @@ -86,7 +90,7 @@ void beforeEach() throws GitAPIException {
@DisplayName("STANDARD comment type")
void test1() throws Exception {
GerritChange change = createChangeViolatingS1186();
triggerAndAssertSuccess(createPipelineJob(change, ReviewCommentType.STANDARD));
triggerAndAssertSuccess(createPipelineJob(change, ReviewCommentType.STANDARD, null));

ChangeInfo changeDetail = change.getDetail();
assertThat(changeDetail.labels.get(GerritServer.CODE_QUALITY_LABEL).all)
Expand All @@ -103,7 +107,7 @@ void test1() throws Exception {
@DisplayName("ROBOT comment type")
void test2() throws Exception {
GerritChange change = createChangeViolatingS1186();
triggerAndAssertSuccess(createPipelineJob(change, ReviewCommentType.ROBOT));
triggerAndAssertSuccess(createPipelineJob(change, ReviewCommentType.ROBOT, null));

ChangeInfo changeDetail = change.getDetail();
assertThat(changeDetail.labels.get(GerritServer.CODE_QUALITY_LABEL).all)
Expand All @@ -126,7 +130,7 @@ void test2() throws Exception {
@DisplayName("Review tag is autogenerated:sonar")
void test3() throws Exception {
GerritChange change = createChangeViolatingS1186();
triggerAndAssertSuccess(createPipelineJob(change, ReviewCommentType.STANDARD));
triggerAndAssertSuccess(createPipelineJob(change, ReviewCommentType.STANDARD, null));

ChangeInfo changeDetail = change.getDetail();
assertThat(changeDetail.labels.get(GerritServer.CODE_QUALITY_LABEL).all)
Expand All @@ -135,10 +139,42 @@ void test3() throws Exception {
.containsExactly("autogenerated:sonar");
}

@Test
@DisplayName("Issue ignored because of path glob pattern")
void test4() throws Exception {
GerritChange change = createChangeViolatingS1186();
triggerAndAssertSuccess(createPipelineJob(change, ReviewCommentType.ROBOT, "/[!src]/**"));

ChangeInfo changeDetail = change.getDetail();
assertThat(changeDetail.labels.get(GerritServer.CODE_QUALITY_LABEL).all)
.hasSize(1)
.map(approvalInfo -> approvalInfo.value)
.containsExactly(1);

assertThat(change.listRobotComments()).isEmpty();
}

@Test
@DisplayName("Issue considered despite of path glob pattern")
void test5() throws Exception {
GerritChange change = createChangeViolatingS1186();
triggerAndAssertSuccess(createPipelineJob(change, ReviewCommentType.ROBOT, "/src/**"));

ChangeInfo changeDetail = change.getDetail();
assertThat(changeDetail.labels.get(GerritServer.CODE_QUALITY_LABEL).all)
.hasSize(1)
.map(approvalInfo -> approvalInfo.value)
.containsExactly(-1);

assertThat(change.listRobotComments())
.filteredOn(comment -> comment.message.contains("S1186"))
.hasSize(1);
}

private GerritChange createChangeViolatingS1186()
throws GitAPIException, IOException, RestApiException {
git.addAndCommitFile(
"src/main/java/org/example/UselessConstructorDeclaration.java",
FILEPATH,
"package org.example; "
+ "public class UselessConstructorDeclaration { "
+ "public UselessConstructorDeclaration() {} "
Expand All @@ -147,10 +183,15 @@ private GerritChange createChangeViolatingS1186()
}

@SuppressWarnings("rawtypes")
private Job createPipelineJob(GerritChange change, ReviewCommentType commentType)
private Job createPipelineJob(
GerritChange change, ReviewCommentType commentType, String includedPathsGlobPattern)
throws IOException {
WorkflowJob job = cluster.jenkinsRule().createProject(WorkflowJob.class);
int patchSetNumber = 1;
String quotedIncludedPathsGlobPattern =
Optional.ofNullable(includedPathsGlobPattern)
.map(s -> StringUtils.wrap(s, "'"))
.orElse(null);
String script =
"node {\n"
+ "stage('Build') {\n"
Expand Down Expand Up @@ -185,14 +226,16 @@ private Job createPipelineJob(GerritChange change, ReviewCommentType commentType
+ "issueFilterConfig: [\n"
+ "severity: 'MINOR',\n"
+ "newIssuesOnly: false,\n"
+ "changedLinesOnly: true\n"
+ "changedLinesOnly: true,\n"
+ String.format("includedPathsGlobPattern: %s\n", quotedIncludedPathsGlobPattern)
+ "]\n" // issueFilterConfig
+ "],\n" // reviewConfig
+ "scoreConfig: [\n"
+ "issueFilterConfig: [\n"
+ "severity: 'MINOR',"
+ "newIssuesOnly: false,"
+ "changedLinesOnly: true"
+ "severity: 'MINOR',\n"
+ "newIssuesOnly: false,\n"
+ "changedLinesOnly: true,\n"
+ String.format("includedPathsGlobPattern: %s\n", quotedIncludedPathsGlobPattern)
+ "],\n" // issueFilterConfig
+ String.format("category: '%s',\n", GerritServer.CODE_QUALITY_LABEL)
+ "noIssuesScore: 1,\n"
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,104 @@
package org.jenkinsci.plugins.sonargerrit.sonar;

import static java.util.Objects.requireNonNull;
import static org.assertj.core.api.Assertions.assertThat;

import java.util.Date;
import java.util.Optional;
import org.junit.jupiter.api.Test;

/** @author Réda Housni Alaoui */
class ByGlobPatternPredicateTest {

@Test
void test() {
assertThat(new ByGlobPatternPredicate("**/*").apply(new DummyIssue("/foo.bar"))).isTrue();
assertThat(new ByGlobPatternPredicate("/foo.bar").apply(new DummyIssue("/foo.bar"))).isTrue();
assertThat(new ByGlobPatternPredicate("/foo/*").apply(new DummyIssue("/foo/bar"))).isTrue();
assertThat(new ByGlobPatternPredicate("/foo/**").apply(new DummyIssue("/foo/bar/baz")))
.isTrue();
assertThat(new ByGlobPatternPredicate("/foo/**").apply(new DummyIssue("/foo/bar/baz/bat.toto")))
.isTrue();
assertThat(new ByGlobPatternPredicate("/bar/**").apply(new DummyIssue("/foo/bar/baz/bat.toto")))
.isFalse();
}

private static class DummyIssue implements Issue {

private final String filePath;

DummyIssue(String filePath) {
this.filePath = requireNonNull(filePath);
}

@Override
public String inspectorName() {
return null;
}

@Override
public String inspectionId() {
return null;
}

@Override
public Optional<String> detailUrl() {
return Optional.empty();
}

@Override
public String getFilepath() {
return filePath;
}

@Override
public String getKey() {
return null;
}

@Override
public String getComponent() {
return null;
}

@Override
public Integer getLine() {
return null;
}

@Override
public String getMessage() {
return null;
}

@Override
public Severity getSeverity() {
return null;
}

@Override
public String getRule() {
return null;
}

@Override
public String getRuleUrl() {
return null;
}

@Override
public String getStatus() {
return null;
}

@Override
public boolean isNew() {
return false;
}

@Override
public Date getCreationDate() {
return null;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ public String addInstallation() throws IOException {
ZipExtractionInstaller installer =
new ZipExtractionInstaller(
null,
"https://dlcdn.apache.org/maven/maven-3/3.8.4/binaries/apache-maven-3.8.4-bin.tar.gz",
"https://archive.apache.org/dist/maven/maven-3/3.8.4/binaries/apache-maven-3.8.4-bin.tar.gz",
"apache-maven-3.8.4");

String name = UUID.randomUUID().toString();
Expand Down

0 comments on commit b9c9b38

Please sign in to comment.