feat: getGrpcClient in integration TestKit, #568 by patriknw · Pull Request #606 · lightbend/kalix-jvm-sdk

patriknw · 2021-10-12T12:31:56Z

References #568

patriknw · 2021-10-12T12:32:50Z

I haven't updated Scala samples yet because waiting for #592 to be merged first

raboof · 2021-10-12T13:40:40Z

I haven't updated Scala samples yet because waiting for #592 to be merged first

Now merged!

patriknw · 2021-10-12T13:42:24Z

thanks, then I'll continue

raboof · 2021-10-12T13:54:31Z

      |   * Use the generated gRPC client to call the service through the Akka Serverless proxy.
      |   */
-      |  private final ${serviceName}Client client;
+      |  private final $serviceName client;


OK, so we lose the information that this is a client, which I think means we lose access to the 'power api' for the client - seems like a bit of a shame but perhaps was tricky to do otherwise?

That is what we have for the client from the Context too. It was intentionally done like that because we thought too much "power" shouldn't leak to the users. If you see a use case where the client is needed you can create an issue about it (maybe we can support both and detect what kind of class that is passed in).

One use case would be to pass metadata/headers I think.

added issue #619

raboof · 2021-10-12T13:56:54Z

-      |  public ${testClassName}() {
-      |    client = ${serviceName}Client.create(testKit.getGrpcClientSettings(), testKit.getActorSystem());
+      |  public $testClassName() {
+      |    client = testKit.getGrpcClient($serviceName.class, "$serviceName");


Is it intentional to have both the class and the service name here? It looks like the 'serviceName' is passed in to find the 'right' implementation of this API from the config, which could be either an external (intra-service) one or the implementation in 'this' service. Since we always want to test the local implementation, perhaps it would be nice if we could have a getGrpcClient method that infers the 'local' service name, instead of requiring it here?

While ${serviceName}Client.create(testKit.getGrpcClientSettings(), testKit.getActorSystem()); was rather verbose, I did like the "no tricks up our sleeve" aspect of it. What advantage does getGrpcClient have here? Caching the clients I think?

The intention is to have the same API from integration tests as they are "used to" from the ActionContext.

For IntegrationTest the service name is not used. I can try to remove it from the testkit.

Since we always want to test the local implementation

Well, I was actually thinking that it could be possible to test a remote service in the same way (with different config), but that is maybe too advanced.

I have removed the serviceName parameter, in TestKit

octonato · 2021-10-12T15:54:35Z

+    private final CustomerService client;

    public CustomerEntityIntegrationTest() {
-        client = CustomerServiceClient.create(testkit.getGrpcClientSettings(), testkit.getActorSystem());
+        client = testKit.getGrpcClient(CustomerService.class, "CustomerService");


I didn't think about this before, but since this method is not returning a client, but the service trait, maybe a more intuitive name would be: getService or serviceInstance.

The fact that is a gRPC client becomes irrelevant. I just want an implementation of CustomerService that will magically do the trick for me.

Sure, the users will get the sense that this is a client calling some remote API, but we may want to reserve that name (ie getGrpcClient) to use for the power API. In that case, we make it more explicitly that what is returned is an akka grpc client. WDYT?

For the record: In the main sources it is called getGrpcClient since it is also a way to re-use/manage lifecycle of any gRPC client you'd want to use, also for a non akkasls service somewhere.

patriknw · 2021-10-13T14:49:14Z

+      client.increase(IncreaseValue(counterId, 42)).flatMap { _ =>
        client.decrease(DecreaseValue(counterId, 32))
-      )).futureValue
+      }.futureValue


@raboof I'm not sure we can guarantee the order from two such "current" client.increase calls (there could be many async boundaries on the way), so I change it to sequential

I'm not sure we can guarantee the order from two such "current" client.increase calls

I agree we can't. I had assumed they'd end at 10 in whatever order they're applied though - though I'll admit I haven't checked, and perhaps that makes this a bad example :)

patriknw · 2021-10-13T14:50:56Z

@@ -16,8 +16,11 @@

 package com.akkaserverless.scalasdk.testkit


@raboof this is the scalasdk AkkaServerlessTestKit changes if you would like to take a second look

… param

patriknw · 2021-10-14T10:09:36Z


-  override def materializer(): Materializer = Materializer(system)
+  override def materializer(): Materializer =
+    SystemMaterializer(system).materializer


@johanandren note this change. If I'm not mistaken Materializer.apply(system) creates a new Materializer?

Ah, indeed, this is what was intended.

patriknw · 2021-10-14T13:33:38Z

        javaClient.closed().asScala
-      case scalaClient: AkkaGrpcScalaClient => scalaClient.close()
+      case scalaClient: AkkaGrpcScalaClient =>
+        scalaClient.closed


Thanks to @raboof for spotting this bug

johanandren

LGTM but I wonder about all those empty unit tests

johanandren · 2021-10-15T08:55:09Z

+    // ActionResult<Empty> result = testKit.decrease(CounterTopicApi.Decreased.newBuilder()...build());
+  }
+
+}


Should we really check in the stub unit tests? It sort of makes it look like something actually is tested in the samples in the CI pr validation and also if we change how the code is generated we'll need to delete and re-check in for it to get up to date with codegen.

ok, I can remove, but it's difficult to "see" that it shouldn't be added when fiddling with the samples

eh, we already have many many of those added
leaving that as a separate cleanup if your find it important

patriknw mentioned this pull request Oct 12, 2021

docs: Add Quickstart for Views #601

Merged

raboof reviewed Oct 12, 2021

View reviewed changes

octonato reviewed Oct 12, 2021

View reviewed changes

patriknw mentioned this pull request Oct 13, 2021

Support AkkaGrpcClient #619

Open

patriknw force-pushed the wip-568-grpc-it-patriknw branch 2 times, most recently from fb40195 to b8eb5eb Compare October 13, 2021 14:46

patriknw commented Oct 13, 2021

View reviewed changes

patriknw added 5 commits October 14, 2021 11:59

feat: getGrpcClient in integration TestKit, #568

fb38948

it's testKit, not testkit

1f22f77

getGrcpClient in Scala AkkaServerlessTestKit, and remove service name…

964af6f

… param

a few more service name

26af6ff

use host port instead of config

37d2b05

patriknw force-pushed the wip-568-grpc-it-patriknw branch from 59fb21f to 37d2b05 Compare October 14, 2021 10:00

patriknw commented Oct 14, 2021

View reviewed changes

Comment thread ...les/scala-valueentity-counter/src/test/scala/com/example/CounterServiceIntegrationSpec.scala

patriknw commented Oct 14, 2021

View reviewed changes

closed, not close()

b386c8d

patriknw commented Oct 14, 2021

View reviewed changes

patriknw requested a review from johanandren October 15, 2021 06:39

johanandren approved these changes Oct 15, 2021

View reviewed changes

patriknw merged commit e08af33 into main Oct 15, 2021

patriknw deleted the wip-568-grpc-it-patriknw branch October 15, 2021 09:06

		@@ -16,8 +16,11 @@

		package com.akkaserverless.scalasdk.testkit

Conversation

patriknw commented Oct 12, 2021

Uh oh!

patriknw commented Oct 12, 2021

Uh oh!

raboof commented Oct 12, 2021

Uh oh!

patriknw commented Oct 12, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patriknw Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

patriknw Oct 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

patriknw Oct 13, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

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

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

patriknw Oct 12, 2021 •

edited

Loading

patriknw Oct 12, 2021 •

edited

Loading

patriknw Oct 13, 2021 •

edited

Loading