[Test] Add test for custom requests in High Level Rest Client by tlrx · Pull Request #25106 · elastic/elasticsearch

tlrx · 2017-06-07T14:52:22Z

This pull request adds a test that tests and demonstrates how RestHighLevelClient can be extended to support custom requests and responses.

This commit adds a test that tests and demonstrates how {@link RestHighLevelClient} can be extended to support custom requests and responses.

javanna · 2017-06-07T15:39:15Z

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

+    private CustomRestClient restHighLevelClient;
+
+    @Before
+    public void iniClients() throws IOException {


initClients

javanna

left a couple of comments

javanna · 2017-06-07T15:40:54Z

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

+        assertEquals(expectedStatus(length), customResponse.status());
+    }
+
+    private Void performRequestAsync(HttpEntity httpEntity, ResponseListener responseListener) {


why Void instead of void?

Because Mockito's doAnswer() method needs a type to be able to stub a method that returns void (ie RestClient's performRequestAsync method).

javanna · 2017-06-07T15:42:28Z

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

+        return null;
+    }
+
+    private Response performRequest(HttpEntity httpEntity) throws IOException {


why do we need these two methods? can we use the internal RestClient instead?

Why do you mean by using the internal RestClient? The test uses a mocked RestClient and executes these methods when the RestClient's performRequest and performRequestAsync methods are called with parameters that match the Matchers configured in initClient(). I might be missing something, but I think this test mocks the right RestClient object ?

right, I had totally misunderstood how the test works

javanna · 2017-06-08T13:08:23Z

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

+import static org.mockito.Mockito.mock;
+
+/**
+ * Test and demonstrates how {@link RestHighLevelClient} can be extended to support custom requests and responses.


add "against custom endpoints" at the end of the sentence?

javanna · 2017-06-08T13:11:55Z

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

+        }
+    }
+
+    static class CustomRequest extends ActionRequest implements ToXContentObject {


maybe we could trim down the test and just reuse existing requests? would it make the test less useful? I think the important bit is that you can add the methods to the custom client class, and that those methods can make use of the existing infra.

I'd like to keep the custom request and custom response because it echoes the custom/customAsync methods. But I can trim down the test by not printing out xcontent request and response body which are not the point here. I'll try that and you'll tell me if you still want to reuse existing requests (I'm fine with reusing MainRequest/MainResponse for example)

javanna · 2017-06-08T13:13:22Z

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

+        }
+
+        public void customAsync(CustomRequest customRequest, ActionListener<CustomResponse> listener, Header... headers) {
+            performRequestAsync(customRequest, this::toRequest, this::toResponse, listener, emptySet(), headers);


I think that what confuses me here is that we call performRequest and performRequestAsync here, why do we mock the rest client then? Wouldn't it be better to test that RestHighLevelClient subclasses can use performRequestAndParseEntity and performRequestAsyncAndParseEntity (which are private at the moment)

I think that what confuses me here is that we call performRequest and performRequestAsync here, why do we mock the rest client then?

I agree, this is confusing. I you noticed it calls the performRequest method from the RestHighLevelClient and not the method from the test. I renamed the test method so that it's not confusing.

Wouldn't it be better to test that RestHighLevelClient subclasses can use performRequestAndParseEntity and performRequestAsyncAndParseEntity (which are private at the moment)

I don't know, it really depends of what the client want to do with the HTTP response. The performRequest/performRequestAsync are nice because we get back the Response from the low level rest client and so we can extract headers and still parse the entity with the parseEntity method. On the other side, exposing performRequestAndParseEntity and performRequestAsyncAndParseEntity would be useful too, so maybe both of them should be protected.

tlrx · 2017-06-08T14:45:38Z

@javanna Thanks for your review! I applied your feedback and trimmed down the test, please let me know what you think.

javanna

I left a couple of small comments

javanna · 2017-06-09T09:40:20Z

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

+            super(restClient);
+        }
+
+        public CustomResponse custom(CustomRequest customRequest, Header... headers) throws IOException {


shall we also have two additional methods that use performRequestAsyncAndParseEntity and performRequestAndParseEntity (which should be made protected for that)?

javanna · 2017-06-09T09:45:24Z

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

+        }
+
+        public CustomResponse custom(CustomRequest customRequest, Header... headers) throws IOException {
+            return performRequest(customRequest, this::toRequest, this::toResponse, emptySet(), headers);


I think that we need to make this method and performRequestAsync protected rather than package private. Can you add for this and the other needed methods a test like this one: https://github.com/elastic/elasticsearch/blob/master/client/rest/src/test/java/org/elasticsearch/client/HeapBufferedAsyncResponseConsumerTests.java#L121 ?

tlrx · 2017-06-09T12:22:31Z

@javanna Thanks for your review. I changed the code and dropped the custom objects and reused existing request/response as you suggested before.

javanna

LGTM thanks @tlrx

This commit adds a test that tests and demonstrates how {@link RestHighLevelClient} can be extended to support custom endpoint.

tlrx · 2017-06-09T15:24:28Z

Thanks @javanna!

I added a missing @SuppressForbidden annotation on the testMethodsVisibility() because there's no other way to check for protected methods than using the forbidden getDeclaredMethods() method.

* master: (53 commits) Log checkout so SHA is known Add link to community Rust Client (elastic#22897) "shard started" should show index and shard ID (elastic#25157) await fix testWithRandomException Change BWC versions on create index response Return the index name on a create index response Remove incorrect bwc branch logic from master Correctly format arrays in output [Test] Extending parsing checks for SearchResponse (elastic#25148) Scripting: Change keys for inline/stored scripts to source/id (elastic#25127) [Test] Add test for custom requests in High Level Rest Client (elastic#25106) nested: In case of a single type the _id field should be added to the nested document instead of _uid field. `type` and `id` are lost upon serialization of `Translog.Delete`. (elastic#24586) fix highlighting docs Fix NPE in token_count datatype with null value (elastic#25046) Remove the postings highlighter and make unified the default highlighter choice (elastic#25028) [Test] Adding test for parsing SearchShardFailure leniently (elastic#25144) Fix typo in shards.asciidoc (elastic#25143) List Hibernate Search (elastic#25145) [DOCS] update maxRetryTimeout in java REST client usage page ...

* master: (80 commits) Test: remove faling test that relies on merge order Log checkout so SHA is known Add link to community Rust Client (elastic#22897) "shard started" should show index and shard ID (elastic#25157) await fix testWithRandomException Change BWC versions on create index response Return the index name on a create index response Remove incorrect bwc branch logic from master Correctly format arrays in output [Test] Extending parsing checks for SearchResponse (elastic#25148) Scripting: Change keys for inline/stored scripts to source/id (elastic#25127) [Test] Add test for custom requests in High Level Rest Client (elastic#25106) nested: In case of a single type the _id field should be added to the nested document instead of _uid field. `type` and `id` are lost upon serialization of `Translog.Delete`. (elastic#24586) fix highlighting docs Fix NPE in token_count datatype with null value (elastic#25046) Remove the postings highlighter and make unified the default highlighter choice (elastic#25028) [Test] Adding test for parsing SearchShardFailure leniently (elastic#25144) Fix typo in shards.asciidoc (elastic#25143) List Hibernate Search (elastic#25145) ...

* master: (1889 commits) Test: remove faling test that relies on merge order Log checkout so SHA is known Add link to community Rust Client (elastic#22897) "shard started" should show index and shard ID (elastic#25157) await fix testWithRandomException Change BWC versions on create index response Return the index name on a create index response Remove incorrect bwc branch logic from master Correctly format arrays in output [Test] Extending parsing checks for SearchResponse (elastic#25148) Scripting: Change keys for inline/stored scripts to source/id (elastic#25127) [Test] Add test for custom requests in High Level Rest Client (elastic#25106) nested: In case of a single type the _id field should be added to the nested document instead of _uid field. `type` and `id` are lost upon serialization of `Translog.Delete`. (elastic#24586) fix highlighting docs Fix NPE in token_count datatype with null value (elastic#25046) Remove the postings highlighter and make unified the default highlighter choice (elastic#25028) [Test] Adding test for parsing SearchShardFailure leniently (elastic#25144) Fix typo in shards.asciidoc (elastic#25143) List Hibernate Search (elastic#25145) ...

tlrx added 2 commits June 7, 2017 16:34

[Test] Add test for a custom RestHighLevelClient

1a8eb85

This commit adds a test that tests and demonstrates how {@link RestHighLevelClient} can be extended to support custom requests and responses.

Fix checkstyle violation

cb8d899

tlrx added :Java High Level REST Client >test Issues or PRs that are addressing/adding tests v5.5.0 v6.0.0 labels Jun 7, 2017

tlrx requested a review from javanna June 7, 2017 14:52

javanna added v5.6.0 and removed v5.5.0 labels Jun 7, 2017

javanna reviewed Jun 7, 2017

View reviewed changes

javanna suggested changes Jun 7, 2017

View reviewed changes

javanna reviewed Jun 8, 2017

View reviewed changes

Apply feedback

d77bfaa

javanna suggested changes Jun 9, 2017

View reviewed changes

Apply feedback

0713822

javanna approved these changes Jun 9, 2017

View reviewed changes

Add @SuppressForbidden

e2770b0

tlrx merged commit 2950210 into elastic:master Jun 9, 2017

tlrx added a commit that referenced this pull request Jun 9, 2017

[Test] Add test for custom requests in High Level Rest Client (#25106)

de6a56b

This commit adds a test that tests and demonstrates how {@link RestHighLevelClient} can be extended to support custom endpoint.

tlrx deleted the custom-high-level-rest-client branch June 9, 2017 15:22

tlrx mentioned this pull request Jun 15, 2017

Allow RestHighLevelClient to use plugins #25024

Closed

colings86 added v6.0.0-beta1 and removed v6.0.0 labels Aug 3, 2017

Conversation

tlrx commented Jun 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Jun 8, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tlrx commented Jun 9, 2017

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

tlrx commented Jun 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants