[TEST] replace ResponseBodyAssertion with existing MatchAssertion

We introduced a special response_body assertion to test our docs snippets. The match assertion does the same job though and can be reused and adapted where needed. ResponseBodyAssertion contains provides much better and accurate errors though, which can be now utilized in MatchAssertion so that many more REST tests can benefit from readable error messages.

 Each response body gets always stashed and can be retrieved for later evaluations already. Instead of providing the response body as strings that get parsed to json objects separately, then converted to maps as ResponseBodyAssertion did, we parse everything once, the json is part of the yaml test, which is supported. The only downside is that json comments cannot be used, rather yaml comments should be used (// C style vs # ). There were only two docs tests that were using comments in ingest-node.asciidoc where I went ahead and remove the comments which didn't seem that useful anyways.

buildSrc/src/main/groovy/org/elasticsearch/gradle/doc/RestTestsFromSnippetsTask.groovy

@@ -131,8 +131,9 @@ public class RestTestsFromSnippetsTask extends SnippetsTask {
         }
 
         private void response(Snippet response) {
-            current.println("  - response_body: |")
-            response.contents.eachLine { current.println("      $it") }
+            current.println("  - match: ")
+            current.println("      \$body: ")
+            response.contents.eachLine { current.println("        $it") }
         }
 
         void emitDo(String method, String pathAndQuery,

docs/reference/ingest/ingest-node.asciidoc

@@ -46,7 +46,6 @@ PUT _ingest/pipeline/my-pipeline-id
         "value": "bar"
       }
     }
-    // other processors
   ]
 }
 --------------------------------------------------
@@ -83,7 +82,6 @@ Example response:
             "value": "bar"
           }
         }
-        // other processors
       ]
     }
   } ]

test/framework/src/main/java/org/elasticsearch/test/rest/parser/RestTestSuiteParseContext.java

@@ -18,22 +18,21 @@
  */
 package org.elasticsearch.test.rest.parser;
 
-import java.io.IOException;
-import java.util.HashMap;
-import java.util.Map;
-
 import org.elasticsearch.common.collect.Tuple;
 import org.elasticsearch.common.xcontent.XContentLocation;
 import org.elasticsearch.common.xcontent.XContentParser;
 import org.elasticsearch.common.xcontent.XContentParser.Token;
 import org.elasticsearch.test.rest.section.DoSection;
 import org.elasticsearch.test.rest.section.ExecutableSection;
-import org.elasticsearch.test.rest.section.ResponseBodyAssertion;
 import org.elasticsearch.test.rest.section.SetupSection;
 import org.elasticsearch.test.rest.section.SkipSection;
 import org.elasticsearch.test.rest.section.TeardownSection;
 import org.elasticsearch.test.rest.section.TestSection;
 
+import java.io.IOException;
+import java.util.HashMap;
+import java.util.Map;
+
 /**
  * Context shared across the whole tests parse phase.
  * Provides shared parse methods and holds information needed to parse the test sections (e.g. es version)
@@ -57,7 +56,6 @@ public class RestTestSuiteParseContext {
         EXECUTABLE_SECTIONS_PARSERS.put("lt", new LessThanParser());
         EXECUTABLE_SECTIONS_PARSERS.put("lte", new LessThanOrEqualToParser());
         EXECUTABLE_SECTIONS_PARSERS.put("length", new LengthParser());
-        EXECUTABLE_SECTIONS_PARSERS.put("response_body", ResponseBodyAssertion.PARSER);
     }
 
     private final String api;

test/framework/src/main/java/org/elasticsearch/test/rest/section/MatchAssertion.java

@@ -18,15 +18,21 @@
  */
 package org.elasticsearch.test.rest.section;
 
+import org.elasticsearch.common.Nullable;
 import org.elasticsearch.common.logging.ESLogger;
 import org.elasticsearch.common.logging.Loggers;
 
+import java.util.List;
+import java.util.Locale;
+import java.util.Map;
+import java.util.Objects;
+import java.util.TreeMap;
 import java.util.regex.Pattern;
 
 import static org.elasticsearch.test.hamcrest.RegexMatcher.matches;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
-import static org.hamcrest.Matchers.notNullValue;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
 
 /**
@@ -45,7 +51,6 @@ public class MatchAssertion extends Assertion {
 
     @Override
     protected void doAssert(Object actualValue, Object expectedValue) {
-
         //if the value is wrapped into / it is a regexp (e.g. /s+d+/)
         if (expectedValue instanceof String) {
             String expValue = ((String) expectedValue).trim();
@@ -60,20 +65,131 @@ public class MatchAssertion extends Assertion {
             }
         }
 
-        assertThat(errorMessage(), actualValue, notNullValue());
+        assertNotNull("field [" + getField() + "] is null", actualValue);
         logger.trace("assert that [{}] matches [{}] (field [{}])", actualValue, expectedValue, getField());
-        if (!actualValue.getClass().equals(safeClass(expectedValue))) {
+        if (actualValue.getClass().equals(safeClass(expectedValue)) == false) {
             if (actualValue instanceof Number && expectedValue instanceof Number) {
                 //Double 1.0 is equal to Integer 1
-                assertThat(errorMessage(), ((Number) actualValue).doubleValue(), equalTo(((Number) expectedValue).doubleValue()));
+                assertThat("field [" + getField() + "] doesn't match the expected value",
+                        ((Number) actualValue).doubleValue(), equalTo(((Number) expectedValue).doubleValue()));
                 return;
             }
         }
 
-        assertThat(errorMessage(), actualValue, equalTo(expectedValue));
+        if (expectedValue.equals(actualValue) == false) {
+            FailureMessage message = new FailureMessage(getField());
+            message.compare(getField(), actualValue, expectedValue);
+            throw new AssertionError(message.message);
+        }
     }
 
-    private String errorMessage() {
-        return "field [" + getField() + "] doesn't match the expected value";
+    private static class FailureMessage {
+        private final StringBuilder message;
+        private int indent = 0;
+
+        private FailureMessage(String field) {
+            this.message = new StringBuilder(field + " didn't match the expected value:\n");
+        }
+
+        private void compareMaps(Map<String, Object> actual, Map<String, Object> expected) {
+            actual = new TreeMap<>(actual);
+            expected = new TreeMap<>(expected);
+            for (Map.Entry<String, Object> expectedEntry : expected.entrySet()) {
+                compare(expectedEntry.getKey(), actual.remove(expectedEntry.getKey()), expectedEntry.getValue());
+            }
+            for (Map.Entry<String, Object> unmatchedEntry : actual.entrySet()) {
+                field(unmatchedEntry.getKey(), "unexpected but found [" + unmatchedEntry.getValue() + "]");
+            }
+        }
+
+        private void compareLists(List<Object> actual, List<Object> expected) {
+            int i = 0;
+            while (i < actual.size() && i < expected.size()) {
+                compare(Integer.toString(i), actual.get(i), expected.get(i));
+                i++;
+            }
+            if (actual.size() == expected.size()) {
+                return;
+            }
+            indent();
+            if (actual.size() < expected.size()) {
+                message.append("expected [").append(expected.size() - i).append("] more entries\n");
+                return;
+            }
+            message.append("received [").append(actual.size() - i).append("] more entries than expected\n");
+        }
+
+        private void compare(String field, @Nullable Object actual, Object expected) {
+            if (expected instanceof Map) {
+                if (actual == null) {
+                    field(field, "expected map but not found");
+                    return;
+                }
+                if (false == actual instanceof Map) {
+                    field(field, "expected map but found [" + actual + "]");
+                    return;
+                }
+                @SuppressWarnings("unchecked")
+                Map<String, Object> expectedMap = (Map<String, Object>) expected;
+                @SuppressWarnings("unchecked")
+                Map<String, Object> actualMap = (Map<String, Object>) actual;
+                if (expectedMap.isEmpty() && actualMap.isEmpty()) {
+                    field(field, "same [empty map]");
+                    return;
+                }
+                field(field, null);
+                indent += 1;
+                compareMaps(actualMap, expectedMap);
+                indent -= 1;
+                return;
+            }
+            if (expected instanceof List) {
+                if (actual == null) {
+                    field(field, "expected list but not found");
+                    return;
+                }
+                if (false == actual instanceof List) {
+                    field(field, "expected list but found [" + actual + "]");
+                    return;
+                }
+                @SuppressWarnings("unchecked")
+                List<Object> expectedList = (List<Object>) expected;
+                @SuppressWarnings("unchecked")
+                List<Object> actualList = (List<Object>) actual;
+                if (expectedList.isEmpty() && actualList.isEmpty()) {
+                    field(field, "same [empty list]");
+                    return;
+                }
+                field(field, null);
+                indent += 1;
+                compareLists(actualList, expectedList);
+                indent -= 1;
+                return;
+            }
+            if (actual == null) {
+                field(field, "expected [" + expected + "] but not found");
+                return;
+            }
+            if (Objects.equals(expected, actual)) {
+                field(field, "same [" + expected + "]");
+                return;
+            }
+            field(field, "expected [" + expected + "] but was [" + actual + "]");
+        }
+
+        private void indent() {
+            for (int i = 0; i < indent; i++) {
+                message.append("  ");
+            }
+        }
+
+        private void field(Object name, String info) {
+            indent();
+            message.append(String.format(Locale.ROOT, "%30s: ", name));
+            if (info != null) {
+                message.append(info);
+            }
+            message.append('\n');
+        }
     }
 }

test/framework/src/main/java/org/elasticsearch/test/rest/section/ResponseBodyAssertion.java

@@ -1,170 +0,0 @@
-/*
- * Licensed to Elasticsearch under one or more contributor
- * license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright
- * ownership. Elasticsearch licenses this file to you under
- * the Apache License, Version 2.0 (the "License"); you may
- * not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *    http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied.  See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.elasticsearch.test.rest.section;
-
-import java.io.IOException;
-import java.util.List;
-import java.util.Locale;
-import java.util.Map;
-import java.util.Objects;
-import java.util.TreeMap;
-
-import org.elasticsearch.common.Nullable;
-import org.elasticsearch.common.xcontent.XContentParser;
-import org.elasticsearch.common.xcontent.json.JsonXContent;
-import org.elasticsearch.test.rest.parser.RestTestFragmentParser;
-import org.elasticsearch.test.rest.parser.RestTestParseException;
-import org.elasticsearch.test.rest.parser.RestTestSuiteParseContext;
-
-/**
- * Checks that the response body matches some text. 
- */
-public class ResponseBodyAssertion extends Assertion {
-    public static final RestTestFragmentParser<ResponseBodyAssertion> PARSER = new RestTestFragmentParser<ResponseBodyAssertion>() {
-        @Override
-        public ResponseBodyAssertion parse(RestTestSuiteParseContext parseContext) throws IOException, RestTestParseException {
-            try (XContentParser parser = JsonXContent.jsonXContent.createParser(parseContext.parseField())) {
-                return new ResponseBodyAssertion("$body", parser.map());
-            }
-        }
-    };
-
-    private ResponseBodyAssertion(String field, Map<String, Object> expectedValue) {
-        super(field, expectedValue);
-    }
-
-    @Override
-    protected void doAssert(Object actualValue, Object expectedValue) {
-        if (false == expectedValue.equals(actualValue)) {
-            @SuppressWarnings("unchecked")
-            Map<String, Object> actual = (Map<String, Object>) actualValue;
-            @SuppressWarnings("unchecked")
-            Map<String, Object> expected = (Map<String, Object>) expectedValue;
-            FailureMessage message = new FailureMessage();
-            message.compareMaps(actual, expected);
-            throw new AssertionError(message.message);
-        }
-    }
-
-    private class FailureMessage {
-        private final StringBuilder message = new StringBuilder("body didn't match the expected value:\n");
-        private int indent = 0;
-
-        private void compareMaps(Map<String, Object> actual, Map<String, Object> expected) {
-            actual = new TreeMap<>(actual);
-            expected = new TreeMap<>(expected);
-            for (Map.Entry<String, Object> expectedEntry : expected.entrySet()) {
-                compare(expectedEntry.getKey(), actual.remove(expectedEntry.getKey()), expectedEntry.getValue());
-            }
-            for (Map.Entry<String, Object> unmatchedEntry : actual.entrySet()) {
-                field(unmatchedEntry.getKey(), "unexpected but found [" + unmatchedEntry.getValue() + "]");
-            }
-        }
-
-        private void compareLists(List<Object> actual, List<Object> expected) {
-            int i = 0;
-            while (i < actual.size() && i < expected.size()) {
-                compare(i, actual.get(i), expected.get(i));
-                i++;
-            }
-            if (actual.size() == expected.size()) {
-                return;
-            }
-            indent();
-            if (actual.size() < expected.size()) {
-                message.append("expected [").append(expected.size() - i).append("] more entries\n");
-                return;
-            }
-            message.append("received [").append(actual.size() - i).append("] more entries than expected\n");
-        }
-
-        private void compare(Object field, @Nullable Object actual, Object expected) {
-            if (expected instanceof Map) {
-                if (actual == null) {
-                    field(field, "expected map but not found");
-                    return;
-                }
-                if (false == actual instanceof Map) {
-                    field(field, "expected map but found [" + actual + "]");
-                    return;
-                }
-                @SuppressWarnings("unchecked")
-                Map<String, Object> expectedMap = (Map<String, Object>) expected;
-                @SuppressWarnings("unchecked")
-                Map<String, Object> actualMap = (Map<String, Object>) actual;
-                if (expectedMap.isEmpty() && actualMap.isEmpty()) {
-                    field(field, "same [empty map]");
-                    return;
-                }
-                field(field, null);
-                indent += 1;
-                compareMaps(actualMap, expectedMap);
-                indent -= 1;
-                return;
-            }
-            if (expected instanceof List) {
-                if (actual == null) {
-                    field(field, "expected list but not found");
-                    return;
-                }
-                if (false == actual instanceof List) {
-                    field(field, "expected list but found [" + actual + "]");
-                    return;
-                }
-                @SuppressWarnings("unchecked")
-                List<Object> expectedList = (List<Object>) expected;
-                @SuppressWarnings("unchecked")
-                List<Object> actualList = (List<Object>) actual;
-                if (expectedList.isEmpty() && actualList.isEmpty()) {
-                    field(field, "same [empty list]");
-                    return;
-                }
-                field(field, null);
-                indent += 1;
-                compareLists(actualList, expectedList);
-                indent -= 1;
-                return;
-            }
-            if (actual == null) {
-                field(field, "expected [" + expected + "] but not found");
-                return;
-            }
-            if (Objects.equals(expected, actual)) {
-                field(field, "same [" + expected + "]");
-                return;
-            }
-            field(field, "expected [" + expected + "] but was [" + actual + "]");
-        }
-
-        private void indent() {
-            for (int i = 0; i < indent; i++) {
-                message.append("  ");
-            }
-        }
-
-        private void field(Object name, String info) {
-            indent();
-            message.append(String.format(Locale.ROOT, "%30s: ", name));
-            if (info != null) {
-                message.append(info);
-            }
-            message.append('\n');
-        }
-    }
-}