-
Notifications
You must be signed in to change notification settings - Fork 66
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
test: add integration tests #493
Conversation
Signed-off-by: tison <wander4096@gmail.com>
use crate::create_universe; | ||
|
||
#[tokio::test] | ||
#[ignore] |
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.
Executed by cargo test --test it -p engula-client -- --ignored
.
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.
It looks like you've deliberately ignored all integration tests, why?
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.
Integration tests require running services which differs from unit tests.
I actually have two ideas here.
The one is as this and run all unit tests by cargo test --workspace
.
The other could be not to ignore any test and running test with exact package and pattern for both unit tests and integration tests.
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.
Got it, in fact, this is equivalent to providing control to run unit tests and integration tests separately.
I think this is great, especially when there is a relatively clear boundary between unit tests and integration tests.
|
||
#[tokio::test] | ||
#[ignore] | ||
async fn test_apis() -> Result<()> { |
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.
Can extend to more tests.
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
src/engula/src/server.rs
Outdated
|
||
info!(message = "Starting Engula server...", %addr); | ||
let liveness = |
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.
Why do you add both gRPC health service and another liveness server?
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.
For running a simplest it case, we don't have to implement it. Let me remove it as overkill.
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.
Updated. Although, we may want to implement similar logics later.
- For a liveness service, it report that the instance is alive. And we'd better implement it in a way that can be easily verified handy.
- For health check, it's more complex to define readiness for kernel services, for example, a recovered cooperator may have to rebuild its state. A health service follow gRPC standard enables we to report health status of each gRPC service and
tonic_health
provides a heath reporter which can be cloned and passed to our kernel services for reporting. However, it can be an overkill for now.
Signed-off-by: tison <wander4096@gmail.com>
Signed-off-by: tison <wander4096@gmail.com>
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.
Rest LGTM
Co-authored-by: Richard <huachao.huang@gmail.com>
This closes #226.