Repository navigation
Conversation
we migrate 10 tests: - AgentLoadedIntoBootstrapTest - CustomLogManagerTest - CustomMBeanServerBuilderTest - DDInfoTest - InitializationTelemetryTest - InstrumenterUnloadTest - JMXFetchTest - ListIntegrationsTest - LogLevelTest - SecurityManagerTest Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
jpbempel
added
comp: testing
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. |
![datadog-official[bot]](https://avatars.githubusercontent.com/in/1358509?s=60&v=4){kind=small}
There was a problem hiding this comment.
More details
The Java migrations preserve the forked-JVM checks, cleanup, and SecurityManager JDK exclusions. Runtime equivalence remains unverified because the tests were not executed.
🤖 Bits Code Review · Commit 27f4e9f
🟢 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. |
| class ListIntegrationsTest { | ||
|
|
||
| @Test | ||
| void ddTraceDebugFalse() throws Exception { |
There was a problem hiding this comment.
this naming looks wrong (test doesn't configure trace debug) but also the naming was wrong in the original tests, so I can fix in a follow-up
AlexeyKuznetsov-DD
left a comment
AlexeyKuznetsov-DD
left a comment
There was a problem hiding this comment.
LGTM, left minor comments
|
|
||
| private static final String DEFAULT_LOG_LEVEL = "debug"; | ||
|
|
||
| // Run all tests using forked jvm because groovy has already set the global log manager |
There was a problem hiding this comment.
nit: this comment is a bit confusing: why mention Groovy and global log manager in JUnit java test?
| /* Priority: datadog.slf4j.simpleLogger.defaultLogLevel > dd.trace.debug > DD_TRACE_DEBUG | ||
| 1: INFO LOGS | ||
| 0: DEBUG Logs | ||
| */ |
There was a problem hiding this comment.
nit: better to be just simple comments //?
Also I do not understand the meaning of comment 1: INFO LOGS and 0: DEBUG Logs
Probably expected exitCode?
| /* Priority: datadog.slf4j.simpleLogger.defaultLogLevel > dd.trace.debug > DD_TRACE_DEBUG | |
| 1: INFO LOGS | |
| 0: DEBUG Logs | |
| */ | |
| // Priority: datadog.slf4j.simpleLogger.defaultLogLevel > dd.trace.debug > DD_TRACE_DEBUG | |
| // INFO Logs exit code: 1 | |
| // DEBUG Logs exit code: 0 |
What Does This Do
we migrate 10 tests:
Motivation
this is part of the effort to migrate groovy tests to Java/JUnit
Additional Notes
No production code change. These tests mostly fork a JVM with the agent and check its exit code:
LogLevelTestare now a single@TableTestCustomLogManagerTestandCustomMBeanServerBuilderTestshare their common JVM arguments through a helper@IgnoreIf(JDK >= 24)becomes@DisabledForJreRange(min = JAVA_24)for the SecurityManager-based tests (verified on JDK 11 as well)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]
🤖 Generated with Claude Code