Increase coverage for IT tests #19
Conversation
| @@ -27,11 +21,17 @@ | |||
| import org.junit.Rule; | |||
| import org.junit.Test; | |||
| import org.testcontainers.containers.KafkaContainer; | |||
| import zipkin2.Endpoint; | |||
| import zipkin2.Span; | |||
| import zipkin2.*; | |||
There was a problem hiding this comment.
Make sure you apply https://github.com/square/java-code-styles to avoid auto-generated import all.
| import java.io.IOException; | ||
| import java.time.Duration; | ||
| import java.time.Instant; | ||
| import java.util.*; |
| public void shouldFindTracesByAnnotation() throws IOException, InterruptedException { | ||
|
|
||
| // todo: it is not explicit that annotations applied for tags |
There was a problem hiding this comment.
This might be related to: openzipkin/zipkin#2236
| // query by annotation {"key_tag_a":"value_tag_a"} = 1 trace | ||
| await() | ||
| .pollDelay(5, TimeUnit.SECONDS) |
There was a problem hiding this comment.
Isn't the .atMost(5, TimeUnit.SECONDS) a better match for this case?
We are waiting for the store to pick up the change on the output topic, after we have validated that messages are published on the topic L210.
If we are validating that message is already published, then any moment, from that point, the storage could be updated.
Using atMost we are telling to check condition during 5 secs, and if not match, then fail.
With pollDelay we are waiting 5secs and then validating the condition.
It seems that the first one covers better the eventual consistency on this operation.
WDYT?
| // query by annotation {"key_tag_non_exist_a":"value_tag_non_exist_a"} = 1 trace | ||
| await() | ||
| .pollDelay(5, TimeUnit.SECONDS) |
There was a problem hiding this comment.
There are some cases, where it should not found traces (f.ex. with non_existing_service_name), in that case exactly need delayPoll.
I will update comment
| // query by span name `op_a` = 1 trace | ||
| await() | ||
| .atMost(5, TimeUnit.SECONDS) |
| // query by span name `op_b` = 1 trace | ||
| await() | ||
| .atMost(5, TimeUnit.SECONDS) |
| @Test | ||
| public void topicDefault() { | ||
| try{ | ||
| KafkaStorage.Topic.Builder topicBuilder = KafkaStorage.Topic.builder(null); |
There was a problem hiding this comment.
nit: remove unused topicBuilder variable.
|
LGTM thanks @NikitaZhevnitskiy ! |
ps. I would like to receive feedback @jeqo, are there any priority for feature to test first ?