-
Notifications
You must be signed in to change notification settings - Fork 39
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: enable component client for integration tests #1744
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.. I think we might need another utility method as commented below but that can be address in a follow-up.
*/ | ||
protected <T> T execute(DeferredCall<Any, T> deferredCall) { | ||
return execute(deferredCall, timeout); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we will also need another utility method to allow you to cast a result to a specific type.. noticed this while having:
public sealed interface Response {
record Ok(String msg) implements Response {
public static Ok of(String created) {
return new Ok(created);
}
}
}
If the deferredCall method is declaring to return Response
and not Response.Ok
then the execution will fail on the test with a jackson error not knowing how to construct Response
.
But this can be solved as follow-up I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually, the solution for this problem is a bit different: https://github.com/aludwiko/kalix-saga-patterns/blob/main/src/main/java/com/example/cinema/application/Response.java#L10
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha, I see. fair enough 👍
@Autowired | ||
private ComponentClient componentClient; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does that mean that we can wire the ComponentClient
anywhere? Like wire it into an Entity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it's not possible, nothing has changed.
...va-sdk-spring-testkit/src/main/java/kalix/spring/testkit/KalixIntegrationTestKitSupport.java
Show resolved
Hide resolved
/** | ||
* Utility method to execute a deferred call and wait for the response for a specified time. | ||
*/ | ||
protected <T> T execute(DeferredCall<Any, T> deferredCall, Duration timeout) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm in doubt if we should add this method. I know that this comes from our own internal test class, but do we need yet another API to offer to users?
In prod code, users will do defferedCall.execute()
so we can expect them to do the same in tests.
They can then chain different calls if they want and eventually add .toCompletableFuture().get(timeout)
as they would usually do when working with Java Futures.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure about this method as well, my motivation was that such a method will appear in any integration test, but maybe it's fine and better for the user to decide when/if to add it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would remove it. It's just a thin layer on top of a very common CompletionStage operation. We don't need to abstract this away for users.
References #1743