Clean up wire test case a bit (#50627)

* Adds JavaDoc to `AbstractWireTestCase` and
`AbstractWireSerializingTestCase` so it is more obvious you should prefer
the latter if you have a choice
* Moves the `instanceReader` method out of `AbstractWireTestCase` becaue
it is no longer used.
* Marks a bunch of methods final so it is more obvious which classes are
for what.
* Cleans up the side effects of the above.

Author: nik9000

libs/geo/src/test/java/org/elasticsearch/geometry/BaseGeometryTestCase.java

@@ -21,7 +21,6 @@
 
 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 @@ protected final T createTestInstance() {
 
     protected abstract T createTestInstance(boolean hasAlt);
 
-    @Override
-    protected Writeable.Reader<T> instanceReader() {
-        throw new IllegalStateException("shouldn't be called in this test");
-    }
-
-
     @SuppressWarnings("unchecked")
     @Override
     protected T copyInstance(T instance, Version version) throws IOException {

server/src/test/java/org/elasticsearch/action/admin/indices/rollover/RolloverResponseTests.java

@@ -19,20 +19,18 @@
 
 package org.elasticsearch.action.admin.indices.rollover;
 
-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.test.AbstractWireTestCase;
+import org.elasticsearch.test.AbstractWireSerializingTestCase;
 
-import java.io.IOException;
 import java.util.ArrayList;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
 import java.util.function.Supplier;
 
-public class RolloverResponseTests extends AbstractWireTestCase<RolloverResponse> {
+public class RolloverResponseTests extends AbstractWireSerializingTestCase<RolloverResponse> {
 
     @Override
     protected RolloverResponse createTestInstance() {
@@ -65,11 +63,6 @@ protected Writeable.Reader<RolloverResponse> instanceReader() {
         return RolloverResponse::new;
     }
 
-    @Override
-    protected RolloverResponse copyInstance(RolloverResponse instance, Version version) throws IOException {
-        return copyWriteable(instance, writableRegistry(), RolloverResponse::new);
-    }
-
     @Override
     protected RolloverResponse mutateInstance(RolloverResponse response) {
         int i = randomIntBetween(0, 6);

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<T extends Writeable> extends AbstractWireTestCase<T> {
+    /**
+     * Returns a {@link Writeable.Reader} that can be used to de-serialize the instance
+     */
+    protected abstract Writeable.Reader<T> 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);
     }
 }

test/framework/src/main/java/org/elasticsearch/test/AbstractWireTestCase.java

@@ -27,6 +27,10 @@
 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<T> extends ESTestCase {
 
     protected static final int NUMBER_OF_TEST_RUNS = 20;
@@ -38,11 +42,6 @@ public abstract class AbstractWireTestCase<T> extends ESTestCase {
      */
     protected abstract T createTestInstance();
 
-    /**
-     * Returns a {@link Writeable.Reader} that can be used to de-serialize the instance
-     */
-    protected abstract Writeable.Reader<T> instanceReader();
-
     /**
      * Returns an instance which is mutated slightly so it should not be equal
      * to the given instance.
@@ -73,18 +72,26 @@ public final void testSerialization() throws IOException {
     }
 
     /**
-     * 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 @@ protected final T copyInstance(T instance) throws IOException {
         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;
 
     /**

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/AbstractSqlWireSerializingTestCase.java

@@ -32,6 +32,11 @@ protected T copyInstance(T instance, Version version) throws IOException {
             }
         }
     }
+    
+    /**
+     * Returns a {@link Writeable.Reader} that can be used to de-serialize the instance
+     */
+    protected abstract Writeable.Reader<T> instanceReader();
 
     protected ZoneId instanceZoneId(T instance) {
         return randomSafeZone();

x-pack/plugin/sql/src/test/java/org/elasticsearch/xpack/sql/session/ListCursorTests.java

@@ -7,15 +7,14 @@
 
 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<ListCursor> {
+public class ListCursorTests extends AbstractWireTestCase<ListCursor> {
     public static ListCursor randomPagingListCursor() {
         int size = between(1, 20);
         int depth = between(1, 20);
@@ -45,17 +44,12 @@ protected ListCursor createTestInstance() {
         return randomPagingListCursor();
     }
 
-    @Override
-    protected Reader<ListCursor> 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()));
     }