Add realistic hlrc request serialization test base class and #40362

martijnvg · 2019-03-22T13:27:55Z

change hlrc ccr request tests to use AbstractRequestTestCase base class.

This way the request classes are tested in a more realistic setting.
Note this change also adds a test dependency on xpack core module.

Similar to #39844 but then for hlrc request serialization tests.
Relates to #39745

change hlrc ccr request tests to use AbstractRequestTestCase base class. This way the request classes are tested in a more realistic setting. Note this change also adds a test dependency on xpack core module. Similar to elastic#39844 but then for hlrc request serialization tests. Relates to elastic#39745

elasticmachine · 2019-03-22T13:27:57Z

Pinging @elastic/es-core-features

…_ccr

alpar-t · 2019-03-25T14:39:16Z

client/rest-high-level/build.gradle

@@ -66,6 +66,9 @@ dependencies {
  testCompile "org.elasticsearch:rest-api-spec:${version}"

  restSpec "org.elasticsearch:rest-api-spec:${version}"
+  // Needed for serialization tests: 
+  // (In order to serialize a server side class to a client side class or the other way around)
+  testCompile "org.elasticsearch.plugin:x-pack-core:${version}"


This is a test dependency but I think it's still one that we have no precedent for.
Maybe it would be better to have these tests somewhere in x-pack/qa ?

If these tests were integration tests then I would agree. However since these tests are unit tests, I think they should stay next to the code that these tests are testing.

I think a test dep on xpack is perfectly fine WRT unit tests here. I agree w/ @martijnvg but thank you for having the eagle eye @atorok and bringing this up, just because it is definitely a major dependency.

martijnvg · 2019-03-26T07:07:28Z

@elasticmachine run elasticsearch-ci/2

hub-cap

SO much cleaner. Ive left one Q about the named writables, and this is approve-worthy pending an answer there.

hub-cap · 2019-04-05T15:06:16Z

client/rest-high-level/build.gradle

@@ -66,6 +66,9 @@ dependencies {
  testCompile "org.elasticsearch:rest-api-spec:${version}"

  restSpec "org.elasticsearch:rest-api-spec:${version}"
+  // Needed for serialization tests: 
+  // (In order to serialize a server side class to a client side class or the other way around)
+  testCompile "org.elasticsearch.plugin:x-pack-core:${version}"


I think a test dep on xpack is perfectly fine WRT unit tests here. I agree w/ @martijnvg but thank you for having the eagle eye @atorok and bringing this up, just because it is definitely a major dependency.

hub-cap · 2019-04-05T15:08:12Z

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

+ */
+public abstract class AbstractRequestTestCase<C extends ToXContent, S> extends ESTestCase {
+
+    private static final int NUMBER_OF_TEST_RUNS = 20;


should this be overridable?

We can make it overridable. However all the other parsing tests don't do this either and have the number of tests runs also fixed to 20. So I think for now this is ok?

Do we need this? Our tests run a bazillion times a day in CI. Is 20x a bazillion buying us anything?

Do we need this? Our tests run a bazillion times a day in CI. Is 20x a bazillion buying us anything?

Good question. We do this in the other xcontent parsing tests too. I personally like like this during development, so that silly mistakes are caught before I merge into master. On the other hand, I can also run the tests with -Dtests.iters=N flag. Like you say we keep paying the price until these tests are no longer run in CI. So from that perspective it makes sense to remove the loop. If we do this here then we should also do this in the other xcontent parsing tests. wdyt?

++ im fine w/ removing. I develop using tests.iters quite typically on the hlrc unit tests.

I've removed the iterations in this base class and AbstractResponseTestCase: e5b1ca7

If we do this here then we should also do this in the other xcontent parsing tests. wdyt?

+1

hub-cap · 2019-04-05T15:09:46Z

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

+
+            final XContent xContent = XContentFactory.xContent(xContentType);
+            final XContentParser parser = xContent.createParser(
+                new NamedXContentRegistry(ClusterModule.getNamedXWriteables()),


is this the only named writeables we will need for these tests?

I turns out we need no named xcontents, at least for now. So I will change this to use an empty registry. I like to be careful requiring the usage of named xcontent on the hlrc side for now.

agree. good call.

…_ccr

hub-cap

changed hlrc ccr request tests to use AbstractRequestTestCase base class. This way the request classes are tested in a more realistic setting. Note this change also adds a test dependency on xpack core module. Similar to #39844 but then for hlrc request serialization tests. Removed iterators from hlrc parsing tests. Use empty xcontent registries. Relates to #39745

…#40362) changed hlrc ccr request tests to use AbstractRequestTestCase base class. This way the request classes are tested in a more realistic setting. Note this change also adds a test dependency on xpack core module. Similar to elastic#39844 but then for hlrc request serialization tests. Removed iterators from hlrc parsing tests. Use empty xcontent registries. Relates to elastic#39745

martijnvg added >test Issues or PRs that are addressing/adding tests :Core/Features/Java High Level REST Client v8.0.0 v7.2.0 labels Mar 22, 2019

Merge remote-tracking branch 'es/master' into realistic_request_tests…

d20824c

…_ccr

alpar-t reviewed Mar 25, 2019

View reviewed changes

martijnvg requested a review from hub-cap April 3, 2019 11:40

hub-cap reviewed Apr 5, 2019

View reviewed changes

martijnvg added 2 commits April 5, 2019 20:49

use empty named xcontent registry

2b464b6

Merge remote-tracking branch 'es/master' into realistic_request_tests…

3713027

…_ccr

martijnvg requested a review from hub-cap April 5, 2019 18:50

martijnvg added 4 commits April 8, 2019 07:09

Merge remote-tracking branch 'es/master' into realistic_request_tests…

811a796

…_ccr

removed iterations

e5b1ca7

removed redundant test dependency

a1b579b

Merge remote-tracking branch 'es/master' into realistic_request_tests…

6095403

…_ccr

hub-cap approved these changes Apr 10, 2019

View reviewed changes

martijnvg merged commit 99ea830 into elastic:master Apr 10, 2019

martijnvg mentioned this pull request May 22, 2019

Run AbstractReq/RespTest parsing tests multiple times #42369

Closed

romseygeek mentioned this pull request May 23, 2019

Drop the number of default iterations in AbstractQueryTestCase to 1 #42429

Closed

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add realistic hlrc request serialization test base class and #40362

Add realistic hlrc request serialization test base class and #40362

martijnvg commented Mar 22, 2019

elasticmachine commented Mar 22, 2019

alpar-t Mar 25, 2019

martijnvg Mar 26, 2019

hub-cap Apr 5, 2019

martijnvg commented Mar 26, 2019

hub-cap left a comment

hub-cap Apr 5, 2019

hub-cap Apr 5, 2019

martijnvg Apr 5, 2019

jasontedor Apr 6, 2019

martijnvg Apr 6, 2019

hub-cap Apr 6, 2019

martijnvg Apr 8, 2019

jasontedor Apr 8, 2019

hub-cap Apr 5, 2019

martijnvg Apr 5, 2019

hub-cap Apr 6, 2019

hub-cap left a comment

Add realistic hlrc request serialization test base class and #40362

Add realistic hlrc request serialization test base class and #40362

Conversation

martijnvg commented Mar 22, 2019

elasticmachine commented Mar 22, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

martijnvg commented Mar 26, 2019

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap left a comment

Choose a reason for hiding this comment