[TEST] improve validation of yaml suites (elastic#34957)

Validation of test sections and suites consists of checking that the proper skip features sections are in place depending on the features used in tests.

The validation logic was previously only performed on do sections included in each test section, and the skip needed to be present in the same test section. What happens often though is that the skip is added to the setup section, or the teardown section.

This commit improves the validation of test suites by validating setup and teardown section first, then looking at each test section while still eventually reading the skip section from setup or teardown.

We are also making SkipSection, SetupSection, TearDownSection, ClientYamlTestSection and ClientYamlTestSuite immutable. Previously it was possible to utilize constants like SetupSection.EMPTY, which were modifiable and affect every other future users by modifiying them. This has been corrected.

Also, validation has been improved to cumulate errors so that all the errors from a suite will be listed at once.

Relates to elastic#34735

test/framework/src/main/java/org/elasticsearch/test/rest/yaml/ESClientYamlSuiteTestCase.java

@@ -38,7 +38,6 @@
 import org.elasticsearch.test.rest.yaml.restspec.ClientYamlSuiteRestSpec;
 import org.elasticsearch.test.rest.yaml.section.ClientYamlTestSection;
 import org.elasticsearch.test.rest.yaml.section.ClientYamlTestSuite;
-import org.elasticsearch.test.rest.yaml.section.DoSection;
 import org.elasticsearch.test.rest.yaml.section.ExecutableSection;
 import org.junit.AfterClass;
 import org.junit.Before;
@@ -184,19 +183,44 @@ public static Iterable<Object[]> createParameters() throws Exception {
      */
     public static Iterable<Object[]> createParameters(NamedXContentRegistry executeableSectionRegistry) throws Exception {
         String[] paths = resolvePathsProperty(REST_TESTS_SUITE, ""); // default to all tests under the test root
-        List<Object[]> tests = new ArrayList<>();
         Map<String, Set<Path>> yamlSuites = loadSuites(paths);
+        List<ClientYamlTestSuite> suites = new ArrayList<>();
+        IllegalArgumentException validationException = null;
         // yaml suites are grouped by directory (effectively by api)
         for (String api : yamlSuites.keySet()) {
             List<Path> yamlFiles = new ArrayList<>(yamlSuites.get(api));
             for (Path yamlFile : yamlFiles) {
-                ClientYamlTestSuite restTestSuite = ClientYamlTestSuite.parse(executeableSectionRegistry, api, yamlFile);
-                for (ClientYamlTestSection testSection : restTestSuite.getTestSections()) {
-                    tests.add(new Object[]{ new ClientYamlTestCandidate(restTestSuite, testSection) });
+                ClientYamlTestSuite suite = ClientYamlTestSuite.parse(executeableSectionRegistry, api, yamlFile);
+                suites.add(suite);
+                try {
+                    suite.validate();
+                } catch(IllegalArgumentException e) {
+                    if (validationException == null) {
+                        validationException = new IllegalArgumentException("Validation errors for the following test suites:\n- "
+                            + e.getMessage());
+                    } else {
+                        String previousMessage = validationException.getMessage();
+                        Throwable[] suppressed = validationException.getSuppressed();
+                        validationException = new IllegalArgumentException(previousMessage + "\n- " + e.getMessage());
+                        for (Throwable t : suppressed) {
+                            validationException.addSuppressed(t);
+                        }
+                    }
+                    validationException.addSuppressed(e);
                 }
             }
         }
 
+        if (validationException != null) {
+            throw validationException;
+        }
+
+        List<Object[]> tests = new ArrayList<>();
+        for (ClientYamlTestSuite yamlTestSuite : suites) {
+            for (ClientYamlTestSection testSection : yamlTestSuite.getTestSections()) {
+                tests.add(new Object[]{ new ClientYamlTestCandidate(yamlTestSuite, testSection) });
+            }
+        }
         //sort the candidates so they will always be in the same order before being shuffled, for repeatability
         tests.sort(Comparator.comparing(o -> ((ClientYamlTestCandidate) o[0]).getTestPath()));
         return tests;
@@ -361,7 +385,7 @@ public void test() throws IOException {
             }
         } finally {
             logger.debug("start teardown test [{}]", testCandidate.getTestPath());
-            for (DoSection doSection : testCandidate.getTeardownSection().getDoSections()) {
+            for (ExecutableSection doSection : testCandidate.getTeardownSection().getDoSections()) {
                 executeSection(doSection);
             }
             logger.debug("end teardown test [{}]", testCandidate.getTestPath());

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

@@ -18,49 +18,53 @@
  */
 package org.elasticsearch.test.rest.yaml.section;
 
-import org.elasticsearch.client.NodeSelector;
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.xcontent.XContentLocation;
 import org.elasticsearch.common.xcontent.XContentParser;
 
 import java.io.IOException;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Objects;
 
 /**
  * Represents a test section, which is composed of a skip section and multiple executable sections.
  */
 public class ClientYamlTestSection implements Comparable<ClientYamlTestSection> {
     public static ClientYamlTestSection parse(XContentParser parser) throws IOException {
         ParserUtils.advanceToFieldName(parser);
-        ClientYamlTestSection testSection = new ClientYamlTestSection(parser.getTokenLocation(), parser.currentName());
+        XContentLocation sectionLocation = parser.getTokenLocation();
+        String sectionName = parser.currentName();
+        List<ExecutableSection> executableSections = new ArrayList<>();
         try {
             parser.nextToken();
-            testSection.setSkipSection(SkipSection.parseIfNext(parser));
+            SkipSection skipSection = SkipSection.parseIfNext(parser);
             while (parser.currentToken() != XContentParser.Token.END_ARRAY) {
                 ParserUtils.advanceToFieldName(parser);
-                testSection.addExecutableSection(ExecutableSection.parse(parser));
+                executableSections.add(ExecutableSection.parse(parser));
             }
             if (parser.nextToken() != XContentParser.Token.END_OBJECT) {
-                throw new IllegalArgumentException("malformed section [" + testSection.getName() + "] expected ["
+                throw new IllegalArgumentException("malformed section [" + sectionName + "] expected ["
                         + XContentParser.Token.END_OBJECT + "] but was [" + parser.currentToken() + "]");
             }
             parser.nextToken();
-            return testSection;
+            return new ClientYamlTestSection(sectionLocation, sectionName, skipSection, executableSections);
         } catch (Exception e) {
-            throw new ParsingException(parser.getTokenLocation(), "Error parsing test named [" + testSection.getName() + "]", e);
+            throw new ParsingException(parser.getTokenLocation(), "Error parsing test named [" + sectionName + "]", e);
         }
     }
 
     private final XContentLocation location;
     private final String name;
-    private SkipSection skipSection;
+    private final SkipSection skipSection;
     private final List<ExecutableSection> executableSections;
 
-    public ClientYamlTestSection(XContentLocation location, String name) {
+    ClientYamlTestSection(XContentLocation location, String name, SkipSection skipSection, List<ExecutableSection> executableSections) {
         this.location = location;
         this.name = name;
-        this.executableSections = new ArrayList<>();
+        this.skipSection = Objects.requireNonNull(skipSection, "skip section cannot be null");
+        this.executableSections = Collections.unmodifiableList(executableSections);
     }
 
     public XContentLocation getLocation() {
@@ -75,40 +79,10 @@ public SkipSection getSkipSection() {
         return skipSection;
     }
 
-    public void setSkipSection(SkipSection skipSection) {
-        this.skipSection = skipSection;
-    }
-
     public List<ExecutableSection> getExecutableSections() {
         return executableSections;
     }
 
-    public void addExecutableSection(ExecutableSection executableSection) {
-        if (executableSection instanceof DoSection) {
-            DoSection doSection = (DoSection) executableSection;
-            if (false == doSection.getExpectedWarningHeaders().isEmpty()
-                    && false == skipSection.getFeatures().contains("warnings")) {
-                throw new IllegalArgumentException("Attempted to add a [do] with a [warnings] section without a corresponding [skip] so "
-                        + "runners that do not support the [warnings] section can skip the test at line ["
-                        + doSection.getLocation().lineNumber + "]");
-            }
-            if (NodeSelector.ANY != doSection.getApiCallSection().getNodeSelector()
-                    && false == skipSection.getFeatures().contains("node_selector")) {
-                throw new IllegalArgumentException("Attempted to add a [do] with a [node_selector] section without a corresponding "
-                    + "[skip] so runners that do not support the [node_selector] section can skip the test at line ["
-                    + doSection.getLocation().lineNumber + "]");
-            }
-        }
-        if (executableSection instanceof ContainsAssertion) {
-            if (false == skipSection.getFeatures().contains("contains")) {
-                throw new IllegalArgumentException("Attempted to add a [contains] assertion without a corresponding "
-                    + "[skip: \"features\": \"contains\"] so runners that do not support the [contains] assertion " +
-                    "can skip the test at line [" + executableSection.getLocation().lineNumber + "]");
-            }
-        }
-        this.executableSections.add(executableSection);
-    }
-
     @Override
     public boolean equals(Object o) {
         if (this == o) return true;

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

@@ -18,6 +18,7 @@
  */
 package org.elasticsearch.test.rest.yaml.section;
 
+import org.elasticsearch.client.NodeSelector;
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
 import org.elasticsearch.common.xcontent.NamedXContentRegistry;
@@ -32,9 +33,13 @@
 import java.nio.file.Path;
 import java.nio.file.StandardOpenOption;
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.List;
+import java.util.Objects;
 import java.util.Set;
 import java.util.TreeSet;
+import java.util.stream.Collectors;
+import java.util.stream.Stream;
 
 /**
  * Holds a REST test suite loaded from a specific yaml file.
@@ -79,11 +84,10 @@ public static ClientYamlTestSuite parse(String api, String suiteName, XContentPa
                     "expected token to be START_OBJECT but was " + parser.currentToken());
         }
 
-        ClientYamlTestSuite restTestSuite = new ClientYamlTestSuite(api, suiteName);
-
-        restTestSuite.setSetupSection(SetupSection.parseIfNext(parser));
-        restTestSuite.setTeardownSection(TeardownSection.parseIfNext(parser));
+        SetupSection setupSection = SetupSection.parseIfNext(parser);
+        TeardownSection teardownSection = TeardownSection.parseIfNext(parser);
 
+        Set<ClientYamlTestSection> testSections = new TreeSet<>();
         while(true) {
             //the "---" section separator is not understood by the yaml parser. null is returned, same as when the parser is closed
             //we need to somehow distinguish between a null in the middle of a test ("---")
@@ -93,27 +97,28 @@ public static ClientYamlTestSuite parse(String api, String suiteName, XContentPa
                     break;
                 }
             }
-
             ClientYamlTestSection testSection = ClientYamlTestSection.parse(parser);
-            if (!restTestSuite.addTestSection(testSection)) {
+            if (testSections.add(testSection) == false) {
                 throw new ParsingException(testSection.getLocation(), "duplicate test section [" + testSection.getName() + "]");
             }
         }
 
-        return restTestSuite;
+        return new ClientYamlTestSuite(api, suiteName, setupSection, teardownSection, new ArrayList<>(testSections));
     }
 
     private final String api;
     private final String name;
+    private final SetupSection setupSection;
+    private final TeardownSection teardownSection;
+    private final List<ClientYamlTestSection> testSections;
 
-    private SetupSection setupSection;
-    private TeardownSection teardownSection;
-
-    private Set<ClientYamlTestSection> testSections = new TreeSet<>();
-
-    public ClientYamlTestSuite(String api, String name) {
+    ClientYamlTestSuite(String api, String name, SetupSection setupSection, TeardownSection teardownSection,
+                        List<ClientYamlTestSection> testSections) {
         this.api = api;
         this.name = name;
+        this.setupSection = Objects.requireNonNull(setupSection, "setup section cannot be null");
+        this.teardownSection = Objects.requireNonNull(teardownSection, "teardown section cannot be null");
+        this.testSections = Collections.unmodifiableList(testSections);
     }
 
     public String getApi() {
@@ -132,27 +137,63 @@ public SetupSection getSetupSection() {
         return setupSection;
     }
 
-    public void setSetupSection(SetupSection setupSection) {
-        this.setupSection = setupSection;
-    }
-
     public TeardownSection getTeardownSection() {
         return teardownSection;
     }
 
-    public void setTeardownSection(TeardownSection teardownSection) {
-        this.teardownSection = teardownSection;
+    public void validate() {
+        Stream<String> errors = validateExecutableSections(setupSection.getExecutableSections(), null, setupSection, null);
+        errors = Stream.concat(errors, validateExecutableSections(teardownSection.getDoSections(), null, null, teardownSection));
+        errors = Stream.concat(errors, testSections.stream()
+            .flatMap(section -> validateExecutableSections(section.getExecutableSections(), section, setupSection, teardownSection)));
+        String errorMessage = errors.collect(Collectors.joining(",\n"));
+        if (errorMessage.isEmpty() == false) {
+            throw new IllegalArgumentException(getPath() + ":\n" + errorMessage);
+        }
+    }
+
+    private static Stream<String> validateExecutableSections(List<ExecutableSection> sections,
+                                                   ClientYamlTestSection testSection,
+                                                   SetupSection setupSection, TeardownSection teardownSection) {
+
+        Stream<String> errors = sections.stream().filter(section -> section instanceof DoSection)
+            .map(section -> (DoSection) section)
+            .filter(section -> false == section.getExpectedWarningHeaders().isEmpty())
+            .filter(section -> false == hasSkipFeature("warnings", testSection, setupSection, teardownSection))
+            .map(section -> "attempted to add a [do] with a [warnings] section " +
+                "without a corresponding [\"skip\": \"features\": \"warnings\"] so runners that do not support the [warnings] " +
+                "section can skip the test at line [" + section.getLocation().lineNumber + "]");
+
+        errors = Stream.concat(errors, sections.stream().filter(section -> section instanceof DoSection)
+            .map(section -> (DoSection) section)
+            .filter(section -> NodeSelector.ANY != section.getApiCallSection().getNodeSelector())
+            .filter(section -> false == hasSkipFeature("node_selector", testSection, setupSection, teardownSection))
+            .map(section -> "attempted to add a [do] with a [node_selector] " +
+                "section without a corresponding [\"skip\": \"features\": \"node_selector\"] so runners that do not support the " +
+                "[node_selector] section can skip the test at line [" + section.getLocation().lineNumber + "]"));
+
+        errors = Stream.concat(errors, sections.stream()
+            .filter(section -> section instanceof ContainsAssertion)
+            .filter(section -> false == hasSkipFeature("contains", testSection, setupSection, teardownSection))
+            .map(section -> "attempted to add a [contains] assertion " +
+                "without a corresponding [\"skip\": \"features\": \"contains\"] so runners that do not support the " +
+                "[contains] assertion can skip the test at line [" + section.getLocation().lineNumber + "]"));
+
+        return errors;
+    }
+
+    private static boolean hasSkipFeature(String feature, ClientYamlTestSection testSection,
+                                          SetupSection setupSection, TeardownSection teardownSection) {
+        return (testSection != null && hasSkipFeature(feature, testSection.getSkipSection())) ||
+            (setupSection != null && hasSkipFeature(feature, setupSection.getSkipSection())) ||
+            (teardownSection != null && hasSkipFeature(feature, teardownSection.getSkipSection()));
     }
 
-    /**
-     * Adds a {@link org.elasticsearch.test.rest.yaml.section.ClientYamlTestSection} to the REST suite
-     * @return true if the test section was not already present, false otherwise
-     */
-    public boolean addTestSection(ClientYamlTestSection testSection) {
-        return this.testSections.add(testSection);
+    private static boolean hasSkipFeature(String feature, SkipSection skipSection) {
+        return skipSection != null && skipSection.getFeatures().contains(feature);
     }
 
     public List<ClientYamlTestSection> getTestSections() {
-        return new ArrayList<>(testSections);
+        return testSections;
     }
 }

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

@@ -182,7 +182,6 @@ public static DoSection parse(XContentParser parser) throws IOException {
         return doSection;
     }
 
-
     private static final Logger logger = LogManager.getLogger(DoSection.class);
 
     private final XContentLocation location;
@@ -206,23 +205,23 @@ public ApiCallSection getApiCallSection() {
         return apiCallSection;
     }
 
-    public void setApiCallSection(ApiCallSection apiCallSection) {
+    void setApiCallSection(ApiCallSection apiCallSection) {
         this.apiCallSection = apiCallSection;
     }
 
     /**
      * Warning headers that we expect from this response. If the headers don't match exactly this request is considered to have failed.
      * Defaults to emptyList.
      */
-    public List<String> getExpectedWarningHeaders() {
+    List<String> getExpectedWarningHeaders() {
         return expectedWarningHeaders;
     }
 
     /**
      * Set the warning headers that we expect from this response. If the headers don't match exactly this request is considered to have
      * failed. Defaults to emptyList.
      */
-    public void setExpectedWarningHeaders(List<String> expectedWarningHeaders) {
+    void setExpectedWarningHeaders(List<String> expectedWarningHeaders) {
         this.expectedWarningHeaders = expectedWarningHeaders;
     }