Create a test control api #191
Conversation
The protocol module will be a shared module between the engine-agent and the extension. It defines the grpc proto files that allow communicating with the engine inside the testcontainer.
Adds a proto file that describes the communication with the engine. The extension will use this as a client. The engine-agent will take care of the server-side implementation.
Adds the server-side implementation of the engine control. The engine control implementation lives in the engine-agent. The reason for this is that we only need to communicate with the engine through grpc when it is running in a testcontainer. The engine-agent is a wrapper around the engine that make it executable in docker and now is also responsible for the communication with the container.
There was a problem hiding this comment.
Sorry for the late review @remcowesterhoud
👍 I like the general direction, and thanks for the good explanation in the commit messages and comments. Great job!
However, I'm not that familiar with some of these topics. So, I still have a bunch of questions that might sound a bit dumb or redundant. Please bear with me. It would be nice if you could clarify them.
❌ The main thing that's missing is tests. Please make sure to add some.
| <!-- protobuf generation --> | ||
| <plugin> | ||
| <groupId>org.xolstice.maven.plugins</groupId> | ||
| <artifactId>protobuf-maven-plugin</artifactId> | ||
| <version>${plugin.version.protobuf-maven-plugin}</version> | ||
| <configuration> | ||
| <checkStaleness>true</checkStaleness> | ||
| <protocArtifact>com.google.protobuf:protoc:${version.protobuf}:exe:${os.detected.classifier}</protocArtifact> | ||
| <pluginId>grpc-java</pluginId> | ||
| <pluginArtifact>io.grpc:protoc-gen-grpc-java:${version.grpc}:exe:${os.detected.classifier}</pluginArtifact> | ||
| </configuration> | ||
| <executions> | ||
| <execution> | ||
| <goals> | ||
| <goal>compile</goal> | ||
| <goal>compile-custom</goal> | ||
| </goals> | ||
| </execution> | ||
| </executions> | ||
| </plugin> |
There was a problem hiding this comment.
🔧 I haven't really created a gRPC implementation before. It would be nice to explain in the commit message why this plugin is necessary instead of the official gRPC tools.
There was a problem hiding this comment.
This plugin is for the code generation. This is how the gRPC docs recommend doing this https://grpc.io/docs/languages/java/generated-code/#codegen
| <extensions> | ||
| <extension> | ||
| <groupId>kr.motd.maven</groupId> | ||
| <artifactId>os-maven-plugin</artifactId> | ||
| <version>${plugin.version.os-maven}</version> | ||
| </extension> | ||
| </extensions> |
There was a problem hiding this comment.
🔧 I'm not familiar with this extension. Can you add a comment explaining what it does and/or add to the commit message why it's needed for this project?
There was a problem hiding this comment.
The os-maven-plugin generates various useful platform-dependent project properties. We need this for the protobuf-maven-plugin for the ${os.detected.classifier} property.
| message WaitForIdleStateRequest { | ||
| // timeout (in ms). The request will be closed if an idle state has not been | ||
| // achieved withing the timeout. | ||
| int64 timeout = 1; |
There was a problem hiding this comment.
❓ Do we only want to have a timeout on this specific message?
There was a problem hiding this comment.
Yes. This is the only request where we will be waiting for something on the server-side. We don't want to get stuck so a timeout here seems like a good addition. This timeout is not the request timeout, it's the waiting timeout. gRPC can handle request timeouts itself.
engine-agent/src/main/java/io/camunda/zeebe/process/test/engine/agent/EngineControlImpl.java
Outdated
Show resolved
Hide resolved
| public void resetEngine( | ||
| final ResetEngineRequest request, | ||
| final StreamObserver<ResetEngineResponse> responseObserver) { | ||
| engine.stop(); |
There was a problem hiding this comment.
❓ Can stopping the engine fail? If so, what happens?
There was a problem hiding this comment.
It could fail. If it happens an error will be thrown which will cause in a failed gRPC request. In the end it would result in a failed unit test. I've added logging in case this happens so we can see what went wrong. I've never had it actually happen before.
engine-agent/src/main/java/io/camunda/zeebe/process/test/engine/agent/EngineControlImpl.java
Outdated
Show resolved
Hide resolved
|
@korthout Thanks for reviewing! I've answered your questions and made some changes. Please have another look. About the unit tests. This will all be extensively tested by the QA tests once the client-side implementation gets merged and this code actually gets used. I wanted to keep the PR's a bit more small and isolated. |
engine-agent/src/main/java/io/camunda/zeebe/process/test/engine/agent/EngineControlImpl.java
Outdated
Show resolved
Hide resolved
We should log our exceptions.
Add documentation on the grpc calls that were still missing it.
remcowesterhoud
force-pushed
the
7-test-control-api
branch
from
f8d7421 to
9d731e7
Compare
Description
This PR adds a way for the extension to communicate with the containerised engine. A new
engine-protocolmodule has been created. In this module the protobuf file will live that defines the interface of communication between the extension and the agent.This PR also adds the serverside implementation of the protocol. This implementation will live in the
engine-agent. This is because this agent is responsible for wrapping the engine in a testcontainer. Communication with the engine comes with this responsibility.Related issues
closes #7
Definition of Done
Not all items need to be done depending on the issue and the pull request.
Code changes:
Testing:
Documentation: