SONAR-11776 Quality Gate failure conditions should be sorted

5 files changed, 99 insertions, 28 deletions

diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/qualitygate/EvaluatedQualityGate.java b/server/sonar-server-common/src/main/java/org/sonar/server/qualitygate/EvaluatedQualityGate.java
index 06d2d1a4a43..b45f0c0da29 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/qualitygate/EvaluatedQualityGate.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/qualitygate/EvaluatedQualityGate.java
@@ -20,11 +20,18 @@
 package org.sonar.server.qualitygate;
 
 import com.google.common.collect.ImmutableSet;
-import java.util.HashMap;
+import java.util.ArrayList;
+import java.util.Arrays;
+import java.util.Collection;
+import java.util.Comparator;
+import java.util.LinkedHashMap;
+import java.util.List;
 import java.util.Map;
 import java.util.Objects;
 import java.util.Set;
+import java.util.function.Function;
 import java.util.stream.Collectors;
+import java.util.stream.IntStream;
 import javax.annotation.Nullable;
 import javax.annotation.concurrent.Immutable;
 import org.sonar.api.measures.Metric;
@@ -32,15 +39,25 @@ import org.sonar.server.qualitygate.EvaluatedCondition.EvaluationStatus;
 
 import static com.google.common.base.Preconditions.checkArgument;
 import static java.util.Objects.requireNonNull;
+import static org.sonar.api.measures.CoreMetrics.COVERAGE_KEY;
+import static org.sonar.api.measures.CoreMetrics.DUPLICATED_LINES_DENSITY_KEY;
+import static org.sonar.api.measures.CoreMetrics.NEW_COVERAGE_KEY;
+import static org.sonar.api.measures.CoreMetrics.NEW_DUPLICATED_LINES_DENSITY_KEY;
+import static org.sonar.api.measures.CoreMetrics.NEW_MAINTAINABILITY_RATING_KEY;
+import static org.sonar.api.measures.CoreMetrics.NEW_RELIABILITY_RATING_KEY;
+import static org.sonar.api.measures.CoreMetrics.NEW_SECURITY_RATING_KEY;
+import static org.sonar.api.measures.CoreMetrics.RELIABILITY_RATING_KEY;
+import static org.sonar.api.measures.CoreMetrics.SECURITY_RATING_KEY;
+import static org.sonar.api.measures.CoreMetrics.SQALE_RATING_KEY;
 
 @Immutable
 public class EvaluatedQualityGate {
   private final QualityGate qualityGate;
   private final Metric.Level status;
-  private final Set<EvaluatedCondition> evaluatedConditions;
+  private final Collection<EvaluatedCondition> evaluatedConditions;
   private final boolean ignoredConditionsOnSmallChangeset;
 
-  private EvaluatedQualityGate(QualityGate qualityGate, Metric.Level status, Set<EvaluatedCondition> evaluatedConditions, boolean ignoredConditionsOnSmallChangeset) {
+  private EvaluatedQualityGate(QualityGate qualityGate, Metric.Level status, Collection<EvaluatedCondition> evaluatedConditions, boolean ignoredConditionsOnSmallChangeset) {
     this.qualityGate = requireNonNull(qualityGate, "qualityGate can't be null");
     this.status = requireNonNull(status, "status can't be null");
     this.evaluatedConditions = evaluatedConditions;
@@ -55,7 +72,7 @@ public class EvaluatedQualityGate {
     return status;
   }
 
-  public Set<EvaluatedCondition> getEvaluatedConditions() {
+  public Collection<EvaluatedCondition> getEvaluatedConditions() {
     return evaluatedConditions;
   }
 
@@ -96,9 +113,21 @@ public class EvaluatedQualityGate {
   }
 
   public static final class Builder {
+    private static final List<String> CONDITIONS_ORDER = Arrays.asList(NEW_SECURITY_RATING_KEY, SECURITY_RATING_KEY, NEW_RELIABILITY_RATING_KEY,
+      RELIABILITY_RATING_KEY, NEW_MAINTAINABILITY_RATING_KEY, SQALE_RATING_KEY, NEW_COVERAGE_KEY, COVERAGE_KEY, NEW_DUPLICATED_LINES_DENSITY_KEY,
+      DUPLICATED_LINES_DENSITY_KEY);
+    private static final Map<String, Integer> CONDITIONS_ORDER_IDX = IntStream.range(0, CONDITIONS_ORDER.size()).boxed()
+      .collect(Collectors.toMap(CONDITIONS_ORDER::get, x -> x));
+
+    private static final Comparator<EvaluatedCondition> CONDITION_COMPARATOR = (c1, c2) -> {
+      Function<EvaluatedCondition, Integer> byList = c -> CONDITIONS_ORDER_IDX.getOrDefault(c.getCondition().getMetricKey(), Integer.MAX_VALUE);
+      Function<EvaluatedCondition, String> byMetricKey = c -> c.getCondition().getMetricKey();
+      return Comparator.comparing(byList).thenComparing(byMetricKey).compare(c1, c2);
+    };
+
     private QualityGate qualityGate;
     private Metric.Level status;
-    private final Map<Condition, EvaluatedCondition> evaluatedConditions = new HashMap<>();
+    private final Map<Condition, EvaluatedCondition> evaluatedConditions = new LinkedHashMap<>();
     private boolean ignoredConditionsOnSmallChangeset = false;
 
     private Builder() {
@@ -120,12 +149,12 @@ public class EvaluatedQualityGate {
       return this;
     }
 
-    public Builder addCondition(Condition condition, EvaluationStatus status, @Nullable String value) {
+    public Builder addEvaluatedCondition(Condition condition, EvaluationStatus status, @Nullable String value) {
       evaluatedConditions.put(condition, new EvaluatedCondition(condition, status, value));
       return this;
     }
 
-    public Builder addCondition(EvaluatedCondition c) {
+    public Builder addEvaluatedCondition(EvaluatedCondition c) {
       evaluatedConditions.put(c.getCondition(), c);
       return this;
     }
@@ -135,14 +164,17 @@ public class EvaluatedQualityGate {
     }
 
     public EvaluatedQualityGate build() {
+      checkEvaluatedConditions(qualityGate, evaluatedConditions);
+      List<EvaluatedCondition> sortedEvaluatedConditions = new ArrayList<>(evaluatedConditions.values());
+      sortedEvaluatedConditions.sort(CONDITION_COMPARATOR);
       return new EvaluatedQualityGate(
         this.qualityGate,
         this.status,
-        checkEvaluatedConditions(qualityGate, evaluatedConditions),
+        sortedEvaluatedConditions,
         ignoredConditionsOnSmallChangeset);
     }
 
-    private static Set<EvaluatedCondition> checkEvaluatedConditions(QualityGate qualityGate, Map<Condition, EvaluatedCondition> evaluatedConditions) {
+    private static void checkEvaluatedConditions(QualityGate qualityGate, Map<Condition, EvaluatedCondition> evaluatedConditions) {
       Set<Condition> conditions = qualityGate.getConditions();
 
       Set<Condition> conditionsNotEvaluated = conditions.stream()
@@ -154,8 +186,6 @@ public class EvaluatedQualityGate {
         .filter(c -> !conditions.contains(c))
         .collect(Collectors.toSet());
       checkArgument(unknownConditions.isEmpty(), "Evaluation provided for unknown conditions: %s", unknownConditions);
-
-      return ImmutableSet.copyOf(evaluatedConditions.values());
     }
   }
 }
diff --git a/server/sonar-server-common/src/main/java/org/sonar/server/qualitygate/QualityGateEvaluatorImpl.java b/server/sonar-server-common/src/main/java/org/sonar/server/qualitygate/QualityGateEvaluatorImpl.java
index 9420450295a..49bdb349087 100644
--- a/server/sonar-server-common/src/main/java/org/sonar/server/qualitygate/QualityGateEvaluatorImpl.java
+++ b/server/sonar-server-common/src/main/java/org/sonar/server/qualitygate/QualityGateEvaluatorImpl.java
@@ -54,10 +54,10 @@ public class QualityGateEvaluatorImpl implements QualityGateEvaluator {
       EvaluatedCondition evaluation = ConditionEvaluator.evaluate(condition, measures);
 
       if (isSmallChangeset && evaluation.getStatus() != EvaluationStatus.OK && METRICS_TO_IGNORE_ON_SMALL_CHANGESETS.contains(metricKey)) {
-        result.addCondition(new EvaluatedCondition(evaluation.getCondition(), EvaluationStatus.OK, evaluation.getValue().orElse(null)));
+        result.addEvaluatedCondition(new EvaluatedCondition(evaluation.getCondition(), EvaluationStatus.OK, evaluation.getValue().orElse(null)));
         result.setIgnoredConditionsOnSmallChangeset(true);
       } else {
-        result.addCondition(evaluation);
+        result.addEvaluatedCondition(evaluation);
       }
     });
 
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/EvaluatedQualityGateTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/EvaluatedQualityGateTest.java
index 03ed5e410b1..621cebb094e 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/EvaluatedQualityGateTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/EvaluatedQualityGateTest.java
@@ -20,11 +20,14 @@
 package org.sonar.server.qualitygate;
 
 import com.google.common.collect.ImmutableSet;
+import java.util.Arrays;
+import java.util.HashSet;
 import java.util.Random;
 import org.apache.commons.lang.RandomStringUtils;
 import org.junit.Rule;
 import org.junit.Test;
 import org.junit.rules.ExpectedException;
+import org.sonar.api.measures.CoreMetrics;
 import org.sonar.api.measures.Metric.Level;
 
 import static java.util.Collections.emptySet;
@@ -36,9 +39,13 @@ public class EvaluatedQualityGateTest {
   private static final String QUALITY_GATE_ID = "qg_id";
   private static final String QUALITY_GATE_NAME = "qg_name";
   private static final QualityGate NO_CONDITION_QUALITY_GATE = new QualityGate(QUALITY_GATE_ID, QUALITY_GATE_NAME, emptySet());
-  private static final Condition CONDITION_1 = new Condition("metric_key", Condition.Operator.LESS_THAN, "2");
-  private static final Condition CONDITION_2 = new Condition("metric_key_2", Condition.Operator.GREATER_THAN, "6");
+  private static final Condition CONDITION_1 = new Condition("metric_key_1", Condition.Operator.LESS_THAN, "2");
+  private static final Condition CONDITION_2 = new Condition("a_metric", Condition.Operator.GREATER_THAN, "6");
+  private static final Condition CONDITION_3 = new Condition(CoreMetrics.NEW_MAINTAINABILITY_RATING_KEY, Condition.Operator.GREATER_THAN, "6");
+
   private static final QualityGate ONE_CONDITION_QUALITY_GATE = new QualityGate(QUALITY_GATE_ID, QUALITY_GATE_NAME, singleton(CONDITION_1));
+  private static final QualityGate ALL_CONDITIONS_QUALITY_GATE = new QualityGate(QUALITY_GATE_ID, QUALITY_GATE_NAME,
+    new HashSet<>(Arrays.asList(CONDITION_1, CONDITION_2, CONDITION_3)));
 
   @Rule
   public ExpectedException expectedException = ExpectedException.none();
@@ -66,7 +73,7 @@ public class EvaluatedQualityGateTest {
     expectedException.expect(NullPointerException.class);
     expectedException.expectMessage("condition can't be null");
 
-    builder.addCondition(null, EvaluatedCondition.EvaluationStatus.ERROR, "a_value");
+    builder.addEvaluatedCondition(null, EvaluatedCondition.EvaluationStatus.ERROR, "a_value");
   }
 
   @Test
@@ -74,12 +81,12 @@ public class EvaluatedQualityGateTest {
     expectedException.expect(NullPointerException.class);
     expectedException.expectMessage("status can't be null");
 
-    builder.addCondition(new Condition("metric_key", Condition.Operator.LESS_THAN, "2"), null, "a_value");
+    builder.addEvaluatedCondition(new Condition("metric_key", Condition.Operator.LESS_THAN, "2"), null, "a_value");
   }
 
   @Test
   public void addCondition_accepts_null_value() {
-    builder.addCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.NO_VALUE, null);
+    builder.addEvaluatedCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.NO_VALUE, null);
 
     assertThat(builder.getEvaluatedConditions())
       .containsOnly(new EvaluatedCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.NO_VALUE, null));
@@ -94,7 +101,7 @@ public class EvaluatedQualityGateTest {
   public void build_fails_with_IAE_if_condition_added_and_no_on_QualityGate() {
     builder.setQualityGate(NO_CONDITION_QUALITY_GATE)
       .setStatus(randomStatus)
-      .addCondition(CONDITION_1, randomEvaluationStatus, randomValue);
+      .addEvaluatedCondition(CONDITION_1, randomEvaluationStatus, randomValue);
 
     expectedException.expect(IllegalArgumentException.class);
     expectedException.expectMessage("Evaluation provided for unknown conditions: [" + CONDITION_1 + "]");
@@ -114,11 +121,27 @@ public class EvaluatedQualityGateTest {
   }
 
   @Test
+  public void getEvaluatedConditions_is_sorted() {
+    EvaluatedQualityGate underTest = builder
+      .setQualityGate(ALL_CONDITIONS_QUALITY_GATE)
+      .setStatus(randomStatus)
+      .addEvaluatedCondition(CONDITION_1, randomEvaluationStatus, randomValue)
+      .addEvaluatedCondition(CONDITION_2, randomEvaluationStatus, randomValue)
+      .addEvaluatedCondition(CONDITION_3, randomEvaluationStatus, randomValue)
+      .build();
+
+    assertThat(underTest.getQualityGate()).isEqualTo(ALL_CONDITIONS_QUALITY_GATE);
+    assertThat(underTest.getStatus()).isEqualTo(randomStatus);
+    assertThat(underTest.getEvaluatedConditions()).extracting(c -> c.getCondition().getMetricKey())
+      .contains(CONDITION_3.getMetricKey(), CONDITION_2.getMetricKey(), CONDITION_1.getMetricKey());
+  }
+
+  @Test
   public void verify_getters() {
     EvaluatedQualityGate underTest = builder
       .setQualityGate(ONE_CONDITION_QUALITY_GATE)
       .setStatus(randomStatus)
-      .addCondition(CONDITION_1, randomEvaluationStatus, randomValue)
+      .addEvaluatedCondition(CONDITION_1, randomEvaluationStatus, randomValue)
       .build();
 
     assertThat(underTest.getQualityGate()).isEqualTo(ONE_CONDITION_QUALITY_GATE);
@@ -145,8 +168,8 @@ public class EvaluatedQualityGateTest {
     EvaluatedQualityGate underTest = builder
       .setQualityGate(qualityGate)
       .setStatus(randomStatus)
-      .addCondition(CONDITION_1, randomEvaluationStatus, randomValue)
-      .addCondition(CONDITION_2, EvaluatedCondition.EvaluationStatus.ERROR, "bad")
+      .addEvaluatedCondition(CONDITION_1, randomEvaluationStatus, randomValue)
+      .addEvaluatedCondition(CONDITION_2, EvaluatedCondition.EvaluationStatus.ERROR, "bad")
       .build();
 
     assertThat(underTest.getQualityGate()).isEqualTo(qualityGate);
@@ -161,7 +184,7 @@ public class EvaluatedQualityGateTest {
     EvaluatedQualityGate.Builder builder = this.builder
       .setQualityGate(ONE_CONDITION_QUALITY_GATE)
       .setStatus(Level.ERROR)
-      .addCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.ERROR, "foo");
+      .addEvaluatedCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.ERROR, "foo");
 
     EvaluatedQualityGate underTest = builder.build();
     assertThat(underTest).isEqualTo(builder.build());
@@ -173,7 +196,7 @@ public class EvaluatedQualityGateTest {
     assertThat(underTest).isNotEqualTo(newBuilder()
       .setQualityGate(ONE_CONDITION_QUALITY_GATE)
       .setStatus(Level.ERROR)
-      .addCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.OK, "foo")
+      .addEvaluatedCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.OK, "foo")
       .build());
   }
 
@@ -182,7 +205,7 @@ public class EvaluatedQualityGateTest {
     EvaluatedQualityGate.Builder builder = this.builder
       .setQualityGate(ONE_CONDITION_QUALITY_GATE)
       .setStatus(Level.ERROR)
-      .addCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.ERROR, "foo");
+      .addEvaluatedCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.ERROR, "foo");
 
     EvaluatedQualityGate underTest = builder.build();
     assertThat(underTest.hashCode()).isEqualTo(builder.build().hashCode());
@@ -194,7 +217,7 @@ public class EvaluatedQualityGateTest {
     assertThat(underTest.hashCode()).isNotEqualTo(newBuilder()
       .setQualityGate(ONE_CONDITION_QUALITY_GATE)
       .setStatus(Level.ERROR)
-      .addCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.OK, "foo")
+      .addEvaluatedCondition(CONDITION_1, EvaluatedCondition.EvaluationStatus.OK, "foo")
       .build().hashCode());
   }
 }
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/QualityGateEvaluatorImplTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/QualityGateEvaluatorImplTest.java
index e29af3c6fdc..ab1a5bfd9b6 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/QualityGateEvaluatorImplTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/qualitygate/QualityGateEvaluatorImplTest.java
@@ -23,6 +23,7 @@ import com.google.common.collect.ImmutableSet;
 import java.util.Set;
 import java.util.stream.Collectors;
 import org.junit.Test;
+import org.sonar.api.measures.CoreMetrics;
 import org.sonar.api.measures.Metric;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -55,6 +56,23 @@ public class QualityGateEvaluatorImplTest {
   }
 
   @Test
+  public void evaluated_conditions_are_sorted() {
+    Set<String> metricKeys = ImmutableSet.of("foo", "bar", CoreMetrics.NEW_MAINTAINABILITY_RATING_KEY);
+    Set<Condition> conditions = metricKeys.stream().map(key -> {
+      Condition condition = mock(Condition.class);
+      when(condition.getMetricKey()).thenReturn(key);
+      return condition;
+    }).collect(Collectors.toSet());
+
+    QualityGate gate = mock(QualityGate.class);
+    when(gate.getConditions()).thenReturn(conditions);
+    QualityGateEvaluator.Measures measures = mock(QualityGateEvaluator.Measures.class);
+
+    assertThat(underTest.evaluate(gate, measures).getEvaluatedConditions()).extracting(x -> x.getCondition().getMetricKey())
+    .containsExactly(CoreMetrics.NEW_MAINTAINABILITY_RATING_KEY, "bar", "foo");
+  }
+
+  @Test
   public void evaluate_is_OK_for_empty_qgate() {
     QualityGate gate = mock(QualityGate.class);
     QualityGateEvaluator.Measures measures = mock(QualityGateEvaluator.Measures.class);
diff --git a/server/sonar-server-common/src/test/java/org/sonar/server/webhook/WebhookPayloadFactoryImplTest.java b/server/sonar-server-common/src/test/java/org/sonar/server/webhook/WebhookPayloadFactoryImplTest.java
index 04ed254c9f3..8dd178a31ad 100644
--- a/server/sonar-server-common/src/test/java/org/sonar/server/webhook/WebhookPayloadFactoryImplTest.java
+++ b/server/sonar-server-common/src/test/java/org/sonar/server/webhook/WebhookPayloadFactoryImplTest.java
@@ -61,7 +61,7 @@ public class WebhookPayloadFactoryImplTest {
     EvaluatedQualityGate gate = EvaluatedQualityGate.newBuilder()
       .setQualityGate(new QualityGate("G1", "Gate One", singleton(condition)))
       .setStatus(Metric.Level.ERROR)
-      .addCondition(condition, EvaluatedCondition.EvaluationStatus.ERROR, "74.0")
+      .addEvaluatedCondition(condition, EvaluatedCondition.EvaluationStatus.ERROR, "74.0")
       .build();
     ProjectAnalysis analysis = newAnalysis(task, gate, null, 1_500_000_000_000L, emptyMap());
 
@@ -105,7 +105,7 @@ public class WebhookPayloadFactoryImplTest {
     EvaluatedQualityGate gate = EvaluatedQualityGate.newBuilder()
       .setQualityGate(new QualityGate("G1", "Gate One", singleton(condition)))
       .setStatus(Metric.Level.ERROR)
-      .addCondition(condition, EvaluatedCondition.EvaluationStatus.NO_VALUE, null)
+      .addEvaluatedCondition(condition, EvaluatedCondition.EvaluationStatus.NO_VALUE, null)
       .build();
     ProjectAnalysis analysis = newAnalysis(task, gate, null, 1_500_000_000_000L, emptyMap());