ClientYamlTestSuiteIT test case failure

[harold-wang]

Task :rest-api-spec:yamlRestTest

REPRODUCE WITH: ./gradlew ':rest-api-spec:yamlRestTest' --tests "org.elasticsearch.test.rest.ClientYamlTestSuiteIT" -Dtests.method="test {p0=cat.templates/10_basic/Sort templates}" -Dtests.seed=923C938CF01174A6 -Dtests.security.manager=true -Dtests.locale=tr-TR -Dtests.timezone=Asia/Bahrain -Druntime.java=15

org.elasticsearch.test.rest.ClientYamlTestSuiteIT > test {p0=cat.templates/10_basic/Sort templates} FAILED
java.lang.AssertionError: Failure at [cat.templates/10_basic:230]: field [$body] was expected to match the provided regex but didn't
Expected: ^
test \s+ [t*] \s+ \n \n
test_1 \s+ [te*] \s+ 1 \n \n
$
but: was "test [t*] \ntest_1 [te*] 1\n"
at __randomizedtesting.SeedInfo.seed([923C938CF01174A6:1A68AC565EED195E]:0)
at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.executeSection(ESClientYamlSuiteTestCase.java:414)
at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.test(ESClientYamlSuiteTestCase.java:391)
at jdk.internal.reflect.GeneratedMethodAccessor12.invoke(Unknown Source)
at java.base/jdk.internal.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
at java.base/java.lang.reflect.Method.invoke(Method.java:564)
at com.carrotsearch.randomizedtesting.RandomizedRunner.invoke(RandomizedRunner.java:1750)
at com.carrotsearch.randomizedtesting.RandomizedRunner$8.evaluate(RandomizedRunner.java:938)
at com.carrotsearch.randomizedtesting.RandomizedRunner$9.evaluate(RandomizedRunner.java:974)
at com.carrotsearch.randomizedtesting.RandomizedRunner$10.evaluate(RandomizedRunner.java:988)
at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at org.apache.lucene.util.TestRuleSetupTeardownChained$1.evaluate(TestRuleSetupTeardownChained.java:49)
at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
at org.apache.lucene.util.TestRuleThreadAndTestName$1.evaluate(TestRuleThreadAndTestName.java:48)
at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)
at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)
at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
at com.carrotsearch.randomizedtesting.ThreadLeakControl.forkTimeoutingTask(ThreadLeakControl.java:817)
at com.carrotsearch.randomizedtesting.ThreadLeakControl$3.evaluate(ThreadLeakControl.java:468)
at com.carrotsearch.randomizedtesting.RandomizedRunner.runSingleTest(RandomizedRunner.java:947)
at com.carrotsearch.randomizedtesting.RandomizedRunner$5.evaluate(RandomizedRunner.java:832)
at com.carrotsearch.randomizedtesting.RandomizedRunner$6.evaluate(RandomizedRunner.java:883)
at com.carrotsearch.randomizedtesting.RandomizedRunner$7.evaluate(RandomizedRunner.java:894)
at org.apache.lucene.util.AbstractBeforeAfterRule$1.evaluate(AbstractBeforeAfterRule.java:45)
at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at org.apache.lucene.util.TestRuleStoreClassName$1.evaluate(TestRuleStoreClassName.java:41)
at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
at com.carrotsearch.randomizedtesting.rules.NoShadowingOrOverridesOnMethodsRule$1.evaluate(NoShadowingOrOverridesOnMethodsRule.java:40)
at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at org.apache.lucene.util.TestRuleAssertionsRequired$1.evaluate(TestRuleAssertionsRequired.java:53)
at org.apache.lucene.util.TestRuleMarkFailure$1.evaluate(TestRuleMarkFailure.java:47)
at org.apache.lucene.util.TestRuleIgnoreAfterMaxFailures$1.evaluate(TestRuleIgnoreAfterMaxFailures.java:64)
at org.apache.lucene.util.TestRuleIgnoreTestSuites$1.evaluate(TestRuleIgnoreTestSuites.java:54)
at com.carrotsearch.randomizedtesting.rules.StatementAdapter.evaluate(StatementAdapter.java:36)
at com.carrotsearch.randomizedtesting.ThreadLeakControl$StatementRunner.run(ThreadLeakControl.java:368)
at java.base/java.lang.Thread.run(Thread.java:832)

    Caused by:
    java.lang.AssertionError: field [$body] was expected to match the provided regex but didn't
    Expected: ^
        test   \s+ \[t\*\]  \s+   \n \n
        test_1 \s+ \[te\*\] \s+ 1 \n \n
    $
         but: was "test   [t*]  \ntest_1 [te*] 1\n"
        at org.hamcrest.MatcherAssert.assertThat(MatcherAssert.java:18)
        at org.junit.Assert.assertThat(Assert.java:956)
        at org.elasticsearch.test.rest.yaml.section.MatchAssertion.doAssert(MatchAssertion.java:68)
        at org.elasticsearch.test.rest.yaml.section.Assertion.execute(Assertion.java:76)
        at org.elasticsearch.test.rest.yaml.ESClientYamlSuiteTestCase.executeSection(ESClientYamlSuiteTestCase.java:407)
        ... 36 more

[nknize]

Interesting; it appears the code logic here doesn't work the way elastic expects but nevertheless is passing because it's actually always being skipped. It appears by removing the no_xpack feature flag we uncovered a logic bug in SkipSection.parse that causes this test to actually run (and fail).

The logic bug is in:SkipSection.parse. This method is not splitting the features string on the ',' delimiter. So Features.areAllSupported is (correctly) returning false but not for the reason expected. With the no_xpack flag added, the Features.SUPPORTED List does not contain the full string default_shards, no_xpack. When no_xpack is removed, Features.SUPPORTED does contain default_shards so the test is actually executed. The poor design here is Features.SUPPORTED doesn't contain no_xpack, so handling xpack availability is clearly a hacked afterthought.

I'll take this and see what I can dig up and do to correct the issue.

[nknize]

For posterity, here's a patch that fixes the logic bug and exposes the test failure:

diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java
index 81eb4708920..0113fd09e85 100644
--- a/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java
+++ b/test/framework/src/main/java/org/elasticsearch/test/rest/yaml/section/SkipSection.java
@@ -71,7 +71,17 @@ public class SkipSection {
                 } else if ("reason".equals(currentFieldName)) {
                     reason = parser.text();
                 } else if ("features".equals(currentFieldName)) {
-                    features.add(parser.text());
+                    String f = parser.text();
+                    // split on ','
+                    String[] fs = f.split(",");
+                    if (fs != null) {
+                        // add each feature, separately:
+                        for (String feature : fs) {
+                            features.add(feature.trim());
+                        }
+                    } else {
+                        features.add(f);
+                    }
                 }
                 else {
                     throw new ParsingException(parser.getTokenLocation(),

I think the fix here is a combination of the above patch, and to fix the expected result by removing the extra 'new line' characters

[nknize]

I opened PR #82 to fix this failure