Uh oh!
There was an error while loading. Please reload this page.
Migrate dd-trace-core groovy files to java part 9 - #11488
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. |
This comment has been minimized.
This comment has been minimized.
bric3
left a comment
There was a problem hiding this comment.
Conversion-specific Mockito comments.
| singletonMap("k", "v")); | ||
| Map<String, String> carrier = mock(Map.class); | ||
| mockedContext.beginEndToEnd(); |
There was a problem hiding this comment.
The Groovy test verified both time-source calls before 0 * _, but this conversion only checks the carrier. Suggested fix: clear the stubbing/build noise before beginEndToEnd(), then verify the two time-source calls too.
clearInvocations(timeSource);
mockedContext.beginEndToEnd();
injector.inject(mockedContext, carrier, MapSetter.INSTANCE);
verify(timeSource).getCurrentTimeNanos();
verify(timeSource).getNanoTicks();
verify(carrier)
.put(
"X-Amzn-Trace-Id",
"Root=1-633c7675-000000000000000000000001;Parent=0000000000000002;_dd.origin=fakeOrigin;t0=1664906869195;k=v");
verifyNoMoreInteractions(timeSource, carrier);That keeps the Mockito version as strict as the original Spock interaction block.
| propagator.inject(span, carrier, setter); | ||
| verify(injector).inject(any(DDSpanContext.class), same(carrier), any()); |
There was a problem hiding this comment.
The Spock assertion matched span.context() specifically. Suggested fix: keep that exact-context check in the Mockito verify instead of accepting any DDSpanContext.
verify(injector).inject(same((DDSpanContext) span.context()), same(carrier), any());same is already statically imported here, so this keeps the conversion tight without adding much noise.
| propagator.inject(span, carrier, setter); | ||
| verify(injector, times(injected)).inject(any(DDSpanContext.class), same(carrier), any()); |
There was a problem hiding this comment.
Same conversion concern here: the original interaction was tied to span.context(). Suggested fix: keep the times(injected) behavior, but match the same context instance.
verify(injector, times(injected))
.inject(same((DDSpanContext) span.context()), same(carrier), any());That preserves the old assertion while keeping the converted table-test flow.
952a80e to
588ea9bCompare🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master resultsStartup Time
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
amarziali
left a comment
There was a problem hiding this comment.
looks good. left minor comments
| verify(carrier).put(OT_BAGGAGE_PREFIX + "k1", "v1"); | ||
| verify(carrier).put(OT_BAGGAGE_PREFIX + "k2", "v2"); | ||
| verify(carrier).put("SOME_CUSTOM_HEADER", "some-value"); | ||
| verify(carrier).put(DD_PARENT_ID_BAGGAGE_KEY, "0"); |
There was a problem hiding this comment.
| verify(carrier).put(DD_PARENT_ID_BAGGAGE_KEY, "0"); | |
| verify(carrier).put(DD_PARENT_ID_BAGGAGE_KEY, "0"); | |
| verifyNoMoreInteractions(carrier); |
| verify(carrier).put(SPAN_ID_KEY, spanUuid); | ||
| verify(carrier).put(DD_SPAN_ID_BAGGAGE_KEY, spanId); | ||
| verify(carrier).put(OT_BAGGAGE_PREFIX + "k1", "v1"); | ||
| verify(carrier).put(OT_BAGGAGE_PREFIX + "k2", "v2"); |
There was a problem hiding this comment.
| verify(carrier).put(OT_BAGGAGE_PREFIX + "k2", "v2"); | |
| verify(carrier).put(OT_BAGGAGE_PREFIX + "k2", "v2"); | |
| verifyNoMoreInteractions(carrier); |
There was a problem hiding this comment.
this is not what is done in groovy, and does not work
| private static final CarrierSetter<Map<String, String>> MAP_SETTER = Map::put; | ||
| private final HttpCodec.Injector injector = | ||
| HaystackHttpCodec.newInjector(singletonMap("some-baggage-key", "SOME_CUSTOM_HEADER")); |
There was a problem hiding this comment.
minor: the groovy test closed the injector on cleanup
we migrate 20 tests from the propagation package: - TagKeyTest - TagValueTest - B3HttpExtractorTest - B3HttpInjectorPaddedTest - B3HttpInjectorTest - DatadogHttpExtractorTest - DatadogHttpInjectorTest - DatadogPropagationTagsTest - HaystackHttpExtractorTest - HaystackHttpInjectorTest - HttpExtractorTest - HttpInjectorB3PaddingTest - HttpInjectorTest - NoneHttpExtractorTest - TracingPropagatorTest - W3CHttpExtractorTest - W3CHttpInjectorTest - W3CPropagationTagsTest - XRayHttpExtractorTest - XRayHttpInjectorTest
f8b5091 to
be578afCompare/merge |
View all feedbacks in Devflow UI.
The expected merge time in
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
What Does This Do
we migrate 20 tests from the propagation package:
Motivation
this is part of the effort to migrate groovy tests to Java/JUnit
part1: #11053
part2: #11062
part3: #11085
part4: #11146
part5: #11217
part6: #11362
part7: #11374
part8: #11437
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 issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-levelJira ticket: [PROJ-IDENT]