From 38914f17edff05801ccda1f818bf7fef0cd33896 Mon Sep 17 00:00:00 2001 From: javanna Date: Mon, 19 Dec 2016 19:32:10 +0100 Subject: [PATCH] [TEST] improve ElasticsearchAssertions#assertEquivalent for ToXContent Rename the method to assertToXContentEquivalent to highlight that it's tailored to ToXContent comparisons. Rather than parsing into a map and replacing byte[] in both those maps, add custom equality assertions that recursively walk maps and lists and call Arrays.equals whenever a byte[] is encountered. --- .../action/get/GetResponseTests.java | 4 +- .../index/get/GetFieldTests.java | 4 +- .../index/get/GetResultTests.java | 4 +- .../hamcrest/ElasticsearchAssertions.java | 82 +++++++++++-------- 4 files changed, 55 insertions(+), 39 deletions(-) diff --git a/core/src/test/java/org/elasticsearch/action/get/GetResponseTests.java b/core/src/test/java/org/elasticsearch/action/get/GetResponseTests.java index 33632f58e20..0fb347dd3d9 100644 --- a/core/src/test/java/org/elasticsearch/action/get/GetResponseTests.java +++ b/core/src/test/java/org/elasticsearch/action/get/GetResponseTests.java @@ -37,7 +37,7 @@ import static org.elasticsearch.index.get.GetResultTests.copyGetResult; import static org.elasticsearch.index.get.GetResultTests.mutateGetResult; import static org.elasticsearch.index.get.GetResultTests.randomGetResult; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertEquivalent; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; public class GetResponseTests extends ESTestCase { @@ -56,7 +56,7 @@ public class GetResponseTests extends ESTestCase { assertEquals(expectedGetResponse, parsedGetResponse); //print the parsed object out and test that the output is the same as the original output BytesReference finalBytes = toXContent(parsedGetResponse, xContentType, false); - assertEquivalent(originalBytes, finalBytes, xContentType); + assertToXContentEquivalent(originalBytes, finalBytes, xContentType); //check that the source stays unchanged, no shuffling of keys nor anything like that assertEquals(expectedGetResponse.getSourceAsString(), parsedGetResponse.getSourceAsString()); diff --git a/core/src/test/java/org/elasticsearch/index/get/GetFieldTests.java b/core/src/test/java/org/elasticsearch/index/get/GetFieldTests.java index 3ca7ef727b4..1058420afa7 100644 --- a/core/src/test/java/org/elasticsearch/index/get/GetFieldTests.java +++ b/core/src/test/java/org/elasticsearch/index/get/GetFieldTests.java @@ -39,7 +39,7 @@ import java.util.function.Supplier; import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertEquivalent; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; public class GetFieldTests extends ESTestCase { @@ -72,7 +72,7 @@ public class GetFieldTests extends ESTestCase { } assertEquals(expectedGetField, parsedGetField); BytesReference finalBytes = toXContent(parsedGetField, xContentType, true); - assertEquivalent(originalBytes, finalBytes, xContentType); + assertToXContentEquivalent(originalBytes, finalBytes, xContentType); } private static GetField copyGetField(GetField getField) { diff --git a/core/src/test/java/org/elasticsearch/index/get/GetResultTests.java b/core/src/test/java/org/elasticsearch/index/get/GetResultTests.java index ff0327687da..d82c4221404 100644 --- a/core/src/test/java/org/elasticsearch/index/get/GetResultTests.java +++ b/core/src/test/java/org/elasticsearch/index/get/GetResultTests.java @@ -39,7 +39,7 @@ import java.util.function.Supplier; import static org.elasticsearch.common.xcontent.XContentHelper.toXContent; import static org.elasticsearch.index.get.GetFieldTests.randomGetField; import static org.elasticsearch.test.EqualsHashCodeTestUtils.checkEqualsAndHashCode; -import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertEquivalent; +import static org.elasticsearch.test.hamcrest.ElasticsearchAssertions.assertToXContentEquivalent; public class GetResultTests extends ESTestCase { @@ -58,7 +58,7 @@ public class GetResultTests extends ESTestCase { assertEquals(expectedGetResult, parsedGetResult); //print the parsed object out and test that the output is the same as the original output BytesReference finalBytes = toXContent(parsedGetResult, xContentType, false); - assertEquivalent(originalBytes, finalBytes, xContentType); + assertToXContentEquivalent(originalBytes, finalBytes, xContentType); //check that the source stays unchanged, no shuffling of keys nor anything like that assertEquals(expectedGetResult.sourceAsString(), parsedGetResult.sourceAsString()); } diff --git a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java index 6c23a610ebf..f542ea8306b 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java +++ b/test/framework/src/main/java/org/elasticsearch/test/hamcrest/ElasticsearchAssertions.java @@ -47,7 +47,6 @@ import org.elasticsearch.cluster.block.ClusterBlock; import org.elasticsearch.cluster.block.ClusterBlockException; import org.elasticsearch.cluster.metadata.IndexTemplateMetaData; import org.elasticsearch.common.Nullable; -import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.io.stream.NamedWriteableAwareStreamInput; @@ -56,6 +55,8 @@ import org.elasticsearch.common.io.stream.StreamInput; import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Streamable; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.rest.RestStatus; @@ -76,6 +77,7 @@ import java.nio.file.Path; import java.util.ArrayList; import java.util.Arrays; import java.util.HashSet; +import java.util.Iterator; import java.util.List; import java.util.Locale; import java.util.Map; @@ -772,60 +774,74 @@ public class ElasticsearchAssertions { } /** - * Asserts that the provided {@link BytesReference}s hold the same content. The comparison is done between the map - * representation of the provided objects. + * Asserts that the provided {@link BytesReference}s created through + * {@link org.elasticsearch.common.xcontent.ToXContent#toXContent(XContentBuilder, ToXContent.Params)} hold the same content. + * The comparison is done by parsing both into a map and comparing those two, so that keys ordering doesn't matter. + * Also binary values (byte[]) are properly compared through arrays comparisons. */ - public static void assertEquivalent(BytesReference expected, BytesReference actual, XContentType xContentType) throws IOException { + public static void assertToXContentEquivalent(BytesReference expected, BytesReference actual, XContentType xContentType) + throws IOException { //we tried comparing byte per byte, but that didn't fly for a couple of reasons: //1) whenever anything goes through a map while parsing, ordering is not preserved, which is perfectly ok //2) Jackson SMILE parser parses floats as double, which then get printed out as double (with double precision) + //Note that byte[] holding binary values need special treatment as they need to be properly compared item per item. try (XContentParser actualParser = xContentType.xContent().createParser(actual)) { Map actualMap = actualParser.map(); - replaceBytesArrays(actualMap); try (XContentParser expectedParser = xContentType.xContent().createParser(expected)) { Map expectedMap = expectedParser.map(); - replaceBytesArrays(expectedMap); - assertEquals(expectedMap, actualMap); + assertMapEquals(expectedMap, actualMap); } } } /** - * Recursively navigates through the provided map argument and replaces every byte[] with a corresponding BytesArray object holding - * the original byte[]. This helps maps to maps comparisons as arrays need to be compared using Arrays.equals otherwise their - * references are compared, which is what happens in {@link java.util.AbstractMap#equals(Object)}. + * Compares two maps recursively, using arrays comparisons for byte[] through Arrays.equals(byte[], byte[]) */ @SuppressWarnings("unchecked") - private static void replaceBytesArrays(Map map) { - for (Map.Entry entry : map.entrySet()) { - Object value = entry.getValue(); - if (value instanceof byte[]) { - map.put(entry.getKey(), new BytesArray((byte[]) value)); - } else if (value instanceof Map) { - replaceBytesArrays((Map) value); - } else if (value instanceof List) { - List list = (List) value; - replaceBytesArrays(list); + private static void assertMapEquals(Map expected, Map actual) { + assertEquals(expected.size(), actual.size()); + for (Map.Entry expectedEntry : expected.entrySet()) { + String expectedKey = expectedEntry.getKey(); + Object expectedValue = expectedEntry.getValue(); + if (expectedValue == null) { + assertTrue(actual.get(expectedKey) == null && actual.containsKey(expectedKey)); + } else { + Object actualValue = actual.get(expectedKey); + assertObjectEquals(expectedValue, actualValue); } } } /** - * Recursively navigates through the provided list argument and replaces every byte[] with a corresponding BytesArray object holding - * the original byte[]. This helps maps to maps comparisons as arrays need to be compared using Arrays.equals otherwise their - * references are compared, which is what happens in {@link java.util.AbstractMap#equals(Object)}. + * Compares two lists recursively, but using arrays comparisons for byte[] through Arrays.equals(byte[], byte[]) */ @SuppressWarnings("unchecked") - private static void replaceBytesArrays(List list) { - for (int i = 0; i < list.size(); i++) { - Object object = list.get(i); - if (object instanceof byte[]) { - list.set(i, new BytesArray((byte[]) object)); - } else if (object instanceof Map) { - replaceBytesArrays((Map) object); - } else if (object instanceof List) { - replaceBytesArrays((List) object); - } + private static void assertListEquals(List expected, List actual) { + assertEquals(expected.size(), actual.size()); + Iterator actualIterator = actual.iterator(); + for (Object expectedValue : expected) { + Object actualValue = actualIterator.next(); + assertObjectEquals(expectedValue, actualValue); + } + } + + /** + * Compares two objects, recursively walking eventual maps and lists encountered, and using arrays comparisons + * for byte[] through Arrays.equals(byte[], byte[]) + */ + @SuppressWarnings("unchecked") + private static void assertObjectEquals(Object expected, Object actual) { + if (expected instanceof Map) { + assertThat(actual, instanceOf(Map.class)); + assertMapEquals((Map) expected, (Map) actual); + } else if (expected instanceof List) { + assertListEquals((List) expected, (List) actual); + } else if (expected instanceof byte[]) { + //byte[] is really a special case for binary values when comparing SMILE and CBOR, arrays of other types + //don't need to be handled. Ordinary arrays get parsed as lists. + assertArrayEquals((byte[]) expected, (byte[]) actual); + } else { + assertEquals(expected, actual); } } }