Skip to content
This repository has been archived by the owner on Jan 7, 2021. It is now read-only.

implement approval conditioned on issue severities #141

Merged
merged 1 commit into from
Aug 2, 2017
Merged

implement approval conditioned on issue severities #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ Go to Stash general settings screen on SonarQube server to fill:

**Stash reviewer approval** (sonar.stash.reviewer.approval): SonarQube is able to approve the pull-request if there is no new issue introduced by the change. By default, this feature is deactivated: if activated, **Stash base user must have REPO_WRITE permission for the repositories.**

**Approval severity** (sonar.stash.reviewer.approval.severity.threshold): Only approve the pull-request if no issues higher than this threshold are detected.

**Include Analysis Overview Comment** (sonar.stash.include.overview): Toggles whether a comment with overview information should be created.

![Screenshot SonarQube plugin](resources/Sonar-plugin-approver.PNG)
Expand Down
22 changes: 15 additions & 7 deletions src/main/java/org/sonar/plugins/stash/StashIssueReportingPostJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.sonar.plugins.stash;

import java.util.Collection;
import java.util.Optional;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.sonar.api.BatchComponent;
Expand Down Expand Up @@ -116,9 +118,6 @@ private void postInfoAndPRsActions(PullRequestRef pr, List<Issue> issueReport, i
StashDiffReport diffReport,
StashClient stashClient) {

// Some local definitions
boolean canApprovePullrequest = config.canApprovePullRequest();

int issueTotal = issueReport.size();

// if threshold exceeded, do not push issue list to Stash
Expand All @@ -134,17 +133,26 @@ private void postInfoAndPRsActions(PullRequestRef pr, List<Issue> issueReport, i
stashRequestFacade.postAnalysisOverview(pr, issueThreshold, issueReport, stashClient);
}

// if no new issues and coverage is improved,
// plugin approves the pull-request
if (canApprovePullrequest) {
if (issueTotal == 0) {
if (config.canApprovePullRequest()) {
if (shouldApprovePullRequest(config.getApprovalSeverityThreshold(), issueReport)) {
stashRequestFacade.approvePullRequest(pr, stashClient);
} else {
stashRequestFacade.resetPullRequestApproval(pr, stashClient);
}
}
}

static boolean shouldApprovePullRequest(Optional<String> approvalSeverityThreshold, Collection<Issue> report) {
if (approvalSeverityThreshold.isPresent()) {
SeverityComparator sc = new SeverityComparator();
return report.stream().noneMatch(issue ->
sc.compare(issue.severity(), approvalSeverityThreshold.get()) > 0
);
}

return report.isEmpty();
}


/*
* Custom exception to keep nested if statements under control
Expand Down
9 changes: 9 additions & 0 deletions src/main/java/org/sonar/plugins/stash/StashPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ public class StashPlugin extends SonarPlugin {
public static final String STASH_PASSWORD = "sonar.stash.password";
public static final String STASH_PASSWORD_ENVIRONMENT_VARIABLE = "sonar.stash.password.variable";
public static final String STASH_REVIEWER_APPROVAL = "sonar.stash.reviewer.approval";
public static final String STASH_REVIEWER_APPROVAL_SEVERITY_THRESHOLD = "sonar.stash.reviewer.approval.severity.threshold";
public static final String STASH_ISSUE_THRESHOLD = "sonar.stash.issue.threshold";
public static final String STASH_TIMEOUT = "sonar.stash.timeout";
public static final String SONARQUBE_URL = "sonar.host.url";
Expand Down Expand Up @@ -130,6 +131,14 @@ public List getExtensions() {
.subCategory(CONFIG_PAGE_SUB_CATEGORY_STASH)
.onQualifiers(Qualifiers.PROJECT)
.defaultValue(DEFAULT_STASH_THRESHOLD_VALUE).build(),
PropertyDefinition.builder(STASH_REVIEWER_APPROVAL_SEVERITY_THRESHOLD)
.name("Threshold tie the approval to the severity of the found issues")
.description("Maximum severity of an issue for approval to complete")
.type(PropertyType.SINGLE_SELECT_LIST)
.subCategory(CONFIG_PAGE_SUB_CATEGORY_STASH)
.onQualifiers(Qualifiers.PROJECT)
.defaultValue(SEVERITY_NONE)
.options(SEVERITY_LIST_WITH_NONE).build(),
PropertyDefinition.builder(STASH_TASK_SEVERITY_THRESHOLD)
.name("Stash tasks severity threshold")
.description("Only create tasks for issues with the same or higher severity")
Expand Down
18 changes: 13 additions & 5 deletions src/main/java/org/sonar/plugins/stash/StashPluginConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,11 +86,11 @@ public boolean resetComments() {
}

public Optional<String> getTaskIssueSeverityThreshold() {
String setting = settings.getString(StashPlugin.STASH_TASK_SEVERITY_THRESHOLD);
if (StashPlugin.SEVERITY_NONE.equals(setting)) {
return Optional.empty();
}
return Optional.of(setting);
return getOptionalSeveritySetting(StashPlugin.STASH_TASK_SEVERITY_THRESHOLD);
}

public Optional<String> getApprovalSeverityThreshold() {
return getOptionalSeveritySetting(StashPlugin.STASH_REVIEWER_APPROVAL_SEVERITY_THRESHOLD);
}

public String getSonarQubeVersion() {
Expand Down Expand Up @@ -125,4 +125,12 @@ public Set<RuleKey> excludedRules() {
.map(RuleKey::parse)
.collect(Collectors.toSet());
}

private Optional<String> getOptionalSeveritySetting(String key) {
String setting = settings.getString(key);
if (StashPlugin.SEVERITY_NONE.equals(setting)) {
return Optional.empty();
}
return Optional.of(setting);
}
}
35 changes: 35 additions & 0 deletions src/test/java/org/sonar/plugins/stash/StashIssueReportingPostJobTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.sonar.plugins.stash;

import java.util.Optional;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
Expand All @@ -9,7 +10,9 @@
import org.sonar.api.batch.SensorContext;
import org.sonar.api.issue.Issue;
import org.sonar.api.issue.ProjectIssues;
import org.sonar.api.issue.internal.DefaultIssue;
import org.sonar.api.resources.Project;
import org.sonar.api.rule.Severity;
import org.sonar.plugins.stash.client.StashClient;
import org.sonar.plugins.stash.client.StashCredentials;
import org.sonar.plugins.stash.coverage.CoverageProjectStore;
Expand All @@ -19,6 +22,8 @@
import java.util.ArrayList;
import java.util.List;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.eq;
import static org.mockito.Mockito.spy;
import static org.mockito.Mockito.times;
Expand Down Expand Up @@ -247,6 +252,36 @@ public void testExecuteOnWithNoDiffReport() throws Exception {
verify(stashRequestFacade, times(0)).resetPullRequestApproval(eq(pr), (StashClient)Mockito.anyObject());
}

@Test
public void testShouldApprovePullRequest() {
Issue minorIssue = new DefaultIssue().setSeverity(Severity.MINOR);
Issue majorIssue = new DefaultIssue().setSeverity(Severity.MAJOR);

report = new ArrayList<>();

report.add(minorIssue);
report.add(majorIssue);

assertFalse(
StashIssueReportingPostJob.shouldApprovePullRequest(Optional.empty(), report)
);

report.clear();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

multiple asserts in a single test method are considered a test smell
It is often better to split such a test method into separate cases. One can give those separate test method meathingful names and do a proper setup in those smaller methods

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general I agree with this being a smell.
However as far as I know this is a concern when testing objects with (complex) internal state.
So when doing multiple asserts the later ones may depend on implicitly changed state in the object.
As the tested functionality here is only a single static method there is no state.
Furthermore by doing it this way if some assertion failed is immediately obvious how this assertion differed from the previous one.
If there are multiple test functions all doing essentially the same setup (only progressing) then I first have to compare all the test methods where the actual differences are.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test with multiple asserts might fail at first assertion and other assertions are not executed at all.

It is good when one break makes only one assert fail, letting other asserts be executed in separate test methods.

As soon as you break test code to separate tests, you have a problem of repetitous setup. It is solved easily by single @Before method or a helper method called in each method.
If the assert should be checked only after some other checks have succeeded (which are checked in separate test methods) then one can use assume* methods of JUnit.

As soon as you have your tests split this way, single defect breaks one test only. Some other tests may become yellow telling you the defect is essential for checks performed there. The good thing you'd know what have failed exactly. If several things have failed you'd know all of them.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I totally agree with you if the object under test is complex and contains internal state. However for a static method I think it is overkill. I very much prefer a simple test execution where I can see the difference of the parameters between the assertions very easily.

It would be even better to have nice parameterized tests in Java like in python, but in Junit this involves a ridiculous amount of boilerplate hiding the actual logic.

Can we agree to disagree on this one with me (ab)using my maintainer powers to keep the status quo?

assertTrue(
StashIssueReportingPostJob.shouldApprovePullRequest(Optional.empty(), report)
);

report.add(minorIssue);
assertTrue(
StashIssueReportingPostJob.shouldApprovePullRequest(Optional.of(Severity.MINOR), report)
Copy link

@kzaikin kzaikin Jul 26, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, shouldApprovePullRequest is an internal method, that is implementation details.
You might test it until one wants to change internal implementation details. Normally tests should not be modified upon such a change to ensure public contract is not changed. However when internal methods are exposed and tested, tests must be modified too. It can lead to extra work

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The method is made a package-scoped static method especially to improve testing.
There is no need for any fixtures or the mocking of dependencies.

);

report.add(majorIssue);
assertFalse(
StashIssueReportingPostJob.shouldApprovePullRequest(Optional.of(Severity.MINOR), report)
);
}

/* FIXME
@Test
public void testExecuteOnWithPullRequestApprovalAndNoNewIssueAndCodeCoverageEvolutionPositive() throws Exception {
Expand Down
7 changes: 5 additions & 2 deletions src/test/java/org/sonar/plugins/stash/StashPluginTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,10 +38,13 @@ public void testGetExtensions() {
PropertyDefinition SP_SIT = (PropertyDefinition)ext.get(15);
assertEquals(StashPlugin.STASH_ISSUE_THRESHOLD, SP_SIT.key());

PropertyDefinition SP_STST = (PropertyDefinition)ext.get(16);
PropertyDefinition SP_RAST = (PropertyDefinition)ext.get(16);
assertEquals(StashPlugin.STASH_REVIEWER_APPROVAL_SEVERITY_THRESHOLD, SP_RAST.key());

PropertyDefinition SP_STST = (PropertyDefinition)ext.get(17);
assertEquals(StashPlugin.STASH_TASK_SEVERITY_THRESHOLD, SP_STST.key());

PropertyDefinition SP_SIAO = (PropertyDefinition)ext.get(17);
PropertyDefinition SP_SIAO = (PropertyDefinition)ext.get(18);
assertEquals(StashPlugin.STASH_INCLUDE_ANALYSIS_OVERVIEW, SP_SIAO.key());
}
}