CodeOwnersPluginConfigSnapshotTest: Fix flaky tests (second attempt)
The tests were flaky because the order of elements in sets is not fixed.
The first attempt to fix this was change Id758c5e4d which sorted the
override approvals in the tests before asserting them. However this
change missed to adapt one test.
Since doing the sorting in each test is error-prone and repetitive we
now do the sorting in
CodeOwnersPluginConfigSnapshot#getOverrideApprovals() and return an
ImmutableSortedSet. This way every caller can rely on the sorting.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: I38415ec45ea309b42fc5e7ed1cd98ca4c330fdd6
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
index 04046ac..df0d418 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshot.java
@@ -20,6 +20,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.ImmutableSetMultimap;
+import com.google.common.collect.ImmutableSortedSet;
import com.google.common.collect.Iterables;
import com.google.common.flogger.FluentLogger;
import com.google.gerrit.common.Nullable;
@@ -40,6 +41,7 @@
import com.google.inject.Inject;
import com.google.inject.assistedinject.Assisted;
import java.io.IOException;
+import java.util.Collection;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
@@ -433,13 +435,17 @@
*
* <p>The first override approval configuration that exists counts and the evaluation is stopped.
*
+ * <p>The returned override approvals are sorted alphabetically by their string representation
+ * (e.g. {@code Owners-Override+1}).
+ *
* @return the override approvals that should be used, an empty set if no override approval is
* configured, in this case the override functionality is disabled
*/
- public ImmutableSet<RequiredApproval> getOverrideApprovals() {
+ public ImmutableSortedSet<RequiredApproval> getOverrideApprovals() {
try {
- return filterOutDuplicateRequiredApprovals(
- getConfiguredRequiredApproval(overrideApprovalConfig));
+ return ImmutableSortedSet.copyOf(
+ filterOutDuplicateRequiredApprovals(
+ getConfiguredRequiredApproval(overrideApprovalConfig)));
} catch (InvalidPluginConfigurationException e) {
logger.atWarning().withCause(e).log(
"Ignoring invalid override approval configuration for project %s."
@@ -447,7 +453,7 @@
projectName.get());
}
- return ImmutableSet.of();
+ return ImmutableSortedSet.of();
}
/**
@@ -462,7 +468,7 @@
* "Code-Review" approvals >= 1)
* </ul>
*/
- private ImmutableSet<RequiredApproval> filterOutDuplicateRequiredApprovals(
+ private Collection<RequiredApproval> filterOutDuplicateRequiredApprovals(
ImmutableList<RequiredApproval> requiredApprovals) {
Map<String, RequiredApproval> requiredApprovalsByLabel = new HashMap<>();
for (RequiredApproval requiredApproval : requiredApprovals) {
@@ -474,7 +480,7 @@
}
requiredApprovalsByLabel.put(labelName, requiredApproval);
}
- return ImmutableSet.copyOf(requiredApprovalsByLabel.values());
+ return requiredApprovalsByLabel.values();
}
/**
diff --git a/java/com/google/gerrit/plugins/codeowners/backend/config/RequiredApproval.java b/java/com/google/gerrit/plugins/codeowners/backend/config/RequiredApproval.java
index f71df6a..4cbdba0 100644
--- a/java/com/google/gerrit/plugins/codeowners/backend/config/RequiredApproval.java
+++ b/java/com/google/gerrit/plugins/codeowners/backend/config/RequiredApproval.java
@@ -20,6 +20,7 @@
import com.google.auto.value.AutoValue;
import com.google.common.annotations.VisibleForTesting;
+import com.google.common.collect.ComparisonChain;
import com.google.common.primitives.Ints;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.entities.PatchSetApproval;
@@ -39,7 +40,7 @@
* </ul>
*/
@AutoValue
-public abstract class RequiredApproval {
+public abstract class RequiredApproval implements Comparable<RequiredApproval> {
/** The label on which an approval is required. */
public abstract LabelType labelType();
@@ -59,6 +60,11 @@
}
@Override
+ public final int compareTo(RequiredApproval other) {
+ return ComparisonChain.start().compare(toString(), other.toString()).result();
+ }
+
+ @Override
public final String toString() {
return labelType().getName() + "+" + value();
}
diff --git a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshotTest.java b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshotTest.java
index 6f40d91..63679e0 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshotTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/backend/config/CodeOwnersPluginConfigSnapshotTest.java
@@ -20,11 +20,11 @@
import static com.google.gerrit.plugins.codeowners.testing.RequiredApprovalSubject.assertThat;
import static com.google.gerrit.testing.GerritJUnit.assertThrows;
import static com.google.gerrit.truth.OptionalSubject.assertThat;
-import static java.util.Comparator.comparing;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
import com.google.gerrit.acceptance.TestAccount;
import com.google.gerrit.acceptance.config.GerritConfig;
import com.google.gerrit.acceptance.testsuite.project.ProjectOperations;
@@ -1280,9 +1280,7 @@
createOwnersOverrideLabel("Other-Override");
configureOverrideApproval(project, "Other-Override+1");
- ImmutableList<RequiredApproval> requiredApprovals =
- ImmutableList.sortedCopyOf(
- comparing(RequiredApproval::toString), cfgSnapshot().getOverrideApprovals());
+ ImmutableSortedSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
assertThat(requiredApprovals).hasSize(2);
assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Other-Override");
assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
@@ -1323,11 +1321,11 @@
createOwnersOverrideLabel("Other-Override");
configureOverrideApproval(allProjects, "Other-Override+1");
- ImmutableSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
+ ImmutableSortedSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
assertThat(requiredApprovals).hasSize(2);
- assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Owners-Override");
+ assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Other-Override");
assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
- assertThat(requiredApprovals).element(1).hasLabelNameThat().isEqualTo("Other-Override");
+ assertThat(requiredApprovals).element(1).hasLabelNameThat().isEqualTo("Owners-Override");
assertThat(requiredApprovals).element(1).hasValueThat().isEqualTo(1);
}
@@ -1352,9 +1350,7 @@
configureOverrideApproval(allProjects, "Owners-Override+1");
configureOverrideApproval(project, "Other-Override+1");
- ImmutableList<RequiredApproval> requiredApprovals =
- ImmutableList.sortedCopyOf(
- comparing(RequiredApproval::toString), cfgSnapshot().getOverrideApprovals());
+ ImmutableSortedSet<RequiredApproval> requiredApprovals = cfgSnapshot().getOverrideApprovals();
assertThat(requiredApprovals).hasSize(2);
assertThat(requiredApprovals).element(0).hasLabelNameThat().isEqualTo("Other-Override");
assertThat(requiredApprovals).element(0).hasValueThat().isEqualTo(1);
diff --git a/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java b/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
index bec1973..3e9b614 100644
--- a/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
+++ b/javatests/com/google/gerrit/plugins/codeowners/restapi/CodeOwnerProjectConfigJsonTest.java
@@ -22,7 +22,7 @@
import static org.mockito.Mockito.when;
import com.google.common.collect.ImmutableList;
-import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.ImmutableSortedSet;
import com.google.gerrit.entities.BranchNameKey;
import com.google.gerrit.entities.LabelType;
import com.google.gerrit.plugins.codeowners.acceptance.AbstractCodeOwnersTest;
@@ -173,7 +173,7 @@
.thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2));
when(codeOwnersPluginConfigSnapshot.getOverrideApprovals())
.thenReturn(
- ImmutableSet.of(
+ ImmutableSortedSet.of(
RequiredApproval.create(
LabelType.withDefaultValues("Owners-Override"), (short) 1)));
when(codeOwnersPluginConfiguration.getProjectConfig(project))
@@ -248,7 +248,7 @@
when(codeOwnersPluginConfigSnapshot.getOverrideApprovals())
.thenReturn(
- ImmutableSet.of(
+ ImmutableSortedSet.of(
RequiredApproval.create(LabelType.withDefaultValues("Owners-Override"), (short) 1),
RequiredApproval.create(LabelType.withDefaultValues("Code-Review"), (short) 2)));
when(codeOwnersPluginConfiguration.getProjectConfig(project))
@@ -292,7 +292,7 @@
.thenReturn(RequiredApproval.create(getDefaultCodeReviewLabel(), (short) 2));
when(codeOwnersPluginConfigSnapshot.getOverrideApprovals())
.thenReturn(
- ImmutableSet.of(
+ ImmutableSortedSet.of(
RequiredApproval.create(
LabelType.withDefaultValues("Owners-Override"), (short) 1)));
when(codeOwnersPluginConfiguration.getProjectConfig(project))
