Unit testing + bug fixing in GRPC Adapter by AndresRoblesMX · Pull Request #79 · dapr/java-sdk

AndresRoblesMX · 2020-01-10T18:58:30Z

Description

Adding Mockito plugin for mocking final classes.
Increasing test coverage for ObjectSerializer
Fixing bug in GRPC Adapter while creating the envelopes, found during unit testing.

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

Code compiles correctly
Created/updated tests
Extended the documentation

artursouza · 2020-01-10T22:34:18Z

   * @throws UnsupportedOperationException every time is called.
   */
  public <T> Mono<Void> invokeService(String verb, String appId, String method, T request) {
-    return Mono.error(new UnsupportedOperationException("Operation not supported for GRPC"));


I have an implementation for this. It just needs to call into the other one, no need to duplicate code.

artursouza · 2020-01-10T22:37:56Z

-      }
-    }
-    return mapOptions;
+  private <K> DaprProtos.InvokeServiceEnvelope getInvodeServceEnvelope(


Typos + "get" gives the impression that it is just fetching an existing field. Please, rename to buildInvokeServiceEnvelope

AndresRoblesMX · 2020-01-11T07:48:57Z

@artursouza can you please take another look at this PR?

artursouza

This is great! My comments are for minor changes.

artursouza · 2020-01-12T00:21:51Z

-   * @return Map for the state options.
-   * @throws IllegalAccessException Cannot extract params.
+   * Builds the object io.dapr.{@link DaprProtos.InvokeServiceEnvelope} to be send based on the parameters.
+   * @param verb


Comments on params.

artursouza · 2020-01-12T00:24:21Z

  enum HttpMethods { GET, PUT, POST, DELETE; }

+  static class Response {
+    private String body;


Send as byte[], please. That should be the raw response.

artursouza · 2020-01-12T00:24:57Z

              }
+
+              Map<String, String> mapHeaders = new HashMap<>();
              String result = response.body().string();


I have a change making this byte[], let's use that.

artursouza · 2020-01-12T00:25:32Z

  private final String key;
+  /**
+   * The ETag to be used
+   * For REDIS ONLY this must be an integer


Make it more generic, saying that in some state stores (like Redis), this must be an integer.

artursouza · 2020-01-12T00:26:19Z


-  private final String consistency;
+public class StateOptions {
+  private Consistency consistency;


Make all fields final.

artursouza · 2020-01-12T00:28:01Z

+        .thenReturn(settableFuture);
+    MyObject event = new MyObject(1, "Event");
+    Mono<Void> result = adater.publishEvent("topic", event);
+    result.block();


verify callback or something.

artursouza · 2020-01-12T00:28:38Z

+    when(client.invokeService(any(DaprProtos.InvokeServiceEnvelope.class)))
+        .thenReturn(settableFuture);
+    Mono<Void> result = adater.invokeService(Verb.GET, "appId", "method", "request", null);
+    result.block();


verify callback is called.

artursouza · 2020-01-12T00:28:52Z

+        .thenReturn(settableFuture);
+    MyObject request = new MyObject(1, "Event");
+    Mono<Void> result = adater.invokeService(Verb.GET, "appId", "method", request, null);
+    result.block();


verify something.

artursouza · 2020-01-12T00:29:28Z

+        .thenReturn(settableFuture);
+    Mono<String> result = adater.invokeService(Verb.GET, "appId", "method", "request", null, String.class);
+    String strOutput = result.block();
+    assertEquals(expected, strOutput);


This should be OK since we have a result to compare.

artursouza · 2020-01-12T00:29:55Z

+    when(client.invokeBinding(any(DaprProtos.InvokeBindingEnvelope.class)))
+        .thenReturn(settableFuture);
+    Mono<Void> result = adater.invokeBinding("BindingName", "request");
+    result.block();


verify callback being called.

Increasing test coverage for ObjectSerializer Fixing bug in GRPC Adapter while creating the envelopes, found during unit testing.

…e DaprClientHttpAdapter Updating State object to match the API. Fixing Broken Unit Tests and increasing coverage for DaprClientGrpcAdapter

…more descriptive.

Increasing test coverage Fixing Merge conflicts

AndresRoblesMX · 2020-01-13T02:02:43Z

@artursouza I already addressed your comments, plus added a little extra covera, please take another look

artursouza · 2020-01-13T02:40:59Z

    String url = String.format(Constants.ACTOR_STATE_KEY_RELATIVE_URL_FORMAT, actorType, actorId, keyName);
-    return this.client.invokeAPI(DaprHttp.HttpMethods.GET.name(), url, "", null);
+    Mono<DaprHttp.Response> responseMono = this.client.invokeAPI(DaprHttp.HttpMethods.GET.name(), url, "", null);
+    return Mono.just(responseMono).flatMap(f -> {


responseMono.flatMap

not sure what you mean here

artursouza · 2020-01-13T02:43:09Z

   * @return DaprError or null if could not parse.
   */
-  private static DaprError parseDaprError(byte[] json) throws IOException {
+  private static DaprError parseDaprError(String json) {


Just throw the exception and use byte[].

artursouza · 2020-01-13T02:44:19Z

+    try {
+      return OBJECT_MAPPER.readValue(json, DaprError.class);
+    } catch (IOException e) {
+      throw new DaprException("500", "Unknown error: could not parse error json.");


DaprException should be only for exceptions sent from Dapr's runtime. Plus, the error code string does not map to HTTP error code. It is custom strings defined in the runtime code in Go.

artursouza · 2020-01-13T02:44:38Z

+            try (okhttp3.Response response = this.httpClient.newCall(request).execute()) {
              if (!response.isSuccessful()) {
-                DaprError error = this.parseDaprError(responseBody);
+                DaprError error = parseDaprError(response.body().string());


Keep it byte[]

artursouza

This is pretty good.

* Adding mockito plugin to be able to mock final classes Increasing test coverage for ObjectSerializer Fixing bug in GRPC Adapter while creating the envelopes, found during unit testing. * First step into returning Http Headers as part of the response for the DaprClientHttpAdapter Updating State object to match the API. Fixing Broken Unit Tests and increasing coverage for DaprClientGrpcAdapter * Adding documentation, fixing typos and renaming support method to be more descriptive. * Addressing PR comments Increasing test coverage Fixing Merge conflicts * Addressing PR comments

* Basic Dapr HTTP Client for Actors. * ActorTypeInformation with parsing logic + some stubs. * Fixing license. * Adding MethodContext + StateSerializer. * Add ActorRuntime (#44) * Add ActorRuntime * split DaprAsyncClient hierarchy into 2 hierarchies for the different directions of communication * Rename DaprClientBase to AbstractDaprClient * Add ActorId * Encapsulating async call in AbstractDaprClient as a single method. (#46) * Add ActorMethodInfoMap (#47) * Add ActorMethodInfoMap * Add ConverterUtils class (#49) * Add ConverterUtils + unit test * Improved async call to okhttp. Former solution just wrapped a blocking call, now we using asynch call and collect the results via callbacks. Mono's are created based on the result. This make more efficient use of the resources since nothing is blocking now. Reformatted according new checkstyle (#48) * More reformatting to 2 spaces indentation. (#50) * Add ActorTimer and related (#51) * Add ActorTimer and related * cr * ActorService + DaprStateProvider + some more. (#53) * Faster JSON building + separate auto-gen jar + tests. (#55) * closed the response objects and returning a string (#56) * Add ReminderInfo (#54) * Add ReminderInfo * remove extra var * More Actors Stuff (#57) * ActorManager * More work done. * Adding example for actor runtime service. * Implements ActorStateManager + fixes + javadocs. * ActorProxy + some refactoring (#62) * ActorManager * More work done. * Adding example for actor runtime service. * Implements ActorStateManager + fixes + javadocs. * Fix OrderManager example in order to process http 201 Change the implement the calls to DAPR using the class AbstractDaprClient because before the change the class always return Mono.Empty Implementation of the ActorAsyncProxy Change the name of the Actor Dapr Http Async Client * Update code with the changes proposed by Artur in the code review * Changes to support ActorProxy + Fixes. Co-authored-by: Juan Jose Herrera <35985447+JuanJose-Herrera@users.noreply.github.com> * Adding new methods for Http Client (#60) * Adding new methods for Http Client * Finishing implementation for HttpClient * Adding JavaDoc and Changing return Type to new Methods * Using ObjectSerializer and changing Constants values * Adding gRPC client adapter, to encapsulate gRPC logic from the user (#61) * Adding a common adapter to be exposed to users, for calling either the gRPC or the HTTP Clients. Implementing gRPC adapter based on interface. Moving ActorStateSerializer and renamed to be used as a generic utility for Serializing objects * Creating a builder for the GrpcClient Abstracting generic serialize and deserialize methods into a generic base class while leaving Actor specific stuff into the ActorStateSerializer class Adding Javadocs * Unit tests for ActorManager + fixes for Timer and Reminder. (#63) * Add UnitTest to ActorProxyImpl and JaCoCo (coverage tool) implementation (#67) * Change from unwrapMethodResponse to deserialize, because the response from an actor method is not wrapped in "Data" object Add Unit test for the ActorProxy class * #24 Implement more testing to the ActorProxyImpl and refactor how we manage the errors with Mono #20 Add the coverage tool JaCoCo, the rules are commented at this moment in order to allow to execute the examples at this moment, if we enable the rules, the compilation fails. * Enable rules for unit test coverage with a minimum of 0 in order to allow the developers to success compile, the minimum should be set to .8 before the projects ends * Use the unwrapMethodResponse of the serilizer in order to deserilize actor responses. * Reverting ActorProxyImpl.java There is not need to change ActorProxyImpl since the previous change to not wrap it with the "data" structure was incorrect. Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Starting package to handle SpringBoot integration. (#69) * Request body must be null on get for okhttp, empty body does not work… (#70) * Request body must be null on get for okhttp, empty body does not work. Also, expose the ActorStateManager methods that ought to be exposed to user. * Make ctor package private and use equals for string compare * Refactoring HTTP Client and creating Client HTTP Adapter (#68) * Refactoring HTTP Client to follow same pattern as the GRPC Client. Creating DaprClientHttpAdapter to be exposed to the users. Refactor all uses of the http Client to use the adapter instead following composition rather than inheritance. Dealing with conflicts * Renaming AppToDaprHttpAsync to AppToDaprHttpAsyncClient changed previously by mistake * Refactor Adapters to centralize all generic and actor specific functionality in the same place, having a single entry point for all communications to DAPR. Leaving GRPC Adapter implemented, but without the possibility to create an instance of it. Fixing Test cases * Addressing PR comments * PubSub + related fixes. (#71) * Fixing issue regarding GET and DELETE method (#76) * Unit tests for PubSub + JavaDocs. (#75) * Invoke service + unit tests. (#78) * Java sdk wip (#77) * Fixing issue regarding GET and DELETE method * Adding unit test for DaprHttp.java * Adding test scope * Renaming a property and fixing conflict to merge. Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * pubsub sample needs update for listener interface (#80) * Fix bindings + add samples (#81) * Fix binding and add samples * Fix sample command line * update comment for name change * lowercase d matches runtime, though it looks like it is case insensitive * Spliting Actor client and runtime into a separate jar. (#83) * Unit testing + bug fixing in GRPC Adapter (#79) * Adding mockito plugin to be able to mock final classes Increasing test coverage for ObjectSerializer Fixing bug in GRPC Adapter while creating the envelopes, found during unit testing. * First step into returning Http Headers as part of the response for the DaprClientHttpAdapter Updating State object to match the API. Fixing Broken Unit Tests and increasing coverage for DaprClientGrpcAdapter * Adding documentation, fixing typos and renaming support method to be more descriptive. * Addressing PR comments Increasing test coverage Fixing Merge conflicts * Addressing PR comments * Addressing comments regarding where to build the query parameters forthe HTTP Client calls. (#84) * Addressing comments regarding where to build the query parameters for the HTTP Client calls. * Making map immutable while also preventing a NPE * Fixing DaprHttp and its tests. (#93) * Increasing test coverage for DaprClientGrpcAdapter (#94) * Upload jacoco test coverage report to artifact storage (#95) * set jacoco report output dir * upload test report * add dapr version info * #26 Integretion Testing Initial Example (#74) * #26 Add Hello World Integration Testing working on Windows, need work to work on MAC and Linux * #26 Add new Integration Test to test DAPR state functionality * #26 Add Hello World Integration Testing working on Windows, need work to work on MAC and Linux * Update Integration Testing getting free ports automatically * #26 Refractor to use a base class for all the integration tests * #26 Make StateOptions as optional in order to not throw a null pointer exception * #26 Remove empty lines and correct the ident * Adding license to DaprIntegrationTestingRunner Co-authored-by: Young Bu Park <youngp@microsoft.com> Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Fixing and finishing unit tests (#97) * Fixing and finishing unit tests * Fixing styling Issues and adding new test * Unit tests for stateful actors + fixes. (#104) * Receiving the StateOptions on save as part of the State, to comply with the DAPR API (#105) * Receiving the StateOptions on save as part of the State, to comply with the DAPR API * #26 Add integration test for Concurrency funtionality in the states module Co-authored-by: Juan Jose Herrera <35985447+JuanJose-Herrera@users.noreply.github.com> Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Deploy Jar packages to Nexus via CI (#106) * Add SNAPSHOT suffix to version * set deployment setup * add ossrh repostiory * OSSRH setting * add deployment steps * exclude examples jar * remove condition * exclude spring boot pkg Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * deactivate should not call save (#108) Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Fixing bug, where call should be deferred instead of async (#102) * Fixing bug, where call should be deferred instead of async * Removing unused pool property Returning null as response body if empty response is found. Adding Test case for validating callback is executed when expected. * Fixing assertion based on previous changes * Adding test case to verify that the call to grpc only happens when the requestor block the thread, instead of in parallel immediately after calling the methods in the adapter. Documenting complex test case * Fixing merge conflicts * Documenting complex test case * Disable GPG sign by default (#111) * [CD] Fix gpg private key import (#113) * fix gpg key import process * add GPG_TTY env setting * revert * rename dapr-sdk to dapr-actors * fix * Fix GPG plugin * [CD] Conditional SNAPSHOT and final release (#115) * Release pkg based on tag and branch * Update build.yml * Update build.yml * Creating unit tests for HttpAdapter + Adding junit 5 dependency + Reformating code (#112) Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Removing springboot jar and adding some unit tests for ActorRuntime. (#114) * Release SNAPSHOT pkg for every build (#118) * Release SNAPSHOT build every build * fix * Fix sample (#119) Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Unit tests for actors, no state. (#117) * actor unit tests, no state clean up a bit * Rename class * Fix merge Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Moving io.dapr.runtime to test + fixing examples accordingly. (#121) * mvn integration-test doesn't work w/o this plugin (#122) * Resolving conflict for PR #110 (#130) * Renaming State domain object. Removing duplicate use of State Options. Adding custom serialize and deserialize classes for StateOptions objects. * #26 Refractor StateOptions to clean unused code * Fixing merge issues. * Fixing unit test for DaprClientHttpAdapterTest after refactoring. Co-authored-by: Juan Jose Herrera <35985447+JuanJose-Herrera@users.noreply.github.com> * Refactor StateOptions and add IT consistency testing (#110) * Renaming State domain object. Removing duplicate use of State Options. Adding custom serialize and deserialize classes for StateOptions objects. * #26 Refractor StateOptions to clean unused code * Fixing merge issues. * #26 Add integration test for states using Retry policies Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Fix IT issues, to run the integration test rember to add the parameter skipITs=false (#131) * Addresses multiple items from design review (#124) * Use byte[] for customer data and serialized data. Fixes for actor demo. * Simplifying ObjectSerializer + handling CloudEvent properly. * Split DaprClient into 3 parts to hide actor APIs. * Remove Actor "dummy" interface. * Supports custom serializer. * Change seriaizer to receive Object directly. * Handling custom serialization in State APIs + enhancements to the state APIs. * Addressing small comment on Mono chain in ActorManager. * Make serializer mandatory, throwing exception when null. * Fix arg parsing problem -Dexec.args= seems to not allow hyphens inside (#136) * Fix bug in Actor due to timer serialization. (#139) * Missing block (#140) * Install local test kafka before running build and tests (#142) * local test kafka docker-compose yaml * add kafka install step * Separate serializer for state. (#135) * Add gRPC State Integration Test (#138) * Add GRP State Integration Test * Explain ignored test cases for GRPC * Explain ignored test cases for GRPC * Update DaprClientTestBuilder.java Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Actor Activation / Deactivation Integartion Test (#144) * Adding common setup for starting dapr applications in Integration Tests. Adding integration test for Actor Activation and Reactivation. * Changing the way parameters are sent to the application running under Dapr in ITs to fix issue found in Ubuntu about wrong parsing. Moving logic of the ActivationClient into the IT class * Fixing compilation issues in test due to rebase from upstream. * Addressing PR comments * Adding more unit test to reach 80% Coverage (#149) * Adding checkstyle + fix style in sdk-actos. (#146) * Update debug instructions since LB is not needed. (#150) * Add E2E testing for bindings (#148) * Add GRP State Integration Test * Explain ignored test cases for GRPC * Explain ignored test cases for GRPC * Update DaprClientTestBuilder.java * Add binding E2E testing. * Add fix binding E2E testing after merge with new changes * Remove example comments Co-authored-by: Artur Souza <artursouza.ms@outlook.com> * Adding documentation for examples (#153) Co-authored-by: Leon Mai <lemai@microsoft.com> Co-authored-by: ji11er <57505038+ji11er@users.noreply.github.com> Co-authored-by: Juan Jose Herrera <35985447+JuanJose-Herrera@users.noreply.github.com> Co-authored-by: mestizoLopez <ing.javierlg@gmail.com> Co-authored-by: Andres Robles <15348598+AndresRoblesMX@users.noreply.github.com> Co-authored-by: Young Bu Park <youngp@microsoft.com> Co-authored-by: Marcos Reyes <59033058+marcosreyes05@users.noreply.github.com>

artursouza reviewed Jan 10, 2020

View reviewed changes

Comment thread sdk/src/main/java/io/dapr/client/DaprClientGrpcAdapter.java

artursouza reviewed Jan 10, 2020

View reviewed changes

AndresRoblesMX force-pushed the java_sdk_wip branch from 458e5ab to 12f59b1 Compare January 11, 2020 07:43

AndresRoblesMX changed the base branch from java_sdk_wip to master January 11, 2020 07:47

AndresRoblesMX changed the base branch from master to java_sdk_wip January 11, 2020 07:48

artursouza reviewed Jan 12, 2020

View reviewed changes

andres.robles added 4 commits January 12, 2020 17:25

Adding mockito plugin to be able to mock final classes

6ffac0c

Increasing test coverage for ObjectSerializer Fixing bug in GRPC Adapter while creating the envelopes, found during unit testing.

First step into returning Http Headers as part of the response for th…

7ce1f3c

…e DaprClientHttpAdapter Updating State object to match the API. Fixing Broken Unit Tests and increasing coverage for DaprClientGrpcAdapter

Adding documentation, fixing typos and renaming support method to be …

0ea90c2

…more descriptive.

Addressing PR comments

8bbf4fa

Increasing test coverage Fixing Merge conflicts

AndresRoblesMX force-pushed the java_sdk_wip branch from 1faeec5 to 8bbf4fa Compare January 13, 2020 02:01

AndresRoblesMX changed the base branch from java_sdk_wip to master January 13, 2020 02:02

AndresRoblesMX changed the base branch from master to java_sdk_wip January 13, 2020 02:02

artursouza reviewed Jan 13, 2020

View reviewed changes

artursouza suggested changes Jan 13, 2020

View reviewed changes

Addressing PR comments

18f0db3

artursouza approved these changes Jan 13, 2020

View reviewed changes

artursouza merged commit 9703dab into dapr:java_sdk_wip Jan 13, 2020

Uh oh!

Conversation

AndresRoblesMX commented Jan 10, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Issue reference

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AndresRoblesMX commented Jan 11, 2020

Uh oh!

artursouza 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!

AndresRoblesMX commented Jan 13, 2020

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!

artursouza left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

AndresRoblesMX commented Jan 10, 2020 •

edited

Loading