diff --git a/libs/geo/src/test/java/org/elasticsearch/geometry/BaseGeometryTestCase.java b/libs/geo/src/test/java/org/elasticsearch/geometry/BaseGeometryTestCase.java index ae2dd17e428..9cf05ca1a00 100644 --- a/libs/geo/src/test/java/org/elasticsearch/geometry/BaseGeometryTestCase.java +++ b/libs/geo/src/test/java/org/elasticsearch/geometry/BaseGeometryTestCase.java @@ -21,7 +21,6 @@ package org.elasticsearch.geometry; import org.elasticsearch.ElasticsearchException; import org.elasticsearch.Version; -import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.geometry.utils.GeographyValidator; import org.elasticsearch.geometry.utils.WellKnownText; import org.elasticsearch.test.AbstractWireTestCase; @@ -42,12 +41,6 @@ abstract class BaseGeometryTestCase extends AbstractWireTest protected abstract T createTestInstance(boolean hasAlt); - @Override - protected Writeable.Reader instanceReader() { - throw new IllegalStateException("shouldn't be called in this test"); - } - - @SuppressWarnings("unchecked") @Override protected T copyInstance(T instance, Version version) throws IOException { diff --git a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponseTests.java b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponseTests.java index 5d6b17ac865..bcc64721203 100644 --- a/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponseTests.java +++ b/server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponseTests.java @@ -19,8 +19,8 @@ package org.elasticsearch.action.admin.indices.rollover; -import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.Version; +import org.elasticsearch.common.io.stream.Writeable; import org.elasticsearch.common.unit.ByteSizeValue; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.XContentParser; diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractWireSerializingTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractWireSerializingTestCase.java index cb7f5ff4a22..ce1e5f7ce97 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractWireSerializingTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractWireSerializingTestCase.java @@ -19,14 +19,26 @@ package org.elasticsearch.test; import org.elasticsearch.Version; +import org.elasticsearch.common.io.stream.StreamInput; +import org.elasticsearch.common.io.stream.StreamOutput; import org.elasticsearch.common.io.stream.Writeable; import java.io.IOException; +/** + * Standard test case for testing the wire serialization of subclasses of {@linkplain Writeable}. + */ public abstract class AbstractWireSerializingTestCase extends AbstractWireTestCase { + /** + * Returns a {@link Writeable.Reader} that can be used to de-serialize the instance + */ + protected abstract Writeable.Reader instanceReader(); + /** + * Copy the {@link Writeable} by round tripping it through {@linkplain StreamInput} and {@linkplain StreamOutput}. + */ @Override - protected T copyInstance(T instance, Version version) throws IOException { + protected final T copyInstance(T instance, Version version) throws IOException { return copyWriteable(instance, getNamedWriteableRegistry(), instanceReader(), version); } } diff --git a/test/framework/src/main/java/org/elasticsearch/test/AbstractWireTestCase.java b/test/framework/src/main/java/org/elasticsearch/test/AbstractWireTestCase.java index 3e58dc8809d..3997db194e9 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/AbstractWireTestCase.java +++ b/test/framework/src/main/java/org/elasticsearch/test/AbstractWireTestCase.java @@ -27,6 +27,10 @@ import org.elasticsearch.common.io.stream.Writeable; import java.io.IOException; import java.util.Collections; +/** + * Standard test case for testing wire serialization. If the class being tested + * extends {@link Writeable} then prefer extending {@link AbstractWireSerializingTestCase}. + */ public abstract class AbstractWireTestCase extends ESTestCase { protected static final int NUMBER_OF_TEST_RUNS = 20; @@ -38,11 +42,6 @@ public abstract class AbstractWireTestCase extends ESTestCase { */ protected abstract T createTestInstance(); - /** - * Returns a {@link Writeable.Reader} that can be used to de-serialize the instance - */ - protected abstract Writeable.Reader instanceReader(); - /** * Returns an instance which is mutated slightly so it should not be equal * to the given instance. @@ -73,18 +72,26 @@ public abstract class AbstractWireTestCase extends ESTestCase { } /** - * Serialize the given instance and asserts that both are equal + * Serialize the given instance and asserts that both are equal. */ - protected final T assertSerialization(T testInstance) throws IOException { - return assertSerialization(testInstance, Version.CURRENT); + protected final void assertSerialization(T testInstance) throws IOException { + assertSerialization(testInstance, Version.CURRENT); } - protected final T assertSerialization(T testInstance, Version version) throws IOException { + /** + * Assert that instances copied at a particular version are equal. The version is useful + * for sanity checking the backwards compatibility of the wire. It isn't a substitute for + * real backwards compatibility tests but it is *so* much faster. + */ + protected final void assertSerialization(T testInstance, Version version) throws IOException { T deserializedInstance = copyInstance(testInstance, version); assertEqualInstances(testInstance, deserializedInstance); - return deserializedInstance; } + /** + * Assert that two instances are equal. This is intentionally not final so we can override + * how equality is checked. + */ protected void assertEqualInstances(T expectedInstance, T newInstance) { assertNotSame(newInstance, expectedInstance); assertEquals(expectedInstance, newInstance); @@ -95,6 +102,11 @@ public abstract class AbstractWireTestCase extends ESTestCase { return copyInstance(instance, Version.CURRENT); } + /** + * Copy the instance as by reading and writing using the code specific to the provided version. + * The version is useful for sanity checking the backwards compatibility of the wire. It isn't + * a substitute for real backwards compatibility tests but it is *so* much faster. + */ protected abstract T copyInstance(T instance, Version version) throws IOException; /** diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/AbstractSqlWireSerializingTestCase.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/AbstractSqlWireSerializingTestCase.java index 057cc76ee02..6f8dae854f4 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/AbstractSqlWireSerializingTestCase.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/AbstractSqlWireSerializingTestCase.java @@ -32,6 +32,11 @@ public abstract class AbstractSqlWireSerializingTestCase ex } } } + + /** + * Returns a {@link Writeable.Reader} that can be used to de-serialize the instance + */ + protected abstract Writeable.Reader instanceReader(); protected ZoneId instanceZoneId(T instance) { return randomSafeZone(); diff --git a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/session/ListCursorTests.java b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/session/ListCursorTests.java index ac14465451e..2fd88e3c0bc 100644 --- a/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/session/ListCursorTests.java +++ b/x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/session/ListCursorTests.java @@ -7,15 +7,14 @@ package org.elasticsearch.xpack.sql.session; import org.elasticsearch.Version; import org.elasticsearch.common.io.stream.NamedWriteableRegistry; -import org.elasticsearch.common.io.stream.Writeable.Reader; -import org.elasticsearch.test.AbstractWireSerializingTestCase; +import org.elasticsearch.test.AbstractWireTestCase; import java.io.IOException; import java.util.ArrayList; import java.util.Arrays; import java.util.List; -public class ListCursorTests extends AbstractWireSerializingTestCase { +public class ListCursorTests extends AbstractWireTestCase { public static ListCursor randomPagingListCursor() { int size = between(1, 20); int depth = between(1, 20); @@ -45,17 +44,12 @@ public class ListCursorTests extends AbstractWireSerializingTestCase return randomPagingListCursor(); } - @Override - protected Reader instanceReader() { - return ListCursor::new; - } - @Override protected ListCursor copyInstance(ListCursor instance, Version version) throws IOException { /* Randomly choose between internal protocol round trip and String based * round trips used to toXContent. */ if (randomBoolean()) { - return super.copyInstance(instance, version); + return copyWriteable(instance, getNamedWriteableRegistry(), ListCursor::new, version); } return (ListCursor) Cursors.decodeFromString(Cursors.encodeToString(instance, randomZone())); }