Migrate dd-trace-core groovy files to java part 6#11362
Conversation
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
we migrate 6 tests - PendingTracerBufferTest - PendingTraceStrictWriteTest - PendingTraceTest - PendingTraceTestBase - TraceInterceptorTest - TracingConfigPollerTest
jpbempel
force-pushed
the
jpbempel/g2j-core-pt6
branch
from
abcd3c1 to
66b5f3c
Compare
jpbempel
added
comp: testing
| return "0"; | ||
| } | ||
|
|
||
| // @VisibleForTesting |
There was a problem hiding this comment.
I'd be fine with package visible member variables, but I don't feel strongly either way.
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
I found a few issues in the port on PendingTraceBufferTest, that's strange that the llm didn't caught them
| continuations.get(0).cancel(); | ||
|
|
||
| assertEquals(0, trace.getPendingReferenceCount()); | ||
| verify(tracer).write(any()); |
There was a problem hiding this comment.
issue: It seems this verification is missing the payload size, line 117 on the groovy counterpart:
1 * tracer.write({ it.size() == 1 })You can probably write that with Mockito's argThat :
| verify(tracer).write(any()); | |
| verify(tracer).write(argThat(trace -> trace.size() == 1)); |
It seems to be applicable to other verifications, I didn't commented them all.
|
|
||
| assertEquals(0, trace.size()); | ||
| assertEquals(0, trace.getPendingReferenceCount()); | ||
| verify(tracer).write(any()); |
There was a problem hiding this comment.
issue:
| verify(tracer).write(any()); | |
| verify(tracer).write(argThat(trace -> trace.size() == 2)); |
From line 153
1 * tracer.write({ it.size() == 2 })| assertEquals(0, trace.size()); | ||
| assertEquals(0, trace.getPendingReferenceCount()); | ||
| assertTrue(trace.isRootSpanWritten()); | ||
| verify(bufferSpy, atLeastOnce()).enqueue(trace); |
There was a problem hiding this comment.
issue: atLeastOnce() allows more than one invocation at line 359, while the original invocation expect only one.
1 * bufferSpy.enqueue(trace)| verify(bufferSpy, atLeastOnce()).enqueue(trace); | |
| verify(bufferSpy, times(1)).enqueue(trace); |
| verify(bufferSpy).enqueue(any()); | ||
| verify(tracer, never()).write(any()); | ||
| verify(tracer).onRootSpanPublished(any()); |
There was a problem hiding this comment.
question: it seems some assertions are missing
1 * tracer.captureTraceConfig() >> tracerConfig
0 * tracer.writeTimer() >> Monitoring.DISABLED.newTimer("")
Note, the verify(tracer, never()).write(any()); is a bit different than 0 * tracer.write({ it.size() == 1 }) but unsure if this is an issue here.
3 participants
What Does This Do
we migrate 6 tests
Motivation
this is part of the effort to migrate groovy tests to Java/JUnit
part1: #11053
part2: #11062
part3: #11085
part4: #11146
part5: #11217
Additional Notes
Contributor Checklist
type:and (comp:orinst:) labels in addition to any other useful labelsclose,fix, or any linking keywords when referencing an issueUse
solvesinstead, and assign the PR milestone to the issueJira ticket: [PROJ-IDENT]
Note: Once your PR is ready to merge, add it to the merge queue by commenting
/merge./merge -ccancels the queue request./merge -f --reason "reason"skips all merge queue checks; please use this judiciously, as some checks do not run at the PR-level. For more information, see this doc.