From b4abdc5eee8fe0e2221fbf1a0053fd032ffef5fe Mon Sep 17 00:00:00 2001 From: Ryan Ernst Date: Wed, 2 May 2018 09:29:35 -0700 Subject: [PATCH] Tests: Simplify VersionUtils released version splitting (#30322) This commit refactors VersionUtils.resolveReleasedVersions to be simpler, and in the process fixes the behavior to match that of VersionCollection.groovy. closes #30133 --- .../org/elasticsearch/test/VersionUtils.java | 134 ++++++++---------- .../elasticsearch/test/VersionUtilsTests.java | 48 ++++--- 2 files changed, 93 insertions(+), 89 deletions(-) diff --git a/test/framework/src/main/java/org/elasticsearch/test/VersionUtils.java b/test/framework/src/main/java/org/elasticsearch/test/VersionUtils.java index 9fde8b66a1f96..792f3fba123da 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/VersionUtils.java +++ b/test/framework/src/main/java/org/elasticsearch/test/VersionUtils.java @@ -19,26 +19,25 @@ package org.elasticsearch.test; -import org.elasticsearch.Version; -import org.elasticsearch.common.Booleans; -import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.collect.Tuple; - -import java.lang.reflect.Field; -import java.lang.reflect.Modifier; import java.util.ArrayList; -import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.Locale; +import java.util.Map; import java.util.Optional; import java.util.Random; +import java.util.SortedSet; +import java.util.TreeSet; import java.util.stream.Collectors; +import java.util.stream.Stream; -import static java.util.Collections.singletonList; -import static java.util.Collections.unmodifiableList; +import org.elasticsearch.Version; +import org.elasticsearch.common.Nullable; +import org.elasticsearch.common.collect.Tuple; /** Utilities for selecting versions in tests */ public class VersionUtils { + /** * Sort versions that have backwards compatibility guarantees from * those that don't. Doesn't actually check whether or not the versions @@ -50,73 +49,65 @@ public class VersionUtils { * guarantees in v1 and versions without the guranteees in v2 */ static Tuple, List> resolveReleasedVersions(Version current, Class versionClass) { - List versions = Version.getDeclaredVersions(versionClass); - - Version last = versions.remove(versions.size() - 1); - assert last.equals(current) : "The highest version must be the current one " - + "but was [" + versions.get(versions.size() - 1) + "] and current was [" + current + "]"; - - if (current.revision != 0) { - /* If we are in a stable branch there should be no unreleased version constants - * because we don't expect to release any new versions in older branches. If there - * are extra constants then gradle will yell about it. */ - return new Tuple<>(unmodifiableList(versions), singletonList(current)); + // group versions into major version + Map> majorVersions = Version.getDeclaredVersions(versionClass).stream() + .collect(Collectors.groupingBy(v -> (int)v.major)); + // this breaks b/c 5.x is still in version list but master doesn't care about it! + //assert majorVersions.size() == 2; + // TODO: remove oldVersions, we should only ever have 2 majors in Version + List oldVersions = majorVersions.getOrDefault((int)current.major - 2, Collections.emptyList()); + List> previousMajor = splitByMinor(majorVersions.get((int)current.major - 1)); + List> currentMajor = splitByMinor(majorVersions.get((int)current.major)); + + List unreleasedVersions = new ArrayList<>(); + final List> stableVersions; + if (currentMajor.size() == 1) { + // on master branch + stableVersions = previousMajor; + // remove current + moveLastToUnreleased(currentMajor, unreleasedVersions); + } else { + // on a stable or release branch, ie N.x + stableVersions = currentMajor; + // remove the next maintenance bugfix + moveLastToUnreleased(previousMajor, unreleasedVersions); } - /* If we are on a patch release then we know that at least the version before the - * current one is unreleased. If it is released then gradle would be complaining. */ - int unreleasedIndex = versions.size() - 1; - while (true) { - if (unreleasedIndex < 0) { - throw new IllegalArgumentException("Couldn't find first non-alpha release"); - } - /* We don't support backwards compatibility for alphas, betas, and rcs. But - * they were released so we add them to the released list. Usually this doesn't - * matter to consumers, but consumers that do care should filter non-release - * versions. */ - if (versions.get(unreleasedIndex).isRelease()) { - break; + // remove next minor + Version lastMinor = moveLastToUnreleased(stableVersions, unreleasedVersions); + if (lastMinor.revision == 0) { + if (stableVersions.get(stableVersions.size() - 1).size() == 1) { + // a minor is being staged, which is also unreleased + moveLastToUnreleased(stableVersions, unreleasedVersions); } - unreleasedIndex--; + // remove the next bugfix + moveLastToUnreleased(stableVersions, unreleasedVersions); } - Version unreleased = versions.remove(unreleasedIndex); - if (unreleased.revision == 0) { - /* - * If the last unreleased version is itself a patch release then Gradle enforces that there is yet another unreleased version - * before that. However, we have to skip alpha/betas/RCs too (e.g., consider when the version constants are ..., 5.6.3, 5.6.4, - * 6.0.0-alpha1, ..., 6.0.0-rc1, 6.0.0-rc2, 6.0.0, 6.1.0 on the 6.x branch. In this case, we will have pruned 6.0.0 and 6.1.0 as - * unreleased versions, but we also need to prune 5.6.4. At this point though, unreleasedIndex will be pointing to 6.0.0-rc2, so - * we have to skip backwards until we find a non-alpha/beta/RC again. Then we can prune that version as an unreleased version - * too. - */ - do { - unreleasedIndex--; - } while (versions.get(unreleasedIndex).isRelease() == false); - Version earlierUnreleased = versions.remove(unreleasedIndex); - - // This earlierUnreleased is either the snapshot on the minor branch lower, or its possible its a staged release. If it is a - // staged release, remove it and return it in unreleased as well. - if (earlierUnreleased.revision == 0) { - unreleasedIndex--; - Version actualUnreleasedPreviousMinor = versions.remove(unreleasedIndex); - return new Tuple<>(unmodifiableList(versions), unmodifiableList(Arrays.asList(actualUnreleasedPreviousMinor, - earlierUnreleased, unreleased, current))); - } + List releasedVersions = Stream.concat(oldVersions.stream(), + Stream.concat(previousMajor.stream(), currentMajor.stream()).flatMap(List::stream)) + .collect(Collectors.toList()); + Collections.sort(unreleasedVersions); // we add unreleased out of order, so need to sort here + return new Tuple<>(Collections.unmodifiableList(releasedVersions), Collections.unmodifiableList(unreleasedVersions)); + } - return new Tuple<>(unmodifiableList(versions), unmodifiableList(Arrays.asList(earlierUnreleased, unreleased, current))); - } else if (unreleased.major == current.major) { - // need to remove one more of the last major's minor set - do { - unreleasedIndex--; - } while (unreleasedIndex > 0 && versions.get(unreleasedIndex).major == current.major); - if (unreleasedIndex > 0) { - // some of the test cases return very small lists, so its possible this is just the end of the list, if so, dont include it - Version earlierMajorsMinor = versions.remove(unreleasedIndex); - return new Tuple<>(unmodifiableList(versions), unmodifiableList(Arrays.asList(earlierMajorsMinor, unreleased, current))); - } + // split the given versions into sub lists grouped by minor version + private static List> splitByMinor(List versions) { + Map> byMinor = versions.stream().collect(Collectors.groupingBy(v -> (int)v.minor)); + return byMinor.entrySet().stream().sorted(Map.Entry.comparingByKey()).map(Map.Entry::getValue).collect(Collectors.toList()); + } + + // move the last version of the last minor in versions to the unreleased versions + private static Version moveLastToUnreleased(List> versions, List unreleasedVersions) { + List lastMinor = new ArrayList<>(versions.get(versions.size() - 1)); + Version lastVersion = lastMinor.remove(lastMinor.size() - 1); + if (lastMinor.isEmpty()) { + versions.remove(versions.size() - 1); + } else { + versions.set(versions.size() - 1, lastMinor); } - return new Tuple<>(unmodifiableList(versions), unmodifiableList(Arrays.asList(unreleased, current))); + unreleasedVersions.add(lastVersion); + return lastVersion; } private static final List RELEASED_VERSIONS; @@ -131,7 +122,7 @@ static Tuple, List> resolveReleasedVersions(Version curre allVersions.addAll(RELEASED_VERSIONS); allVersions.addAll(UNRELEASED_VERSIONS); Collections.sort(allVersions); - ALL_VERSIONS = unmodifiableList(allVersions); + ALL_VERSIONS = Collections.unmodifiableList(allVersions); } /** @@ -244,5 +235,4 @@ public static Version maxCompatibleVersion(Version version) { assert compatible.size() > 0; return compatible.get(compatible.size() - 1); } - } diff --git a/test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java b/test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java index f6ddb48aaaf2c..e1807f44174ea 100644 --- a/test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java +++ b/test/framework/src/test/java/org/elasticsearch/test/VersionUtilsTests.java @@ -42,6 +42,7 @@ public void testAllVersionsSorted() { } public void testRandomVersionBetween() { + // TODO: rework this test to use a dummy Version class so these don't need to change with each release // full range Version got = VersionUtils.randomVersionBetween(random(), VersionUtils.getFirstVersion(), Version.CURRENT); assertTrue(got.onOrAfter(VersionUtils.getFirstVersion())); @@ -54,10 +55,10 @@ public void testRandomVersionBetween() { assertTrue(got.onOrBefore(Version.CURRENT)); // sub range - got = VersionUtils.randomVersionBetween(random(), Version.V_5_0_0, - Version.V_6_0_0_beta1); - assertTrue(got.onOrAfter(Version.V_5_0_0)); - assertTrue(got.onOrBefore(Version.V_6_0_0_beta1)); + got = VersionUtils.randomVersionBetween(random(), Version.V_6_0_0_alpha1, + Version.V_6_2_4); + assertTrue(got.onOrAfter(Version.V_6_0_0_alpha1)); + assertTrue(got.onOrBefore(Version.V_6_2_4)); // unbounded lower got = VersionUtils.randomVersionBetween(random(), null, Version.V_6_0_0_beta1); @@ -68,8 +69,8 @@ public void testRandomVersionBetween() { assertTrue(got.onOrBefore(VersionUtils.allReleasedVersions().get(0))); // unbounded upper - got = VersionUtils.randomVersionBetween(random(), Version.V_5_0_0, null); - assertTrue(got.onOrAfter(Version.V_5_0_0)); + got = VersionUtils.randomVersionBetween(random(), Version.V_6_0_0, null); + assertTrue(got.onOrAfter(Version.V_6_0_0)); assertTrue(got.onOrBefore(Version.CURRENT)); got = VersionUtils.randomVersionBetween(random(), VersionUtils.getPreviousVersion(), null); assertTrue(got.onOrAfter(VersionUtils.getPreviousVersion())); @@ -100,6 +101,8 @@ public void testRandomVersionBetween() { } public static class TestReleaseBranch { + public static final Version V_4_0_0 = Version.fromString("4.0.0"); + public static final Version V_4_0_1 = Version.fromString("4.0.1"); public static final Version V_5_3_0 = Version.fromString("5.3.0"); public static final Version V_5_3_1 = Version.fromString("5.3.1"); public static final Version V_5_3_2 = Version.fromString("5.3.2"); @@ -113,19 +116,24 @@ public void testResolveReleasedVersionsForReleaseBranch() { List unreleased = t.v2(); assertThat(released, equalTo(Arrays.asList( + TestReleaseBranch.V_4_0_0, TestReleaseBranch.V_5_3_0, TestReleaseBranch.V_5_3_1, TestReleaseBranch.V_5_3_2, TestReleaseBranch.V_5_4_0))); - assertThat(unreleased, equalTo(Collections.singletonList(TestReleaseBranch.V_5_4_1))); + assertThat(unreleased, equalTo(Arrays.asList( + TestReleaseBranch.V_4_0_1, + TestReleaseBranch.V_5_4_1))); } public static class TestStableBranch { - public static final Version V_5_3_0 = Version.fromString("5.3.0"); - public static final Version V_5_3_1 = Version.fromString("5.3.1"); - public static final Version V_5_3_2 = Version.fromString("5.3.2"); - public static final Version V_5_4_0 = Version.fromString("5.4.0"); - public static final Version CURRENT = V_5_4_0; + public static final Version V_4_0_0 = Version.fromString("4.0.0"); + public static final Version V_4_0_1 = Version.fromString("4.0.1"); + public static final Version V_5_0_0 = Version.fromString("5.0.0"); + public static final Version V_5_0_1 = Version.fromString("5.0.1"); + public static final Version V_5_0_2 = Version.fromString("5.0.2"); + public static final Version V_5_1_0 = Version.fromString("5.1.0"); + public static final Version CURRENT = V_5_1_0; } public void testResolveReleasedVersionsForUnreleasedStableBranch() { Tuple, List> t = VersionUtils.resolveReleasedVersions(TestStableBranch.CURRENT, @@ -134,14 +142,18 @@ public void testResolveReleasedVersionsForUnreleasedStableBranch() { List unreleased = t.v2(); assertThat(released, equalTo(Arrays.asList( - TestStableBranch.V_5_3_0, - TestStableBranch.V_5_3_1))); + TestStableBranch.V_4_0_0, + TestStableBranch.V_5_0_0, + TestStableBranch.V_5_0_1))); assertThat(unreleased, equalTo(Arrays.asList( - TestStableBranch.V_5_3_2, - TestStableBranch.V_5_4_0))); + TestStableBranch.V_4_0_1, + TestStableBranch.V_5_0_2, + TestStableBranch.V_5_1_0))); } public static class TestStableBranchBehindStableBranch { + public static final Version V_4_0_0 = Version.fromString("4.0.0"); + public static final Version V_4_0_1 = Version.fromString("4.0.1"); public static final Version V_5_3_0 = Version.fromString("5.3.0"); public static final Version V_5_3_1 = Version.fromString("5.3.1"); public static final Version V_5_3_2 = Version.fromString("5.3.2"); @@ -156,9 +168,11 @@ public void testResolveReleasedVersionsForStableBranchBehindStableBranch() { List unreleased = t.v2(); assertThat(released, equalTo(Arrays.asList( + TestStableBranchBehindStableBranch.V_4_0_0, TestStableBranchBehindStableBranch.V_5_3_0, TestStableBranchBehindStableBranch.V_5_3_1))); assertThat(unreleased, equalTo(Arrays.asList( + TestStableBranchBehindStableBranch.V_4_0_1, TestStableBranchBehindStableBranch.V_5_3_2, TestStableBranchBehindStableBranch.V_5_4_0, TestStableBranchBehindStableBranch.V_5_5_0))); @@ -214,13 +228,13 @@ public void testResolveReleasedVersionsAtNewMajorRelease() { assertThat(released, equalTo(Arrays.asList( TestNewMajorRelease.V_5_6_0, TestNewMajorRelease.V_5_6_1, - TestNewMajorRelease.V_5_6_2, TestNewMajorRelease.V_6_0_0_alpha1, TestNewMajorRelease.V_6_0_0_alpha2, TestNewMajorRelease.V_6_0_0_beta1, TestNewMajorRelease.V_6_0_0_beta2, TestNewMajorRelease.V_6_0_0))); assertThat(unreleased, equalTo(Arrays.asList( + TestNewMajorRelease.V_5_6_2, TestNewMajorRelease.V_6_0_1))); }