Migrate dd-trace-core groovy files to java part 7#11374
Conversation
we migrate 9 tests: - BaggagePropagatorTest - ServiceDiscoveryTest - TagInterceptorTest - LatencyTraceInterceptorTest - GlobPatternTest - LRUCacheTest - MatchersTest - SimpleRateLimiterTest - SystemAccessTest
|
Hi! 👋 Thanks for your pull request! 🎉 To help us review it, please make sure to:
If you need help, please check our contributing guidelines. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f76adddb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| throw new ArgumentConversionException("Cannot convert " + source); | ||
| } | ||
| } | ||
| return source.toString(); |
There was a problem hiding this comment.
Preserve non-string table values in DDTagsConverter
The fallback return source.toString() coerces every non-symbol literal to String, including numeric/boolean map values. In this migration that means rows like Tags.HTTP_STATUS: 404 in TagInterceptorTest are now passed as "404", so tests no longer cover the statusCode instanceof Number branch in TagInterceptor.interceptHttpStatusCode. This silently weakens regression coverage for type-sensitive tag handling; the converter should return source unchanged when it is not a DDTags.*/Tags.* token.
Useful? React with 👍 / 👎.
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
LGTM in general, left several minor comments.
I see a lot of tests started with test prefix and a lot of tests without.
Let's be consistent and use test everywhere?
| ",,", | ||
| "=" | ||
| }) | ||
| void extractInvalidBaggageHeaders(String baggageHeader) { |
There was a problem hiding this comment.
nit: looks like a missing prefix test ?
| @ValueSource( | ||
| strings = { | ||
| "no-equal-sign,foo=gets-dropped-because-previous-pair-is-malformed", | ||
| "foo=gets-dropped-because-subsequent-pair-is-malformed,=", | ||
| "=no-key", | ||
| "no-value=", | ||
| "", | ||
| ",,", | ||
| "=" | ||
| }) |
There was a problem hiding this comment.
nit: can this be a TableTest with named scenarios for human readability?
| // creating a new instance after injecting config | ||
| propagator = new BaggagePropagator(true, true, DEFAULT_TRACE_BAGGAGE_MAX_ITEMS, 20); | ||
| Map<String, String> headers = singletonMap(BAGGAGE_KEY, baggageHeader); |
There was a problem hiding this comment.
nit: This comment and code looks like repeating... maybe extract to a function with self documented name?
| import org.msgpack.core.MessagePack; | ||
| import org.msgpack.value.MapValue; | ||
|
|
||
| @Timeout(value = 10, unit = TimeUnit.SECONDS) |
There was a problem hiding this comment.
nit: I would put a comment why we need timeout here and other similar places.
| assertEquals( | ||
| "{\"schema_version\":2,\"tracer_language\":\"java\",\"tracer_version\":\"1.2.3\",\"hostname\":\"test-host\",\"logs_collected\":true,\"runtime_id\":\"rid-123\",\"service_name\":\"orders\",\"service_env\":\"prod\",\"service_version\":\"1.1.1\",\"process_tags\":\"key1:val1,key2:val2\",\"container_id\":\"containerID\"}", | ||
| map.toString()); |
There was a problem hiding this comment.
nit: a bit ugly code... any ideas how this can be better?
| static Stream<Arguments> treat1ValueAsTrueForBooleanTagValuesArguments() { | ||
| return Stream.of( | ||
| arguments("manual.drop / true", DDTags.MANUAL_DROP, true, (int) PrioritySampling.USER_DROP), |
| @NullSource | ||
| @ValueSource(strings = {"*", "**"}) |
There was a problem hiding this comment.
nit, I do believe that TableTest with named scenarios would be better.
| @ParameterizedTest | ||
| @NullSource | ||
| @ValueSource(strings = {"*", "**"}) | ||
| void matchAllScenariosMustReturnAnAnyMatcher(String glob) { |
There was a problem hiding this comment.
nit: again missing test prefix?
| static Stream<Arguments> aPatternMatcherTestArguments() { | ||
| return Stream.of( | ||
| arguments("fo? matches Foo", "fo?", "Foo", true), | ||
| arguments("Fo? matches Foo", "Fo?", "Foo", true), |
There was a problem hiding this comment.
TableTest? StringBuilder can be tested as arguments.
| implementation(project(":dd-trace-api")) | ||
| implementation(project(":internal-api")) |
There was a problem hiding this comment.
Just curious how tests refactoring needs this?
What Does This Do
we migrate 9 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
part6: #11362
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.