High level REST client XContent tests are misleading

[jasontedor]

Many high-level REST client XContent tests where we use a dedicated client class (e.g., org.elasticsearch.client.core.AcknowledgeResponse) are misleading and not actually testing what occurs in production.

For example, let us consider the case of org.elasticsearch.core.AcknowledgedResponse for which XContent is tested via org.elasticsearch.client.core.AcknowledgedResponseTests. However, this test does not actually test what occurs in production.

What occurs in production is that a concrete instance of org.elasticsearch.action.support.master.AcknowledgedResponse is serialized to XContent on the REST layer, and then client side is deserialized from XContent by the HLRC. We need confidence that this translation is working.

The test that we have today based on AbstractXContentTestCase#xContentTester does not actually test this. Instead, it creates a concrete instance of org.elasticsearch.client.core.AcknowledgedResponse, converts this to XContent via non-production test code, and then checks that that XContent can be converted back to an instance of org.elasticsearch.client.core.AcknowledgedResponse. This does not give us confidence that the translation between a serialized XContent of org.elasticsearch.action.support.master.AcknowledgedResponse is properly deserialized to an instance of org.elasticsearch.client.core.AcknowledgedResponse in the HLRC. That's the production code path that we want to test.

We probably want a dedicated framework like AbstractXContentTestCase#xContentTester for this. See #39713 comment for the discussion that triggered this issue, and a test that is based on production code rather than non-production code.

[elasticmachine]

Pinging @elastic/es-core-features

[hub-cap]

Yes I agree with you. @jtibshirani brought this up to me recently-ish and we arrived at the conclusion that if the toXContent method in the test creates a server response, it can then use the server responses toXContent method to turn the data into json for testing. You can see an example here

Do you think this is enough to mimic production code? cc @martijnvg who was on the other issue as well.

[jasontedor]

Yes, that is similar to what I did (although I did not go from client to server to XContent to client, instead server to XContent to client):

elasticsearch/client/rest-high-level/src/test/java/org/elasticsearch/client/core/BroadcastResponseTests.java

Lines 65 to 76 in bae7e71

	final org.elasticsearch.action.support.broadcast.BroadcastResponse to =
	new org.elasticsearch.action.support.broadcast.BroadcastResponse(total, successful, failed, failures);
	final XContentType xContentType = randomFrom(XContentType.values());
	final BytesReference bytes = toShuffledXContent(to, xContentType, ToXContent.EMPTY_PARAMS, randomBoolean());
	final XContent xContent = XContentFactory.xContent(xContentType);
	final XContentParser parser = xContent.createParser(
	new NamedXContentRegistry(ClusterModule.getNamedXWriteables()),
	LoggingDeprecationHandler.INSTANCE,
	bytes.streamInput());
	final BroadcastResponse from = BroadcastResponse.fromXContent(parser);

[martijnvg]

Do you think this is enough to mimic production code?

At least for xpack hlrc, I think this the best we can do without pulling in xpack modules as a test dependency for hlrc tests. Maybe we should consider adding xpack-core as test dependency to hlrc? This will make the request/response unit tests much more realistic.

[hub-cap]

Cool, let me eyeball this. going from server to xContent to client is certainly less (and there may be things set on the server objects that we dont expose client side). Im thinking your approach is better, but it definitely needs to be frameworked to be sure we test additional things like inserting additional random fields into the json. Im self assigning to look at how we can do this.

[hub-cap]

also Im perfectly fine with having a test dependency on server/xpack, since we are testing that those do ser/deser properly from server (and to server, as we discussed as well @martijnvg )

[hub-cap]

Chatted w @martijnvg and we think this can be closed with those two old base tests gone.