From 821c44c440706e769b3d41c8ffce25282a72f3fb Mon Sep 17 00:00:00 2001
From: Rabi Panda <adnapibar@gmail.com>
Date: Fri, 23 Apr 2021 07:31:41 -0700
Subject: [PATCH] Fix failing test caused by versioning change. (#598)

The change in 0ba0e7cc, introduced the issue where randomly selecting an incompatible version fails the test. It caused the filtering logic to incorrectly identify all ES 7.*.* versions as bad versions for joining which should not be the case.

Additionally, split the test into two separate tests where earlier only one of them was run at random.

Signed-off-by: Rabi Panda <adnapibar@gmail.com>
---
 .../src/main/java/org/opensearch/Version.java |  7 ++++++
 .../java/org/opensearch/VersionTests.java     |  4 ++++
 .../zen/NodeJoinControllerTests.java          | 22 +++++++++++--------
 3 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/server/src/main/java/org/opensearch/Version.java b/server/src/main/java/org/opensearch/Version.java
index 0da65fd1ef8..b86426c5736 100644
--- a/server/src/main/java/org/opensearch/Version.java
+++ b/server/src/main/java/org/opensearch/Version.java
@@ -234,6 +234,13 @@ public class Version implements Comparable<Version>, ToXContentFragment {
         return version.id >= id;
     }
 
+    // LegacyESVersion major 7 is equivalent to Version major 1
+    public int compareMajor(Version other) {
+        int m = major == 1 ? 7 : major;
+        int om = other.major == 1 ? 7 : other.major;
+        return Integer.compare(m, om);
+    }
+
     @Override
     public int compareTo(Version other) {
         return Integer.compare(this.id, other.id);
diff --git a/server/src/test/java/org/opensearch/VersionTests.java b/server/src/test/java/org/opensearch/VersionTests.java
index 34d9a7aa9ac..36bdbd2e1b7 100644
--- a/server/src/test/java/org/opensearch/VersionTests.java
+++ b/server/src/test/java/org/opensearch/VersionTests.java
@@ -88,6 +88,10 @@ public class VersionTests extends OpenSearchTestCase {
         assertThat(V_6_3_0, is(lessThan(V_7_0_0)));
         assertThat(V_6_3_0.compareTo(V_6_3_0), is(0));
         assertThat(V_7_0_0, is(greaterThan(V_6_3_0)));
+
+        assertThat(Version.V_1_0_0.compareMajor(LegacyESVersion.V_7_0_0), is(0));
+        assertThat(Version.V_1_0_0.compareMajor(LegacyESVersion.V_6_3_0), is(1));
+        assertThat(LegacyESVersion.V_6_3_0.compareMajor(Version.V_1_0_0), is(-1));
     }
 
     public void testMin() {
diff --git a/server/src/test/java/org/opensearch/discovery/zen/NodeJoinControllerTests.java b/server/src/test/java/org/opensearch/discovery/zen/NodeJoinControllerTests.java
index 74918e0e82e..1f8759e57a7 100644
--- a/server/src/test/java/org/opensearch/discovery/zen/NodeJoinControllerTests.java
+++ b/server/src/test/java/org/opensearch/discovery/zen/NodeJoinControllerTests.java
@@ -600,20 +600,24 @@ public class NodeJoinControllerTests extends OpenSearchTestCase {
         assertThat(e.getMessage(), containsString("found existing node"));
     }
 
-    @AwaitsFix(bugUrl = "https://github.com/opensearch-project/OpenSearch/issues/577")
-    public void testRejectingJoinWithIncompatibleVersion() throws InterruptedException, ExecutionException {
+    public void testRejectingJoinWithBeforeMinCompatibleVersion() throws ExecutionException, InterruptedException {
+        final Version badVersion = getPreviousVersion(Version.CURRENT.minimumCompatibilityVersion());
+        assertRejectingJoinWithIncompatibleVersion(badVersion);
+    }
+
+    public void testRejectingJoinWithPreviousMajorVersion() throws ExecutionException, InterruptedException {
+        final Version badVersion =
+            randomFrom(allVersions().stream().filter(v -> v.compareMajor(Version.CURRENT) < 0).collect(Collectors.toList()));
+        assertRejectingJoinWithIncompatibleVersion(badVersion);
+    }
+
+    private void assertRejectingJoinWithIncompatibleVersion(final Version badVersion) throws InterruptedException, ExecutionException {
         addNodes(randomInt(5));
-        final Version badVersion;
-        if (randomBoolean()) {
-            badVersion = getPreviousVersion(Version.CURRENT.minimumCompatibilityVersion());
-        } else {
-            badVersion = randomFrom(allVersions().stream().filter(v -> v.before(Version.CURRENT)).collect(Collectors.toList()));
-        }
         final DiscoveryNode badNode = new DiscoveryNode("badNode", buildNewFakeTransportAddress(), emptyMap(),
             new HashSet<>(randomSubsetOf(DiscoveryNodeRole.BUILT_IN_ROLES)), badVersion);
 
         final Version goodVersion =
-            randomFrom(allVersions().stream().filter(v -> v.onOrAfter(Version.CURRENT)).collect(Collectors.toList()));
+            randomFrom(allVersions().stream().filter(v -> v.compareMajor(Version.CURRENT) >= 0).collect(Collectors.toList()));
         final DiscoveryNode goodNode = new DiscoveryNode("goodNode", buildNewFakeTransportAddress(), emptyMap(),
             new HashSet<>(randomSubsetOf(DiscoveryNodeRole.BUILT_IN_ROLES)), goodVersion);