-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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 cleanup including being less sensitive to single-host span storage #1658
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.
made some notes explaining how these changes help focus tests on what they are doing. Before, they just made troubleshooting mysterious
@@ -72,7 +72,6 @@ | |||
.addAnnotation(Annotation.create((TODAY + 150) * 1000, CLIENT_SEND, APP_ENDPOINT)) | |||
.addAnnotation(Annotation.create((TODAY + 200) * 1000, CLIENT_RECV, APP_ENDPOINT)) | |||
.addAnnotation(Annotation.create((TODAY + 190) * 1000, "⻩", NO_IP_ENDPOINT)) | |||
.addBinaryAnnotation(BinaryAnnotation.address(CLIENT_ADDR, APP_ENDPOINT)) |
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 is invalid to put a CLIENT_ADDR on a client span. Scrubbed this as it confuses other things
@@ -144,14 +144,12 @@ public void evict_detailed() { | |||
|
|||
@Test | |||
public void evict_oneTraceMultipleSpans() { | |||
Span testSpan1 = span1.toBuilder().traceIdHigh(1L).traceId(123). |
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.
This test was accidentally testing defaults related to ignoring traceIdHigh.
@@ -519,11 +520,11 @@ public void getTraces_differentiateOnServiceName() { | |||
|
|||
Span trace2 = Span.builder().traceId(2).name("get").id(2) | |||
.timestamp((today + 2) * 1000) | |||
.addAnnotation(Annotation.create((today + 1) * 1000, CLIENT_SEND, APP_ENDPOINT)) |
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.
this data had incorrectly aligned timestamps (which wasn't the point of the test)
.map(a -> Annotation.create(a.timestamp + i, a.value, a.endpoint)) | ||
.collect(toList())) | ||
.build(); | ||
trace[i + 1] = Span.builder().traceId(trace[0].traceId).parentId(trace[0].id).id(i).name("") |
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.
this test is also testing "raw spans". To make sure this passes, we should make a span which has no authoritative timestamp (client spans do have an authoritative timestamp and that was the prior test data)
cdb4439
to
dc668e6
Compare
There were tests accidentally assuming merge semantics when testing unrelated things. This scrubs some of the tests to focus on what they are testing, and in doing so help pave forward single-host span storage.
dc668e6
to
92d2dec
Compare
…ge (openzipkin#1658) There were tests accidentally assuming merge semantics when testing unrelated things. This scrubs some of the tests to focus on what they are testing, and in doing so help pave forward single-host span storage.
There were tests accidentally assuming merge semantics when testing
unrelated things. This scrubs some of the tests to focus on what they
are testing, and in doing so help pave forward single-host span storage.
See #1644