Skip to content

Commit 088102b

Browse files
jpbempeljpbempel
committed
address comments
1 parent 3b6d83d commit 088102b

4 files changed

Lines changed: 97 additions & 85 deletions

File tree

‎.claude/skills/migrate-groovy-to-java/SKILL.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,6 +30,8 @@ When converting Groovy code to Java code, make sure that:
3030
- Do not mark local variables `final`
3131
- Ensure variables are human-readable; avoid single-letter names and pre-define variables that are referenced multiple times
3232
- When translating Spock `Mock(...)` usage, use `libs.bundles.mockito` instead of writing manual recording/stub implementations
33+
- Keep inline comments
34+
- Migrate the named Spock clauses if they exist as inline comments in the Java unit test
3335

3436
TableTest usage
3537
Import: `import org.tabletest.junit.TableTest;`

‎dd-trace-core/src/test/java/datadog/trace/core/DDSpanContextTest.java‎

Lines changed: 43 additions & 48 deletions
Original file line numberDiff line numberDiff line change
@@ -22,9 +22,8 @@
2222
import static datadog.trace.core.DDSpanContext.SPAN_SAMPLING_RULE_RATE_TAG;
2323
import static org.junit.jupiter.api.Assertions.assertEquals;
2424
import static org.junit.jupiter.api.Assertions.assertFalse;
25+
import static org.junit.jupiter.api.Assertions.assertInstanceOf;
2526
import static org.junit.jupiter.api.Assertions.assertNull;
26-
import static org.junit.jupiter.api.Assertions.assertTrue;
27-
import static org.junit.jupiter.params.provider.Arguments.arguments;
2827
import static org.mockito.Mockito.mock;
2928
import static org.mockito.Mockito.times;
3029
import static org.mockito.Mockito.verify;
@@ -44,11 +43,9 @@
4443
import datadog.trace.junit.utils.tabletest.TableTestTypeConverters;
4544
import java.util.HashMap;
4645
import java.util.Map;
47-
import java.util.stream.Stream;
4846
import org.junit.jupiter.api.BeforeEach;
4947
import org.junit.jupiter.api.Test;
5048
import org.junit.jupiter.params.ParameterizedTest;
51-
import org.junit.jupiter.params.provider.Arguments;
5249
import org.junit.jupiter.params.provider.MethodSource;
5350
import org.junit.jupiter.params.provider.ValueSource;
5451
import org.tabletest.junit.TableTest;
@@ -73,16 +70,9 @@ void setup() {
7370
.build();
7471
}
7572

76-
// spotless:off
77-
@TableTest({
78-
"scenario | name",
79-
"SERVICE_NAME | " + DDTags.SERVICE_NAME,
80-
"RESOURCE_NAME | " + DDTags.RESOURCE_NAME,
81-
"SPAN_TYPE | " + DDTags.SPAN_TYPE,
82-
"spme.tag | some.tag"
83-
})
84-
//spotless:on
85-
void nullValuesForTagsDeleteExistingTags(String scenario, String name) throws Exception {
73+
@ParameterizedTest
74+
@ValueSource(strings = {DDTags.SERVICE_NAME, DDTags.RESOURCE_NAME, DDTags.SPAN_TYPE, "some.tag"})
75+
void nullValuesForTagsDeleteExistingTags(String name) throws Exception {
8676
AgentSpan span =
8777
tracer
8878
.buildSpan("fakeOperation")
@@ -93,35 +83,40 @@ void nullValuesForTagsDeleteExistingTags(String scenario, String name) throws Ex
9383
DDSpanContext context = (DDSpanContext) span.context();
9484

9585
context.setTag("some.tag", "asdf");
96-
context.setTag(name, (String) null);
86+
context.setTag(name, null);
9787
context.setErrorFlag(true, ErrorPriorities.DEFAULT);
9888
span.finish();
9989
writer.waitForTraces(1);
10090

101-
Thread thread = Thread.currentThread();
102-
Map<String, Object> expectedTags = new HashMap<>();
103-
if (!name.equals("some.tag")) {
91+
Map<String, Object> expectedTags = createExpectedTagsFromCurrentThread();
92+
if (!"some.tag".equals(name)) {
10493
expectedTags.put("some.tag", "asdf");
10594
}
106-
expectedTags.put(THREAD_NAME, thread.getName());
107-
expectedTags.put(THREAD_ID, thread.getId());
108-
expectedTags.put(DDTags.DD_SVC_SRC, ServiceNameSources.MANUAL);
10995

11096
assertTagmap(context.getTags(), expectedTags);
11197
assertEquals("fakeService", context.getServiceName());
11298
assertEquals("fakeResource", context.getResourceName().toString());
11399
assertEquals("fakeType", context.getSpanType().toString());
114100
}
115101

102+
private static Map<String, Object> createExpectedTagsFromCurrentThread() {
103+
Thread thread = Thread.currentThread();
104+
Map<String, Object> expectedTags = new HashMap<>();
105+
expectedTags.put(THREAD_NAME, thread.getName());
106+
expectedTags.put(THREAD_ID, thread.getId());
107+
expectedTags.put(DDTags.DD_SVC_SRC, ServiceNameSources.MANUAL);
108+
return expectedTags;
109+
}
110+
116111
// spotless:off
117112
@TableTest({
118-
"scenario | name | value | method ",
113+
"scenario | name | expected | method ",
119114
"SERVICE_NAME tag | " + DDTags.SERVICE_NAME + " | different service | serviceName ",
120115
"RESOURCE_NAME tag | " + DDTags.RESOURCE_NAME + " | different resource | resourceName ",
121116
"SPAN_TYPE tag | " + DDTags.SPAN_TYPE + " | different type | spanType "
122117
})
123118
//spotless:on
124-
void specialTagsSetCertainValues(String scenario, String name, String value, String method)
119+
void specialTagsSetCertainValues(String scenario, String name, String expected, String method)
125120
throws Exception {
126121
AgentSpan span =
127122
tracer
@@ -132,40 +127,37 @@ void specialTagsSetCertainValues(String scenario, String name, String value, Str
132127
.start();
133128
DDSpanContext context = (DDSpanContext) span.context();
134129

135-
context.setTag(name, value);
130+
context.setTag(name, expected);
136131
span.finish();
137132
writer.waitForTraces(1);
138133

139-
Thread thread = Thread.currentThread();
140-
Map<String, Object> expectedTags = new HashMap<>();
141-
expectedTags.put(THREAD_NAME, thread.getName());
142-
expectedTags.put(THREAD_ID, thread.getId());
143-
expectedTags.put(DDTags.DD_SVC_SRC, ServiceNameSources.MANUAL);
134+
Map<String, Object> expectedTags = createExpectedTagsFromCurrentThread();
144135
assertTagmap(context.getTags(), expectedTags);
145136

146-
Object actualValue;
137+
String value;
147138
switch (method) {
148139
case "serviceName":
149-
actualValue = context.getServiceName();
140+
value = context.getServiceName();
150141
break;
151142
case "resourceName":
152-
actualValue = context.getResourceName().toString();
143+
value = context.getResourceName().toString();
153144
break;
154145
case "spanType":
155-
actualValue = context.getSpanType().toString();
146+
value = context.getSpanType().toString();
156147
break;
157148
default:
158149
throw new IllegalArgumentException("Unknown method: " + method);
159150
}
160-
assertEquals(value, actualValue);
151+
assertEquals(expected, value);
161152
}
162153

163-
static Stream<Arguments> tagsCanBeAddedToContextArguments() {
164-
return Stream.of(
165-
arguments("tag.name", "some value"),
166-
arguments("tag with int", 1234),
167-
arguments("tag-with-bool", false),
168-
arguments("tag_with_float", 0.321));
154+
static Object[][] tagsCanBeAddedToContextArguments() {
155+
return new Object[][] {
156+
{"tag.name", "some value"},
157+
{"tag with int", 1234},
158+
{"tag-with-bool", false},
159+
{"tag_with_float", 0.321}
160+
};
169161
}
170162

171163
@ParameterizedTest
@@ -184,13 +176,8 @@ void tagsCanBeAddedToContext(String name, Object value) throws Exception {
184176
span.finish();
185177
writer.waitForTraces(1);
186178

187-
Thread thread = Thread.currentThread();
188-
Map<String, Object> expectedTags = new HashMap<>();
179+
Map<String, Object> expectedTags = createExpectedTagsFromCurrentThread();
189180
expectedTags.put(name, value);
190-
expectedTags.put(THREAD_NAME, thread.getName());
191-
expectedTags.put(THREAD_ID, thread.getId());
192-
expectedTags.put(DDTags.DD_SVC_SRC, ServiceNameSources.MANUAL);
193-
194181
assertTagmap(context.getTags(), expectedTags);
195182
}
196183

@@ -212,6 +199,7 @@ void tagsCanBeAddedToContext(String name, Object value) throws Exception {
212199
"java.lang.Integer | 0x55 "
213200
})
214201
void metricsUseExpectedTypes(Class<?> expectedType, Number value) {
202+
// floats should be converted to doubles.
215203
AgentSpan span =
216204
tracer
217205
.buildSpan("fakeOperation")
@@ -222,7 +210,7 @@ void metricsUseExpectedTypes(Class<?> expectedType, Number value) {
222210

223211
context.setMetric("test", value);
224212

225-
assertTrue(expectedType.isInstance(context.getTag("test")));
213+
assertInstanceOf(expectedType, context.getTag("test"));
226214

227215
span.finish();
228216
}
@@ -312,7 +300,6 @@ void setSingleSpanSamplingTags(double rate, int limit) {
312300
.withResourceName("fakeResource")
313301
.start();
314302
DDSpanContext context = (DDSpanContext) span.context();
315-
316303
assertEquals(UNSET, context.getSamplingPriority());
317304

318305
context.setSpanSamplingPriority(rate, limit);
@@ -322,7 +309,9 @@ void setSingleSpanSamplingTags(double rate, int limit) {
322309
assertEquals(
323310
limit == Integer.MAX_VALUE ? null : Double.valueOf(limit),
324311
context.getTag(SPAN_SAMPLING_MAX_PER_SECOND_TAG));
312+
// single span sampling should not change the trace sampling priority
325313
assertEquals(UNSET, context.getSamplingPriority());
314+
// make sure the `_dd.p.dm` tag has not been set by single span sampling
326315
assertFalse(context.getPropagationTags().createTagMap().containsKey("_dd.p.dm"));
327316

328317
span.finish();
@@ -397,6 +386,9 @@ void spanIdsPrintedAsUnsignedLong() {
397386

398387
DDSpanContext context = (DDSpanContext) span.context();
399388

389+
// even though span ID and parent ID are setup as negative numbers, they should be printed as
390+
// their unsigned value
391+
// asserting there is no negative sign after ids is the best I can do.
400392
assertFalse(context.toString().contains("id=-"));
401393

402394
span.finish();
@@ -418,15 +410,18 @@ void serviceNameSourceIsPropagatedFromParentToChildSpan() {
418410

419411
@Test
420412
void spanKindOrdinalConstantsAndSpanKindValuesArrayStayInSync() {
413+
// SPAN_KIND_VALUES array covers all ordinals
421414
assertEquals(DDSpanContext.SPAN_KIND_CUSTOM + 1, SPAN_KIND_VALUES.length);
422415

416+
// each known ordinal maps to the correct Tags constant"
423417
assertEquals(Tags.SPAN_KIND_SERVER, SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_SERVER]);
424418
assertEquals(Tags.SPAN_KIND_CLIENT, SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_CLIENT]);
425419
assertEquals(Tags.SPAN_KIND_PRODUCER, SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_PRODUCER]);
426420
assertEquals(Tags.SPAN_KIND_CONSUMER, SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_CONSUMER]);
427421
assertEquals(Tags.SPAN_KIND_INTERNAL, SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_INTERNAL]);
428422
assertEquals(Tags.SPAN_KIND_BROKER, SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_BROKER]);
429423

424+
// UNSET and CUSTOM map to null
430425
assertNull(SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_UNSET]);
431426
assertNull(SPAN_KIND_VALUES[DDSpanContext.SPAN_KIND_CUSTOM]);
432427
}

‎dd-trace-core/src/test/java/datadog/trace/core/DDSpanTest.java‎

Lines changed: 20 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,7 @@ void durationMeasuredInNanoseconds() {
123123
span.finish();
124124
long total = System.nanoTime() - start;
125125

126+
// Generous 5 seconds to execute this test
126127
assertTrue(
127128
Math.abs(
128129
TimeUnit.NANOSECONDS.toSeconds(span.getStartTime())
@@ -140,8 +141,9 @@ void phasedFinishCapturesDurationButDoesNotPublishImmediately() throws Exception
140141
DDSpan span = (DDSpan) tracer.buildSpan("test").start();
141142
long between = System.nanoTime();
142143
long betweenDur = System.nanoTime() - between;
143-
144+
// calling publish before phasedFinish
144145
span.publish();
146+
// has no effect
145147
assertEquals(0, span.getDurationNano());
146148
assertEquals(1, pendingReferenceCount(span));
147149
assertEquals(0, writer.size());
@@ -153,32 +155,36 @@ void phasedFinishCapturesDurationButDoesNotPublishImmediately() throws Exception
153155
assertEquals(1, pendingReferenceCount(span));
154156
assertTrue(spans(span).isEmpty());
155157
assertTrue(writer.isEmpty());
156-
158+
// duration is recorded as negative to allow publishing
157159
assertTrue(span.getDurationNano() < 0);
158160
long actualDurationNano = span.getDurationNano() & Long.MAX_VALUE;
161+
// Generous 5 seconds to execute this test
159162
assertTrue(
160163
Math.abs(
161164
TimeUnit.NANOSECONDS.toSeconds(span.getStartTime())
162165
- TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis()))
163166
< 5);
164167
assertTrue(actualDurationNano > betweenDur);
165168
assertTrue(actualDurationNano < total);
166-
assertTrue(actualDurationNano % mod > 0);
167-
169+
assertTrue(actualDurationNano % mod > 0); // Very slim chance of a false negative.
170+
// extra finishes
168171
finish = span.phasedFinish();
169-
span.finish();
172+
// have no effect
173+
span.finish(); // verify conflicting finishes are ignored
170174
assertFalse(finish);
171175
assertEquals(1, pendingReferenceCount(span));
172176
assertTrue(spans(span).isEmpty());
173177
assertTrue(writer.isEmpty());
174178

175179
span.publish();
180+
// duration is flipped to positive
176181
assertTrue(span.getDurationNano() > 0);
177182
assertEquals(actualDurationNano, span.getDurationNano());
178183
assertEquals(0, pendingReferenceCount(span));
179184
assertEquals(1, writer.size());
180-
185+
// duplicate call to publish"
181186
span.publish();
187+
// has no effect
182188
assertEquals(0, pendingReferenceCount(span));
183189
assertEquals(1, writer.size());
184190
}
@@ -198,6 +204,7 @@ void startingWithTimestampDisablesNanotime() {
198204
span.finish();
199205
long total = Math.max(1, System.currentTimeMillis() - start);
200206

207+
// Generous 5 seconds to execute this test
201208
assertTrue(
202209
Math.abs(
203210
TimeUnit.NANOSECONDS.toSeconds(span.getStartTime())
@@ -218,19 +225,22 @@ void stoppingWithTimestampDisablesNanotime() {
218225
span.finish(TimeUnit.MILLISECONDS.toMicros(System.currentTimeMillis() + 1));
219226
long total = System.currentTimeMillis() - start + 1;
220227

228+
// Generous 5 seconds to execute this test
221229
assertTrue(
222230
Math.abs(
223231
TimeUnit.NANOSECONDS.toSeconds(span.getStartTime())
224232
- TimeUnit.MILLISECONDS.toSeconds(System.currentTimeMillis()))
225233
< 5);
226234
assertTrue(span.getDurationNano() >= TimeUnit.MILLISECONDS.toNanos(betweenDur));
227235
assertTrue(span.getDurationNano() <= TimeUnit.MILLISECONDS.toNanos(total));
236+
// true span duration can be <1ms if clock was about to tick over, so allow for that
228237
assertTrue(span.getDurationNano() % mod == 0 || span.getDurationNano() == 1);
229238
}
230239

231240
@Test
232241
void stoppingWithTimestampBeforeStartTimeYieldsMinDurationOfOne() {
233242
DDSpan span = (DDSpan) tracer.buildSpan("test").start();
243+
// remove tick precision part of our internal time to match previous test condition
234244
span.finish(
235245
TimeUnit.MILLISECONDS.toMicros(TimeUnit.NANOSECONDS.toMillis(span.getStartTimeNano()))
236246
- 10);
@@ -284,10 +294,12 @@ void originSetOnlyOnRootSpan(String scenario, AgentSpanContext extractedContext)
284294
(DDSpanContext) tracer.buildSpan("testChild1").asChildOf(parent).start().context();
285295

286296
assertEquals("some-origin", parent.getOrigin().toString());
297+
// Access field directly instead of getter.
287298
Field originField = DDSpanContext.class.getDeclaredField("origin");
288299
originField.setAccessible(true);
289300
assertEquals("some-origin", originField.get(parent).toString());
290301
assertEquals("some-origin", child.getOrigin().toString());
302+
// Access field directly instead of getter.
291303
assertNull(originField.get(child));
292304
}
293305

@@ -343,6 +355,7 @@ void getApplicationRootSpanInAndNotInContextOfDistributedTracing(
343355

344356
assertEquals(root, root.getLocalRootSpan());
345357
assertEquals(root, child.getLocalRootSpan());
358+
// Checking for backward compatibility method names
346359
assertEquals(root, root.getRootSpan());
347360
assertEquals(root, child.getRootSpan());
348361

@@ -448,6 +461,7 @@ void setSingleSpanSamplingTags(String scenario, double rate, int limit) {
448461
assertEquals(
449462
limit == Integer.MAX_VALUE ? null : (double) limit,
450463
span.getTag(SPAN_SAMPLING_MAX_PER_SECOND_TAG));
464+
// single span sampling should not change the trace sampling priority
451465
assertEquals(UNSET, span.samplingPriority());
452466
}
453467

0 commit comments

Comments
 (0)