From 2320a388338f7eeb7c0f5f8af9abb6d45c2d2b70 Mon Sep 17 00:00:00 2001 From: Przemyslaw Gomulka Date: Thu, 15 Jul 2021 10:50:42 +0200 Subject: [PATCH] [Rest Api Compatibility] Allow transforming warnings per test (#75187) Warning related transformations missed the possibility to apply per single test only. Also a warning changed in #67158 for indices.close so this PR also applies the transformation for 7.x test relates #51816 --- .../compat/RestCompatTestTransformTask.java | 18 +++++++++++++ .../warnings/InjectAllowedWarnings.java | 27 ++++++++++++++++++- .../transform/warnings/RemoveWarnings.java | 23 ++++++++++++++++ .../test/rest/transform/TransformTests.java | 20 +++++++------- .../InjectAllowedWarningsRegexTests.java | 3 +-- .../warnings/InjectAllowedWarningsTests.java | 18 ++++++++++++- .../warnings/InjectWarningsRegexTests.java | 4 +-- .../warnings/InjectWarningsTests.java | 4 +-- .../warnings/RemoveWarningsTests.java | 17 ++++++++++++ .../with_existing_allowed_warnings.yml | 12 +++++++++ rest-api-spec/build.gradle | 7 +++-- 11 files changed, 132 insertions(+), 21 deletions(-) diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/rest/compat/RestCompatTestTransformTask.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/rest/compat/RestCompatTestTransformTask.java index 3414acf06b3d3..6446a11fd3eda 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/rest/compat/RestCompatTestTransformTask.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/rest/compat/RestCompatTestTransformTask.java @@ -282,6 +282,15 @@ public void removeWarning(String... warnings) { transformations.add(new RemoveWarnings(Set.copyOf(Arrays.asList(warnings)))); } + /** + * Removes one or more warnings + * @param warnings the warning(s) to remove + * @param testName the test name to remove the warning + */ + public void removeWarningForTest(String warnings, String testName) { + transformations.add(new RemoveWarnings(Set.copyOf(Arrays.asList(warnings)), testName)); + } + /** * Adds one or more allowed warnings * @param allowedWarnings the warning(s) to add @@ -298,6 +307,15 @@ public void addAllowedWarningRegex(String... allowedWarningsRegex) { transformations.add(new InjectAllowedWarnings(true, Arrays.asList(allowedWarningsRegex))); } + /** + * Adds one or more allowed regular expression warnings + * @param allowedWarningsRegex the regex warning(s) to add + * @testName the test name to add a allowedWarningRegex + */ + public void addAllowedWarningRegexForTest(String allowedWarningsRegex, String testName) { + transformations.add(new InjectAllowedWarnings(true, Arrays.asList(allowedWarningsRegex), testName)); + } + @OutputDirectory public DirectoryProperty getOutputDirectory() { return outputDirectory; diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarnings.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarnings.java index 6976ce6cd9443..8bfbadbe86ad3 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarnings.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarnings.java @@ -11,10 +11,13 @@ import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.JsonNodeFactory; import com.fasterxml.jackson.databind.node.ObjectNode; + +import org.elasticsearch.gradle.internal.test.rest.transform.RestTestContext; import org.elasticsearch.gradle.internal.test.rest.transform.RestTestTransformByParentObject; import org.elasticsearch.gradle.internal.test.rest.transform.feature.FeatureInjector; import org.gradle.api.tasks.Input; import org.gradle.api.tasks.Internal; +import org.gradle.api.tasks.Optional; import java.util.List; @@ -26,6 +29,7 @@ public class InjectAllowedWarnings extends FeatureInjector implements RestTestTr private static JsonNodeFactory jsonNodeFactory = JsonNodeFactory.withExactBigDecimals(false); private final List allowedWarnings; + private String testName; private final boolean isRegex; /** @@ -40,8 +44,18 @@ public InjectAllowedWarnings(List allowedWarnings) { * @param allowedWarnings The allowed warnings to inject */ public InjectAllowedWarnings(boolean isRegex, List allowedWarnings) { + this(isRegex, allowedWarnings, null); + } + + /** + * @param isRegex true if should inject the regex variant of allowed warnings + * @param allowedWarnings The allowed warnings to inject + * @param testName The testName to inject + */ + public InjectAllowedWarnings(boolean isRegex, List allowedWarnings, String testName) { this.isRegex = isRegex; this.allowedWarnings = allowedWarnings; + this.testName = testName; } @Override @@ -52,7 +66,7 @@ public void transformTest(ObjectNode doNodeParent) { arrayWarnings = new ArrayNode(jsonNodeFactory); doNodeValue.set(getSkipFeatureName(), arrayWarnings); } - allowedWarnings.forEach(arrayWarnings::add); + this.allowedWarnings.forEach(arrayWarnings::add); } @Override @@ -71,4 +85,15 @@ public String getSkipFeatureName() { public List getAllowedWarnings() { return allowedWarnings; } + + @Override + public boolean shouldApply(RestTestContext testContext) { + return testName == null || testContext.getTestName().equals(testName); + } + + @Input + @Optional + public String getTestName() { + return testName; + } } diff --git a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/RemoveWarnings.java b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/RemoveWarnings.java index a1d4e6d206924..bd816f6a680f8 100644 --- a/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/RemoveWarnings.java +++ b/build-tools-internal/src/main/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/RemoveWarnings.java @@ -10,9 +10,12 @@ import com.fasterxml.jackson.databind.node.ArrayNode; import com.fasterxml.jackson.databind.node.ObjectNode; + +import org.elasticsearch.gradle.internal.test.rest.transform.RestTestContext; import org.elasticsearch.gradle.internal.test.rest.transform.RestTestTransformByParentObject; import org.gradle.api.tasks.Input; import org.gradle.api.tasks.Internal; +import org.gradle.api.tasks.Optional; import java.util.ArrayList; import java.util.List; @@ -28,6 +31,7 @@ public class RemoveWarnings implements RestTestTransformByParentObject { private final Set warnings; + private String testName; /** * @param warnings The allowed warnings to inject @@ -35,6 +39,14 @@ public class RemoveWarnings implements RestTestTransformByParentObject { public RemoveWarnings(Set warnings) { this.warnings = warnings; } + /** + * @param warnings The allowed warnings to inject + * @param testName The testName to inject + */ + public RemoveWarnings(Set warnings, String testName) { + this.warnings = warnings; + this.testName = testName; + } @Override public void transformTest(ObjectNode doNodeParent) { @@ -66,4 +78,15 @@ public String getKeyToFind() { public Set getWarnings() { return warnings; } + + @Override + public boolean shouldApply(RestTestContext testContext) { + return testName == null || testContext.getTestName().equals(testName); + } + + @Input + @Optional + public String getTestName() { + return testName; + } } diff --git a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/TransformTests.java b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/TransformTests.java index 62ed726ae949d..b0a5e02e1b93b 100644 --- a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/TransformTests.java +++ b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/TransformTests.java @@ -17,24 +17,25 @@ import com.fasterxml.jackson.databind.node.TextNode; import com.fasterxml.jackson.dataformat.yaml.YAMLFactory; import com.fasterxml.jackson.dataformat.yaml.YAMLParser; + import org.elasticsearch.gradle.internal.test.GradleUnitTestCase; import org.elasticsearch.gradle.internal.test.rest.transform.headers.InjectHeaders; import org.hamcrest.CoreMatchers; +import org.hamcrest.Matchers; import org.hamcrest.core.IsCollectionContaining; import org.junit.Before; import java.io.File; import java.io.IOException; import java.util.ArrayList; +import java.util.Collection; import java.util.Collections; import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; -import java.util.Set; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.LongAdder; -import java.util.function.Consumer; import java.util.stream.Collectors; public abstract class TransformTests extends GradleUnitTestCase { @@ -181,11 +182,11 @@ protected ObjectNode getSkipNode(ArrayNode setupNodeValue) { return null; } - protected void validateBodyHasWarnings(String featureName, List tests, Set expectedWarnings) { + protected void validateBodyHasWarnings(String featureName, List tests, Collection expectedWarnings) { validateBodyHasWarnings(featureName, null, tests, expectedWarnings); } - protected void validateBodyHasWarnings(String featureName, String testName, List tests, Set expectedWarnings) { + protected void validateBodyHasWarnings(String featureName, String testName, List tests, Collection expectedWarnings) { AtomicBoolean actuallyDidSomething = new AtomicBoolean(false); tests.forEach(test -> { Iterator> testsIterator = test.fields(); @@ -201,13 +202,10 @@ protected void validateBodyHasWarnings(String featureName, String testName, List ObjectNode doSection = (ObjectNode) testSection.get("do"); assertThat(doSection.get(featureName), CoreMatchers.notNullValue()); ArrayNode warningsNode = (ArrayNode) doSection.get(featureName); - LongAdder assertions = new LongAdder(); - warningsNode.forEach(warning -> { - if (expectedWarnings.contains(warning.asText())) { - assertions.increment(); - } - }); - assertThat(assertions.intValue(), CoreMatchers.equalTo(expectedWarnings.size())); + List actual = new ArrayList<>(); + warningsNode.forEach(node -> actual.add(node.asText())); + String[] expected = expectedWarnings.toArray(new String[]{}); + assertThat(actual, Matchers.containsInAnyOrder(expected)); actuallyDidSomething.set(true); } }); diff --git a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarningsRegexTests.java b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarningsRegexTests.java index 3deddc8309a47..9d24b57f5689e 100644 --- a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarningsRegexTests.java +++ b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarningsRegexTests.java @@ -49,8 +49,7 @@ public void testInjectAllowedWarningsWithPreExisting() throws Exception { List transformedTests = transformTests(tests); printTest(testName, transformedTests); validateSetupAndTearDown(transformedTests); - validateBodyHasWarnings(ALLOWED_WARNINGS_REGEX, tests, Set.of("c", "d")); - validateBodyHasWarnings(ALLOWED_WARNINGS_REGEX, tests, addWarnings); + validateBodyHasWarnings(ALLOWED_WARNINGS_REGEX, tests, Set.of("c", "d", "added warning")); } @Override diff --git a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarningsTests.java b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarningsTests.java index 580204d6e6819..b958b07773bca 100644 --- a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarningsTests.java +++ b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectAllowedWarningsTests.java @@ -49,8 +49,24 @@ public void testInjectAllowedWarningsWithPreExisting() throws Exception { List transformedTests = transformTests(tests); printTest(testName, transformedTests); validateSetupAndTearDown(transformedTests); + validateBodyHasWarnings(ALLOWED_WARNINGS, transformedTests, List.of("a", "b", "added warning")); + } + + @Test + public void testInjectAllowedWarningsWithPreExistingForSingleTest() throws Exception { + String testName = "/rest/transform/warnings/with_existing_allowed_warnings.yml"; + List tests = getTests(testName); + validateSetupExist(tests); validateBodyHasWarnings(ALLOWED_WARNINGS, tests, Set.of("a", "b")); - validateBodyHasWarnings(ALLOWED_WARNINGS, tests, addWarnings); + List transformedTests = transformTests(tests, getTransformationsForTest("Test with existing allowed warnings")); + printTest(testName, transformedTests); + validateSetupAndTearDown(transformedTests); + validateBodyHasWarnings(ALLOWED_WARNINGS, "Test with existing allowed warnings", transformedTests, Set.of("a", "b", "added warning")); + validateBodyHasWarnings(ALLOWED_WARNINGS, "Test with existing allowed warnings not to change", transformedTests, Set.of("a", "b")); + } + + private List> getTransformationsForTest(String testName) { + return Collections.singletonList(new InjectAllowedWarnings(false, new ArrayList<>(addWarnings), testName)); } @Override diff --git a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectWarningsRegexTests.java b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectWarningsRegexTests.java index ad95016add15c..6107387d796fc 100644 --- a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectWarningsRegexTests.java +++ b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectWarningsRegexTests.java @@ -67,8 +67,8 @@ public void testInjectWarningsWithPreExisting() throws Exception { List transformedTests = transformTests(tests); printTest(testName, transformedTests); validateSetupAndTearDown(transformedTests); - validateBodyHasWarnings(WARNINGS_REGEX, tests, Set.of("c", "d")); - validateBodyHasWarnings(WARNINGS_REGEX, "Test warnings", tests, addWarnings); + validateBodyHasWarnings(WARNINGS_REGEX, "Not the test to change", tests, Set.of("c", "d")); + validateBodyHasWarnings(WARNINGS_REGEX, "Test warnings", tests, Set.of("c", "d", "added warning")); } @Override diff --git a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectWarningsTests.java b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectWarningsTests.java index cd338502e00d0..a4dbc1c0f49a2 100644 --- a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectWarningsTests.java +++ b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/InjectWarningsTests.java @@ -66,8 +66,8 @@ public void testInjectWarningsWithPreExisting() throws Exception { List transformedTests = transformTests(tests); printTest(testName, transformedTests); validateSetupAndTearDown(transformedTests); - validateBodyHasWarnings(WARNINGS, tests, Set.of("a", "b")); - validateBodyHasWarnings(WARNINGS, "Test warnings", tests, addWarnings); + validateBodyHasWarnings(WARNINGS, "Not the test to change", tests, Set.of("a", "b")); + validateBodyHasWarnings(WARNINGS, "Test warnings", tests, Set.of("a", "b", "added warning")); } @Override diff --git a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/RemoveWarningsTests.java b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/RemoveWarningsTests.java index 8f4b4d8066853..cc6e1c256a15a 100644 --- a/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/RemoveWarningsTests.java +++ b/build-tools-internal/src/test/java/org/elasticsearch/gradle/internal/test/rest/transform/warnings/RemoveWarningsTests.java @@ -50,6 +50,23 @@ public void testRemoveWarningWithPreExisting() throws Exception { validateBodyHasWarnings(WARNINGS, tests, Set.of("b")); } + @Test + public void testRemoveWarningWithPreExistingFromSingleTest() throws Exception { + String testName = "/rest/transform/warnings/with_existing_warnings.yml"; + List tests = getTests(testName); + validateSetupExist(tests); + validateBodyHasWarnings(WARNINGS, tests, Set.of("a", "b")); + List transformedTests = transformTests(tests, getTransformationsForTest("Test warnings")); + printTest(testName, transformedTests); + validateSetupAndTearDown(transformedTests); + validateBodyHasWarnings(WARNINGS, "Test warnings", tests, Set.of("b")); + validateBodyHasWarnings(WARNINGS, "Not the test to change", tests, Set.of("a", "b")); + } + + private List> getTransformationsForTest(String testName) { + return Collections.singletonList(new RemoveWarnings(Set.of("a"), testName)); + } + /** * test file has preexisting single warning */ diff --git a/build-tools-internal/src/test/resources/rest/transform/warnings/with_existing_allowed_warnings.yml b/build-tools-internal/src/test/resources/rest/transform/warnings/with_existing_allowed_warnings.yml index 2e07c956239fd..baf19b6fe185f 100644 --- a/build-tools-internal/src/test/resources/rest/transform/warnings/with_existing_allowed_warnings.yml +++ b/build-tools-internal/src/test/resources/rest/transform/warnings/with_existing_allowed_warnings.yml @@ -17,3 +17,15 @@ setup: id: "something" - match: { acknowledged: true } +--- +"Test with existing allowed warnings not to change": + - do: + allowed_warnings: + - "a" + - "b" + allowed_warnings_regex: + - "c" + - "d" + something: + id: "something_else" + - match: { acknowledged: true } diff --git a/rest-api-spec/build.gradle b/rest-api-spec/build.gradle index cf4b2f6b8f76c..1a7251640334a 100644 --- a/rest-api-spec/build.gradle +++ b/rest-api-spec/build.gradle @@ -92,8 +92,6 @@ tasks.named("yamlRestCompatTest").configure { 'cluster.voting_config_exclusions/10_basic/Throw exception when adding voting config exclusion without specifying nodes', 'indices.flush/10_basic/Index synced flush rest test', 'indices.forcemerge/10_basic/Check deprecation warning when incompatible only_expunge_deletes and max_num_segments values are both set', - 'indices.open/10_basic/?wait_for_active_shards default is deprecated', - 'indices.open/10_basic/?wait_for_active_shards=index-setting', // not fixing this in #70966 'indices.put_template/11_basic_with_types/Put template with empty mappings', 'search.aggregation/200_top_hits_metric/top_hits aggregation with sequence numbers', @@ -224,6 +222,11 @@ tasks.named("transformV7RestTests").configure({ task -> ) task.replaceValueInMatch("_all.primaries.indexing.types._doc.index_total", 2) + //override for "indices.open/10_basic/?wait_for_active_shards default is deprecated" and "indices.open/10_basic/?wait_for_active_shards=index-setting" + task.addAllowedWarningRegexForTest("\\?wait_for_active_shards=index-setting is now the default behaviour.*", "?wait_for_active_shards=index-setting") + task.removeWarningForTest("the default value for the ?wait_for_active_shards parameter will change from '0' to 'index-setting' in version 8; " + + "specify '?wait_for_active_shards=index-setting' to adopt the future default behaviour, or '?wait_for_active_shards=0' to preserve today's behaviour" + , "?wait_for_active_shards default is deprecated") }) tasks.register('enforceYamlTestConvention').configure {