Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
This comment has been minimized.
This comment has been minimized.
![datadog-datadog-prod-us1-2[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
🟢 Java Benchmark SLOs — All performance SLOs passed
PR vs. master results
Commit: Load and DaCapo benchmarks can be triggered manually in the GitLab pipeline. Results will appear in the Benchmarking Platform UI after completion. |
jpbempel
added
comp: testing
we migrate tests: - ConfigProviderTest - OtelEnvironmentConfigSourceTest - StableConfigParserTest - BaggageTest - SpanLinkTest - TaskWrapperTest - URIDataAdapterTest - URIDefaultDataAdapterTest - URINoRawDataAdapterTest - URIRawDataAdapterTest - URIUtilsTest - UnparseableURIAdapterTest - Utf8ByteStringTest
6ac6cc6 to
12c67f3
Compare
bric3
left a comment
bric3
left a comment
There was a problem hiding this comment.
Overall looks ok to me, I got a few suggestions, some I'd like to see in the PR before merging, some might be later improvements (non-blocking).
| } | ||
|
|
||
| @Test | ||
| @WithConfig(key = "dd.trace.otel.enabled", value = "true", addPrefix = false) |
There was a problem hiding this comment.
suggestion: Use the already imported TRACE_OTEL_ENABLED in the five system-property overrides, with the default dd. prefix. The span-metrics override can likewise use OTEL_TRACES_SPAN_METRICS_ENABLED while retaining addPrefix = false. This is non-blocking and follows migration rule RULE-A02.
| @WithConfig(key = "dd.trace.otel.enabled", value = "true", addPrefix = false) | |
| @WithConfig(key = TRACE_OTEL_ENABLED, value = "true") |
Note
I only mention this once, but oter tests could apply this suggestion
|
|
||
| @Disabled("enable this test when we enable the OpenTelemetry integration by default") | ||
| @Test | ||
| @WithConfig(key = "otel.sdk.disabled", value = "true", addPrefix = false) |
There was a problem hiding this comment.
note (non-blocking): Beyond the scope of this PR it's "odd" these config key do not exists as constant somewhere (they do not).
| String baseYaml = | ||
| "\n" | ||
| + "config_id: 12345\n" | ||
| + "apm_configuration_default:\n" | ||
| + " KEY_ONE: \"value_one\"\n" | ||
| + "apm_configuration_rules:\n"; | ||
| String builderYaml = | ||
| "\n" | ||
| + " - selectors:\n" | ||
| + " - origin: language\n" | ||
| + " matches: [\"Java\"]\n" | ||
| + " operator: equals\n" | ||
| + " configuration:\n" | ||
| + " KEY_TWO: \"value_two\"\n"; |
There was a problem hiding this comment.
thought (non-blocking): I just thought that maybe we could use String.join instead in multi-line strings?
| SpanAttributes.Builder builder = SpanAttributes.builder(); | ||
|
|
||
| builder.put("string", "value"); | ||
| builder.put("string-empty", ""); | ||
| builder.put("string-null", (String) null); | ||
| builder.put("bool", true); | ||
| builder.put("bool-false", false); | ||
| builder.put("long", 12345L); | ||
| builder.put("long-negative", -12345L); | ||
| builder.put("double", 67.89); | ||
| builder.put("double-negative", -67.89); | ||
| builder.putStringArray("string-array", asList("abc", "", null, "def")); | ||
| builder.putStringArray("string-array-null", null); | ||
| builder.putBooleanArray("bool-array", asList(true, false, null, Boolean.TRUE, Boolean.FALSE)); | ||
| builder.putStringArray("bool-array-null", null); | ||
| builder.putLongArray("long-array", asList(123L, 456L, null, Long.MIN_VALUE, Long.MAX_VALUE)); | ||
| builder.putStringArray("long-array-null", null); | ||
| builder.putDoubleArray( | ||
| "double-array", asList(12.3D, 45.6D, null, Double.MIN_VALUE, Double.MAX_VALUE)); | ||
| builder.putStringArray("double-array-null", null); | ||
| Map<String, String> map = builder.build().asMap(); |
There was a problem hiding this comment.
suggestion (non-blocking): SpanAttributes.Builder api is fluent, the following could be rewritten this way
Map<String, String> map = SpanAttributes.builder()
.put("string", "value")
.put("string-empty", "")
// ...
.build().asMap();| import java.net.URI; | ||
| import org.tabletest.junit.TableTest; | ||
|
|
||
| abstract class URIDataAdapterTest { |
There was a problem hiding this comment.
suggestion: In groovy URIDataAdapterTest is an abstract class, and the file declares the other concrete test classes, e.g. URIDefaultDataAdapterTest.
In order to keep the same organisation, I propose to use JUnit 5's nested classes. In pusedo-code :
class URIDataAdapterTest {
abstract static class AdapterContract {
abstract URIDataAdapter adapter(URI uri);
boolean supportsRaw() {
return true;
}
@TableTest({
// scenari (yes italian plural :D)
})
void testUriParts(
String input,
String scheme, String host, int port,
String path, String fragment, String query,
String rawPath, String rawQuery, String raw) {
URIDataAdapter adapter =
URIDataAdapterBase.fromURI(input, this::adapter);
// test
}
}
@Nested
class URIDefaultDataAdapterTest extends AdapterContract {
@Override
URIDataAdapter adapter(URI uri) {
return new URIDefaultDataAdapter(uri);
}
}
@Nested
class URIRawDataAdapterTest extends AdapterContract {
@Override
URIDataAdapter adapter(URI uri) {
return new RawTestAdapter(uri);
}
}
@Nested
class URINoRawDataAdapterTest extends AdapterContract {
@Override
URIDataAdapter adapter(URI uri) {
return new NoRawTestAdapter(uri);
}
@Override
boolean supportsRaw() {
return false;
}
}
@Nested
class UnparseableURIAdapterTest {
@Test
void shouldReturnRawUriOnly() {
UnparseableURIDataAdapter adapter =
new UnparseableURIDataAdapter(
"http://myurl/path?query=value#fragment");
// test
}
}
static class RawTestAdapter extends URIRawDataAdapter {
private final URI uri;
RawTestAdapter(URI uri) {
this.uri = uri;
}
// test
}
static class NoRawTestAdapter extends URIDataAdapterBase {
private final URI uri;
NoRawTestAdapter(URI uri) {
this.uri = uri;
}
// test
}
}| "too few characters | '%' | '�' ", | ||
| "too few characters after one digit | '%1' | '�' ", | ||
| "illegal 1st character | '%W1' | '�' ", | ||
| "illegal 2nd character | '%1X' | '�' ", | ||
| "illegal 1st character in word | 'wh%Y1t' | 'wh�t' ", | ||
| "illegal 2nd character in word | 'wh%1Zt' | 'wh�t' ", | ||
| "too few characters at end | 'wh%' | 'wh�' ", | ||
| "too few characters after one digit at end | 'wh%1' | 'wh�' ", | ||
| "invalid 2 byte sequence | '%C3%28' | '�(' ", | ||
| "invalid sequence identifier | '%A0%A1' | '��' ", | ||
| "invalid 3 byte sequence in 2nd byte | '%E2%28%A1' | '�(�' ", | ||
| "invalid 3 byte sequence in 3rd byte | '%E2%82%28' | '�(' ", | ||
| "invalid 4 byte sequence in 2nd byte | '%F0%28%8C%BC' | '�(��' ", | ||
| "invalid 4 byte sequence in 3rd byte | '%F0%90%28%BC' | '�(�' ", | ||
| "invalid 4 byte sequence in 4th byte | '%F0%28%8C%28' | '�(�(' ", | ||
| "valid 5 byte sequence but not unicode | '%F8%A1%A1%A1%A1' | '�����' ", | ||
| "valid 6 byte sequence but not unicode | '%FC%A1%A1%A1%A1%A1' | '������' " |
There was a problem hiding this comment.
quibble (if-minor): I know it was there before, but I wonder if it could be safer to use escaped chars here.
This branch has not been deployed
3 participants
What Does This Do
we migrate tests:
Motivation
this is part of the effort to migrate groovy tests to Java/JUnit
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]