Skip to content

Commit

Permalink
SONAR-6303 Apply feedback from PR
Browse files Browse the repository at this point in the history
  • Loading branch information
jblievremont committed Apr 15, 2015
1 parent 03084ba commit 042ce5a
Show file tree
Hide file tree
Showing 7 changed files with 352 additions and 16 deletions.
Expand Up @@ -377,7 +377,7 @@ void startLevel4Components(ComponentContainer pico) {
pico.addSingleton(QProfileLookup.class);
pico.addSingleton(QProfileProjectOperations.class);
pico.addSingleton(QProfileProjectLookup.class);
pico.addSingleton(QProfileComparator.class);
pico.addSingleton(QProfileComparison.class);
pico.addSingleton(BuiltInProfiles.class);
pico.addSingleton(QProfileRestoreBuiltInAction.class);
pico.addSingleton(QProfileSearchAction.class);
Expand Down
Expand Up @@ -34,13 +34,13 @@
import java.util.Map;
import java.util.Set;

public class QProfileComparator implements ServerComponent {
public class QProfileComparison implements ServerComponent {

private final DbClient dbClient;

private final QProfileLoader profileLoader;

public QProfileComparator(DbClient dbClient, QProfileLoader profileLoader) {
public QProfileComparison(DbClient dbClient, QProfileLoader profileLoader) {
this.dbClient = dbClient;
this.profileLoader = profileLoader;
}
Expand Down Expand Up @@ -78,18 +78,17 @@ private void compare(String leftKey, String rightKey, DbSession session, QProfil
} else if (!rightActiveRulesByRuleKey.containsKey(ruleKey)) {
result.inLeft.put(ruleKey, leftActiveRulesByRuleKey.get(ruleKey));
} else {
compare(leftActiveRulesByRuleKey.get(ruleKey), rightActiveRulesByRuleKey.get(ruleKey), result);
compareActivationParams(leftActiveRulesByRuleKey.get(ruleKey), rightActiveRulesByRuleKey.get(ruleKey), result);
}
}
}

private void compare(ActiveRule leftRule, ActiveRule rightRule, QProfileComparisonResult result) {
private void compareActivationParams(ActiveRule leftRule, ActiveRule rightRule, QProfileComparisonResult result) {
RuleKey key = leftRule.key().ruleKey();
if (leftRule.params().equals(rightRule.params()) && leftRule.severity().equals(rightRule.severity())) {
result.same.put(key, leftRule);
} else {
ActiveRuleDiff diff = new ActiveRuleDiff();
diff.key = key;

if (leftRule.severity().equals(rightRule.severity())) {
diff.leftSeverity = diff.rightSeverity = leftRule.severity();
Expand Down Expand Up @@ -156,15 +155,10 @@ public Collection<RuleKey> collectRuleKeys() {
}

public static class ActiveRuleDiff {
private RuleKey key;
private String leftSeverity;
private String rightSeverity;
private MapDifference<String, String> paramDifference;

public RuleKey key() {
return key;
}

public String leftSeverity() {
return leftSeverity;
}
Expand Down
Expand Up @@ -32,9 +32,9 @@
import org.sonar.core.qualityprofile.db.QualityProfileDto;
import org.sonar.core.util.NonNullInputFunction;
import org.sonar.server.qualityprofile.ActiveRule;
import org.sonar.server.qualityprofile.QProfileComparator;
import org.sonar.server.qualityprofile.QProfileComparator.ActiveRuleDiff;
import org.sonar.server.qualityprofile.QProfileComparator.QProfileComparisonResult;
import org.sonar.server.qualityprofile.QProfileComparison;
import org.sonar.server.qualityprofile.QProfileComparison.ActiveRuleDiff;
import org.sonar.server.qualityprofile.QProfileComparison.QProfileComparisonResult;
import org.sonar.server.rule.Rule;
import org.sonar.server.rule.RuleRepositories;
import org.sonar.server.rule.RuleRepositories.Repository;
Expand All @@ -49,12 +49,12 @@ public class QProfileCompareAction implements BaseQProfileWsAction {
private static final String PARAM_LEFT_KEY = "leftKey";
private static final String PARAM_RIGHT_KEY = "rightKey";

private final QProfileComparator comparator;
private final QProfileComparison comparator;
private final RuleService ruleService;
private final RuleRepositories ruleRepositories;
private final Languages languages;

public QProfileCompareAction(QProfileComparator comparator, RuleService ruleService, RuleRepositories ruleRepositories, Languages languages) {
public QProfileCompareAction(QProfileComparison comparator, RuleService ruleService, RuleRepositories ruleRepositories, Languages languages) {
this.comparator = comparator;
this.ruleService = ruleService;
this.ruleRepositories = ruleRepositories;
Expand Down
@@ -0,0 +1,242 @@
/*
* SonarQube, open source software quality management tool.
* Copyright (C) 2008-2014 SonarSource
* mailto:contact AT sonarsource DOT com
*
* SonarQube is free software; you can redistribute it and/or
* modify it under the terms of the GNU Lesser General Public
* License as published by the Free Software Foundation; either
* version 3 of the License, or (at your option) any later version.
*
* SonarQube is distributed in the hope that it will be useful,
* but WITHOUT ANY WARRANTY; without even the implied warranty of
* MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
* Lesser General Public License for more details.
*
* You should have received a copy of the GNU Lesser General Public License
* along with this program; if not, write to the Free Software Foundation,
* Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
*/
package org.sonar.server.qualityprofile;

import com.google.common.collect.MapDifference.ValueDifference;
import org.assertj.core.data.MapEntry;
import org.junit.After;
import org.junit.Before;
import org.junit.ClassRule;
import org.junit.Test;
import org.sonar.api.rule.Severity;
import org.sonar.api.server.rule.RuleParamType;
import org.sonar.core.persistence.DbSession;
import org.sonar.core.qualityprofile.db.QualityProfileDto;
import org.sonar.core.rule.RuleDto;
import org.sonar.core.rule.RuleParamDto;
import org.sonar.server.db.DbClient;
import org.sonar.server.qualityprofile.QProfileComparison.ActiveRuleDiff;
import org.sonar.server.qualityprofile.QProfileComparison.QProfileComparisonResult;
import org.sonar.server.qualityprofile.index.ActiveRuleIndex;
import org.sonar.server.rule.RuleTesting;
import org.sonar.server.tester.ServerTester;

import static org.assertj.core.api.Assertions.assertThat;

public class QProfileComparisonMediumTest {

@ClassRule
public static ServerTester tester = new ServerTester();

DbClient db;
DbSession dbSession;
ActiveRuleIndex index;
RuleActivator ruleActivator;
QProfileComparison comparison;

RuleDto xooRule1;
RuleDto xooRule2;
QualityProfileDto left;
QualityProfileDto right;

@Before
public void before() {
tester.clearDbAndIndexes();
db = tester.get(DbClient.class);
dbSession = db.openSession(false);
ruleActivator = tester.get(RuleActivator.class);
comparison = tester.get(QProfileComparison.class);

xooRule1 = RuleTesting.newXooX1().setSeverity("MINOR");
xooRule2 = RuleTesting.newXooX2().setSeverity("MAJOR");
db.ruleDao().insert(dbSession, xooRule1, xooRule2);
db.ruleDao().addRuleParam(dbSession, xooRule1, RuleParamDto.createFor(xooRule1)
.setName("max").setType(RuleParamType.INTEGER.type()));
db.ruleDao().addRuleParam(dbSession, xooRule1, RuleParamDto.createFor(xooRule1)
.setName("min").setType(RuleParamType.INTEGER.type()));

left = QProfileTesting.newXooP1();
right = QProfileTesting.newXooP2();
db.qualityProfileDao().insert(dbSession, left, right);
dbSession.commit();
dbSession.clearCache();
}

@After
public void after() throws Exception {
dbSession.close();
}

@Test
public void compare_empty_profiles() throws Exception {
QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey());
assertThat(result.left().getKey()).isEqualTo(left.getKey());
assertThat(result.right().getKey()).isEqualTo(right.getKey());
assertThat(result.same()).isEmpty();
assertThat(result.inLeft()).isEmpty();
assertThat(result.inRight()).isEmpty();
assertThat(result.modified()).isEmpty();
assertThat(result.collectRuleKeys()).isEmpty();
}

@Test
public void compare_same() throws Exception {
RuleActivation commonActivation = new RuleActivation(xooRule1.getKey())
.setSeverity(Severity.CRITICAL)
.setParameter("min", "7")
.setParameter("max", "42");
ruleActivator.activate(dbSession, commonActivation, left);
ruleActivator.activate(dbSession, commonActivation, right);
dbSession.commit();

QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey());
assertThat(result.left().getKey()).isEqualTo(left.getKey());
assertThat(result.right().getKey()).isEqualTo(right.getKey());
assertThat(result.same()).isNotEmpty().containsOnlyKeys(xooRule1.getKey());
assertThat(result.inLeft()).isEmpty();
assertThat(result.inRight()).isEmpty();
assertThat(result.modified()).isEmpty();
assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey());
}

@Test
public void compare_only_left() throws Exception {
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()), left);
dbSession.commit();

QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey());
assertThat(result.left().getKey()).isEqualTo(left.getKey());
assertThat(result.right().getKey()).isEqualTo(right.getKey());
assertThat(result.same()).isEmpty();
assertThat(result.inLeft()).isNotEmpty().containsOnlyKeys(xooRule1.getKey());
assertThat(result.inRight()).isEmpty();
assertThat(result.modified()).isEmpty();
assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey());
}

@Test
public void compare_only_right() throws Exception {
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()), right);
dbSession.commit();

QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey());
assertThat(result.left().getKey()).isEqualTo(left.getKey());
assertThat(result.right().getKey()).isEqualTo(right.getKey());
assertThat(result.same()).isEmpty();
assertThat(result.inLeft()).isEmpty();
assertThat(result.inRight()).isNotEmpty().containsOnlyKeys(xooRule1.getKey());
assertThat(result.modified()).isEmpty();
assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey());
}

@Test
public void compare_disjoint() throws Exception {
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()), left);
ruleActivator.activate(dbSession, new RuleActivation(xooRule2.getKey()), right);
dbSession.commit();

QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey());
assertThat(result.left().getKey()).isEqualTo(left.getKey());
assertThat(result.right().getKey()).isEqualTo(right.getKey());
assertThat(result.same()).isEmpty();
assertThat(result.inLeft()).isNotEmpty().containsOnlyKeys(xooRule1.getKey());
assertThat(result.inRight()).isNotEmpty().containsOnlyKeys(xooRule2.getKey());
assertThat(result.modified()).isEmpty();
assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey(), xooRule2.getKey());
}

@Test
public void compare_modified_severity() throws Exception {
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setSeverity(Severity.CRITICAL), left);
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setSeverity(Severity.BLOCKER), right);
dbSession.commit();

QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey());
assertThat(result.left().getKey()).isEqualTo(left.getKey());
assertThat(result.right().getKey()).isEqualTo(right.getKey());
assertThat(result.same()).isEmpty();
assertThat(result.inLeft()).isEmpty();
assertThat(result.inRight()).isEmpty();
assertThat(result.modified()).isNotEmpty().containsOnlyKeys(xooRule1.getKey());
assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey());

ActiveRuleDiff activeRuleDiff = result.modified().get(xooRule1.getKey());
assertThat(activeRuleDiff.leftSeverity()).isEqualTo(Severity.CRITICAL);
assertThat(activeRuleDiff.rightSeverity()).isEqualTo(Severity.BLOCKER);
assertThat(activeRuleDiff.paramDifference().areEqual()).isTrue();
}

@Test
public void compare_modified_param() throws Exception {
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setParameter("max", "20"), left);
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setParameter("max", "30"), right);
dbSession.commit();

QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey());
assertThat(result.left().getKey()).isEqualTo(left.getKey());
assertThat(result.right().getKey()).isEqualTo(right.getKey());
assertThat(result.same()).isEmpty();
assertThat(result.inLeft()).isEmpty();
assertThat(result.inRight()).isEmpty();
assertThat(result.modified()).isNotEmpty().containsOnlyKeys(xooRule1.getKey());
assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey());

ActiveRuleDiff activeRuleDiff = result.modified().get(xooRule1.getKey());
assertThat(activeRuleDiff.leftSeverity()).isEqualTo(activeRuleDiff.rightSeverity()).isEqualTo(xooRule1.getSeverityString());
assertThat(activeRuleDiff.paramDifference().areEqual()).isFalse();
assertThat(activeRuleDiff.paramDifference().entriesDiffering()).isNotEmpty();
ValueDifference<String> paramDiff = activeRuleDiff.paramDifference().entriesDiffering().get("max");
assertThat(paramDiff.leftValue()).isEqualTo("20");
assertThat(paramDiff.rightValue()).isEqualTo("30");
}

@Test
public void compare_different_params() throws Exception {
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setParameter("max", "20"), left);
ruleActivator.activate(dbSession, new RuleActivation(xooRule1.getKey()).setParameter("min", "5"), right);
dbSession.commit();

QProfileComparisonResult result = comparison.compare(left.getKey(), right.getKey());
assertThat(result.left().getKey()).isEqualTo(left.getKey());
assertThat(result.right().getKey()).isEqualTo(right.getKey());
assertThat(result.same()).isEmpty();
assertThat(result.inLeft()).isEmpty();
assertThat(result.inRight()).isEmpty();
assertThat(result.modified()).isNotEmpty().containsOnlyKeys(xooRule1.getKey());
assertThat(result.collectRuleKeys()).containsOnly(xooRule1.getKey());

ActiveRuleDiff activeRuleDiff = result.modified().get(xooRule1.getKey());
assertThat(activeRuleDiff.leftSeverity()).isEqualTo(activeRuleDiff.rightSeverity()).isEqualTo(xooRule1.getSeverityString());
assertThat(activeRuleDiff.paramDifference().areEqual()).isFalse();
assertThat(activeRuleDiff.paramDifference().entriesDiffering()).isEmpty();
assertThat(activeRuleDiff.paramDifference().entriesOnlyOnLeft()).containsExactly(MapEntry.entry("max", "20"));
assertThat(activeRuleDiff.paramDifference().entriesOnlyOnRight()).containsExactly(MapEntry.entry("min", "5"));
}

@Test(expected = IllegalArgumentException.class)
public void fail_on_unknown_left() throws Exception {
comparison.compare("polop", right.getKey());
}

@Test(expected = IllegalArgumentException.class)
public void fail_on_unknown_right() throws Exception {
comparison.compare(left.getKey(), "polop");
}
}
Expand Up @@ -115,6 +115,42 @@ public void compare_nominal() throws Exception {
.execute().assertJson(this.getClass(), "compare_nominal.json");
}

@Test
public void compare_param_on_left() throws Exception {
RuleDto rule1 = createRuleWithParam("xoo", "rule1");

QualityProfileDto profile1 = createProfile("xoo", "Profile 1", "xoo-profile-1-01234");
createActiveRuleWithParam(rule1, profile1, "polop");
session.commit();

QualityProfileDto profile2 = createProfile("xoo", "Profile 2", "xoo-profile-2-12345");
createActiveRule(rule1, profile2);
session.commit();

wsTester.newGetRequest("api/qualityprofiles", "compare")
.setParam("leftKey", profile1.getKey())
.setParam("rightKey", profile2.getKey())
.execute().assertJson(this.getClass(), "compare_param_on_left.json");
}

@Test
public void compare_param_on_right() throws Exception {
RuleDto rule1 = createRuleWithParam("xoo", "rule1");

QualityProfileDto profile1 = createProfile("xoo", "Profile 1", "xoo-profile-1-01234");
createActiveRule(rule1, profile1);
session.commit();

QualityProfileDto profile2 = createProfile("xoo", "Profile 2", "xoo-profile-2-12345");
createActiveRuleWithParam(rule1, profile2, "polop");
session.commit();

wsTester.newGetRequest("api/qualityprofiles", "compare")
.setParam("leftKey", profile1.getKey())
.setParam("rightKey", profile2.getKey())
.execute().assertJson(this.getClass(), "compare_param_on_right.json");
}

@Test(expected = IllegalArgumentException.class)
public void fail_on_missing_left_param() throws Exception {
wsTester.newGetRequest("api/qualityprofiles", "compare")
Expand Down

0 comments on commit 042ce5a

Please sign in to comment.