fix(execution): Fix START_TIME_OR_ID comparator (spinnaker#3392)

* test(execution): Add tests to START_TIME_OR_ID

The next commit will fix a bug in the START_TIME_OR_ID comparator;
add tests to document the current behavior and ensure that the
bug fix only affects the intended case.

* fix(execution): Fix START_TIME_OR_ID comparator

I noticed that the START_TIME_OR_ID comparator violates the general
contract of Comparator in the case where one of hte executions has a
null start time. I haven't noticed this causing actual bugs, but I
think the failure mode would be intermittent cases of
"Comparison method violates its general contract" being thrown
when trying to sort based on this comparator, which would be annoying
and hard to debug, so fixing this now.

* fix(execution): Fix double-null case in sorting

The prior commit did not properly handle the case where we compare
two executions with null startTime; instead of calling them equal,
we should compare by id.

This was caught by a test in orca-redis; while it was broken before
this change I guess the test was working even in that broken state.

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

orca-core/src/main/java/com/netflix/spinnaker/orca/pipeline/persistence/ExecutionRepository.java

@@ -257,14 +257,15 @@ public int compare(Execution a, Execution b) {
         Long aStartTime = a.getStartTime();
         Long bStartTime = b.getStartTime();
 
+        int startCompare;
         if (aStartTime == null) {
-          return -1;
-        }
-        if (bStartTime == null) {
-          return 0;
+          startCompare = bStartTime == null ? 0 : -1;
+        } else if (bStartTime == null) {
+          startCompare = 1;
+        } else {
+          startCompare = bStartTime.compareTo(aStartTime);
         }
 
-        int startCompare = bStartTime.compareTo(aStartTime);
         if (startCompare == 0) {
           return b.getId().compareTo(a.getId());
         }

orca-core/src/test/java/com/netflix/spinnaker/orca/pipeline/persistence/ExecutionRepositoryTest.java

@@ -0,0 +1,101 @@
+/*
+ * Copyright 2020 Google, LLC
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License")
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package com.netflix.spinnaker.orca.pipeline.persistence;
+
+import static com.netflix.spinnaker.orca.pipeline.persistence.ExecutionRepository.ExecutionComparator.START_TIME_OR_ID;
+import static org.assertj.core.api.Assertions.assertThat;
+
+import com.google.common.collect.ImmutableList;
+import com.netflix.spinnaker.orca.pipeline.model.Execution;
+import org.junit.jupiter.api.Test;
+import org.junit.platform.runner.JUnitPlatform;
+import org.junit.runner.RunWith;
+
+@RunWith(JUnitPlatform.class)
+final class ExecutionRepositoryTest {
+  private static final String APPLICATION = "myapp";
+
+  private static final long EARLIER_TIME = 629510400000L;
+  private static final String EARLIER_ULID = "00JA8VWT00BNG9YKW1B9YEVX7Y";
+
+  private static final long LATER_TIME = 1576195200000L;
+  private static final String LATER_ULID_1 = "01DVY8W500KZ1D9TSSDM4D30A8";
+  private static final String LATER_ULID_2 = "01DVY8W500S4M3CXD2QJ7P2877";
+
+  @Test
+  void nullSortsFirst() {
+    Execution a = withStartTimeAndId(null, EARLIER_ULID);
+    Execution b = withStartTimeAndId(EARLIER_TIME, EARLIER_ULID);
+
+    assertThat(START_TIME_OR_ID.compare(a, a)).isEqualTo(0);
+    assertThat(START_TIME_OR_ID.compare(a, b)).isLessThan(0);
+    assertThat(START_TIME_OR_ID.compare(b, a)).isGreaterThan(0);
+  }
+
+  @Test
+  void nullStartTimesSortById() {
+    Execution a = withStartTimeAndId(null, LATER_ULID_1);
+    Execution b = withStartTimeAndId(null, EARLIER_ULID);
+
+    assertThat(START_TIME_OR_ID.compare(a, a)).isEqualTo(0);
+    assertThat(START_TIME_OR_ID.compare(a, b)).isLessThan(0);
+    assertThat(START_TIME_OR_ID.compare(b, a)).isGreaterThan(0);
+  }
+
+  @Test
+  void laterStartTimeSortsFirst() {
+    Execution a = withStartTimeAndId(LATER_TIME, EARLIER_ULID);
+    Execution b = withStartTimeAndId(EARLIER_TIME, EARLIER_ULID);
+
+    assertThat(START_TIME_OR_ID.compare(a, a)).isEqualTo(0);
+    assertThat(START_TIME_OR_ID.compare(a, b)).isLessThan(0);
+    assertThat(START_TIME_OR_ID.compare(b, a)).isGreaterThan(0);
+  }
+
+  @Test
+  void laterIdSortsFirst() {
+    Execution a = withStartTimeAndId(LATER_TIME, LATER_ULID_2);
+    Execution b = withStartTimeAndId(LATER_TIME, LATER_ULID_1);
+
+    assertThat(START_TIME_OR_ID.compare(a, a)).isEqualTo(0);
+    assertThat(START_TIME_OR_ID.compare(a, b)).isLessThan(0);
+    assertThat(START_TIME_OR_ID.compare(b, a)).isGreaterThan(0);
+  }
+
+  @Test
+  void fullSort() {
+    Execution nullStartTime = withStartTimeAndId(null, EARLIER_ULID);
+    Execution earlierStartTime = withStartTimeAndId(EARLIER_TIME, EARLIER_ULID);
+    Execution laterStartTimeEarlierId = withStartTimeAndId(LATER_TIME, LATER_ULID_1);
+    Execution laterStartTimeLaterId = withStartTimeAndId(LATER_TIME, LATER_ULID_2);
+
+    // The order of this list is not significant, only that it is some random non-sorted order
+    ImmutableList<Execution> executions =
+        ImmutableList.of(
+            laterStartTimeEarlierId, earlierStartTime, laterStartTimeLaterId, nullStartTime);
+
+    assertThat(ImmutableList.sortedCopyOf(START_TIME_OR_ID, executions))
+        .containsExactly(
+            nullStartTime, laterStartTimeLaterId, laterStartTimeEarlierId, earlierStartTime);
+  }
+
+  private Execution withStartTimeAndId(Long startTime, String id) {
+    Execution execution = new Execution(Execution.ExecutionType.PIPELINE, id, APPLICATION);
+    execution.setStartTime(startTime);
+    return execution;
+  }
+}