Allow skipping ranges of versions by pgomulka · Pull Request #50014 · elastic/elasticsearch

pgomulka · 2019-12-10T09:09:03Z

Multiple version ranges are allowed to be used in section skip in yml
tests. This is useful when a bugfix was backported to latest versions
and all previous releases contain a wire breaking bug.
examples:
6.1.0 - 6.3.0, 6.6.0 - 6.7.9, 7.0 -
- 7.2, 8.0.0 -

Multiple version ranges are allowed to be used in section skip in yml tests. This is useful when a bugfix was backported to latest versions and all previous releases contain a wire breaking bug. examples: 6.1.0 - 6.3.0, 6.6.0 - 6.7.9, 7.0 - - 7.2, 8.0.0 -

elasticmachine · 2019-12-10T09:09:06Z

Pinging @elastic/es-core-infra (:Core/Infra/Build)

pugnascotia · 2019-12-10T09:26:24Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java

-        this.lowerVersion = versions[0];
-        this.upperVersion = versions[1];
+        this.versionRanges = parseVersionRanges(versionRange);
+        assert versionRanges.size() >= 1;


Would this be clearer?

Suggested change

assert versionRanges.size() >= 1;

assert versionRanges.isEmpty() == false;

pugnascotia · 2019-12-10T09:31:27Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java

-            && currentVersion.onOrBefore(upperVersion);
+        boolean skip = false;
+        for (VersionRange versionRange : versionRanges) {
+            skip |= versionRange.contain(currentVersion);


What about:

boolean skip = versionRanges.stream().anyMatch(range -> range.contain(currentVersion)); return skip || Features.areAllSupported(features) == false;

pugnascotia · 2019-12-10T09:35:54Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java

+        String[] ranges = rawRanges.split(",");
+        List<VersionRange> versionRanges = new ArrayList<>();
+        for (String rawRange : ranges) {
+            String[] skipVersions = wrapWithSpaces(rawRange).split("-");


I don't understand why wrapWithSpaces is necessary here?

Ah, it's to ensure you get two elements in the skipVersions array. That's a little subtle.

yes, definitely not needed.

@pugnascotia ok so I see why I put that there. I wanted to allow formats like
8.0.0 - where there is no additional space. Also didn't want to make it a special case.
how about using "8.0.0 -".split("-",-1) ? -1 there means that the length of array element can have any element. With 0 it means that trailing empty element will not be returned

pugnascotia · 2019-12-10T09:39:13Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/VersionRange.java

+        return upper;
+    }
+
+    public boolean contain(Version currentVersion) {


...I don't suppose you'd rename this to contains? It's just that I'm so used to Collection.contains 😅

it is a good idea, will do

pugnascotia · 2019-12-10T10:01:41Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/VersionRange.java

+        this.upper = upper;
+    }
+
+    public Version lower() {


Do we not usually use bean-style naming, i.e. getLower and getUpper?

you are right, will fix

pugnascotia · 2019-12-10T11:34:30Z

rest-api-spec/src/main/resources/rest-api-spec/test/search/180_locale_dependent_mapping.yml

 ---
 "Test Index and Search locale dependent mappings / dates":
-  - skip:
-      version: " - 6.1.99"


Is this redundant now?

so in master I think it is not needed. The - 6.1.99 is translated into 7.0.0 - 6.1.99 And master runs bwc tests from 7.0 - 8 range

pugnascotia · 2019-12-10T11:40:46Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java

-        boolean skip = lowerVersion != null && upperVersion != null && currentVersion.onOrAfter(lowerVersion)
-            && currentVersion.onOrBefore(upperVersion);
+        boolean skip = versionRanges.stream().anyMatch(range -> range.contains(currentVersion));
        skip |= Features.areAllSupported(features) == false;


Do you think it's worth changing this to the following?

return skip || Features.areAllSupported(features) == false;

pugnascotia · 2019-12-10T11:43:00Z

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java

-            lower.isEmpty() ? VersionUtils.getFirstVersion() : Version.fromString(lower),
-            upper.isEmpty() ? Version.CURRENT : Version.fromString(upper)
-        };
+    private static String wrapWithSpaces(String rawRange) {


Can we remove this now?

Multiple version ranges are allowed to be used in section skip in yml tests. This is useful when a bugfix was backported to latest versions and all previous releases contain a wire breaking bug. examples: 6.1.0 - 6.3.0, 6.6.0 - 6.7.9, 7.0 - - 7.2, 8.0.0 -

Multiple version ranges are allowed to be used in section skip in yml tests. This is useful when a bugfix was backported to latest versions and all previous releases contain a wire breaking bug. examples: 6.1.0 - 6.3.0, 6.6.0 - 6.7.9, 7.0 - - 7.2, 8.0.0 - backport #50014

Multiple version ranges are allowed to be used in section skip in yml tests. This is useful when a bugfix was backported to latest versions and all previous releases contain a wire breaking bug. examples: 6.1.0 - 6.3.0, 6.6.0 - 6.7.9, 7.0 - - 7.2, 8.0.0 -

pgomulka added 2 commits December 10, 2019 10:02

Merge branch 'master' into test/skip_section_range

c0b1bc3

pgomulka added the :Delivery/Build Build or test infrastructure label Dec 10, 2019

pgomulka self-assigned this Dec 10, 2019

cleanup

c096724

pugnascotia reviewed Dec 10, 2019

View reviewed changes

code review follow up and licence header

ef6b01a

pugnascotia reviewed Dec 10, 2019

View reviewed changes

method renames and split fix

566f7e2

pgomulka requested a review from pugnascotia December 10, 2019 11:32

pugnascotia reviewed Dec 10, 2019

View reviewed changes

pugnascotia approved these changes Dec 10, 2019

View reviewed changes

cleanup

1b7bf45

pgomulka added v7.6.0 v8.0.0 labels Dec 10, 2019

pgomulka merged commit 90c59a1 into elastic:master Dec 10, 2019

pgomulka mentioned this pull request Dec 10, 2019

Allow skipping ranges of versions backport(#50014) #50028

Merged

mark-vieira added the Team:Delivery Meta label for Delivery team label Nov 11, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

ChrisHegarty unassigned pgomulka Oct 29, 2024

	assert versionRanges.size() >= 1;
	assert versionRanges.isEmpty() == false;

Conversation

pgomulka commented Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

elasticmachine commented Dec 10, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgomulka Dec 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pgomulka commented Dec 10, 2019 •

edited

Loading

pgomulka Dec 10, 2019 •

edited

Loading

pgomulka Dec 10, 2019 •

edited

Loading