Uh oh!
There was an error while loading. Please reload this page.
Add smoke test JUnit framework - #12111
Conversation
🟢 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. |
4a50f04 to
11530c9CompareBits has a CI fix ready🟢 Investigated · 🟢 Fix prepared · ⚪ Validation skipped · 🟠 Ready
View in Datadog | Reviewed commit 4abdba0 · Any feedback? Reach out in #deveng-pr-agent |
11530c9 to
4abdba0ComparePerfectSlayer
commented
Jul 31, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:4abdba0567
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| new PollingConditions(timeoutSeconds) | ||
| .eventually(() -> SmokeTraceAssertions.assertTraces(getTraces(), options, matchers)); |
There was a problem hiding this comment.
Reject additional traces that arrive after an early match
For the default count-exact assertion, PollingConditions returns immediately when one snapshot matches. Because both backends receive traces asynchronously, the expected traces can match first and an additional trace from the same test action can arrive just afterward, causing the test to pass even though the final collection violates the exact-count contract; this also makes IGNORE_ADDITIONAL_TRACES ineffective as the distinction between strict and permissive behavior in that scenario. The strict path needs a stabilization/final-count check rather than accepting the first transient match.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
b169c19 to
9c11764ComparePerfectSlayer
commented
Jul 31, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:9c1176468d
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| @Override | ||
| public void beforeEach(ExtensionContext context) { | ||
| if (clearsBetweenTests()) { | ||
| clear(); | ||
| } |
There was a problem hiding this comment.
Preserve startup traces for shared CLI backends
When a SmokeCliApp uses a backend that is also registered as a shared extension, the CLI process can finish and emit all of its traces during beforeAll, after which this beforeEach callback clears them before the test body runs. The CLI app's deliberate lack of an owned-backend reset cannot prevent this; TestAgentBackend only works if callers discover and enable retainAcrossTests(), while MockAgentBackend offers no retention mode, so shared CLI trace assertions are otherwise unusable.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
9c11764 to
b6b0809ComparePerfectSlayer
commented
Jul 31, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:b6b0809cde
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
b6b0809 to
20e23f3ComparePerfectSlayer
commented
Aug 3, 2026
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:20e23f3f65
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| this.process.destroyForcibly(); | ||
| } |
There was a problem hiding this comment.
Wait for forced termination after interruption
When teardown is interrupted while waiting for the child, this branch restores the interrupt flag and returns immediately after destroyForcibly(), which is asynchronous and does not guarantee the process has exited. During Gradle cancellation or another teardown interrupt, the child can therefore retain its port and files after the extension completes; the interrupted path should still confirm termination while preserving the interrupt status.
Useful? React with 👍 / 👎.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
This comment has been minimized.
This comment has been minimized.
20e23f3 to
76991beCompareThere was a problem hiding this comment.
The new JUnit smoke framework cannot compile as configured: the PR removes the module's direct JUnit Jupiter compile dependency while its new main sources import JUnit extension APIs. Restore that compile-only dependency before shipping. No additional tests recommended: this is a build-configuration defect, and the existing tests cannot compile until the dependency is restored.
🤖 Datadog Autotest · Commit 76991be · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:76991be5ec
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Uh oh!
There was an error while loading. Please reload this page.
76991be to
0f9f704Compare0f9f704 to
5080400ComparePerfectSlayer
commented
Aug 4, 2026
As using stacked PR, I will keep rebasing the PR multiple times a day. |
2932b08 to
9a07a24CompareUh oh!
There was an error while loading. Please reload this page.
9a07a24 to
608f482Compare
bric3
left a comment
There was a problem hiding this comment.
Looks good to me. But the test agent seems to be missing env vars.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| } else { | ||
| GenericContainer<?> started = new GenericContainer<>(DockerImageName.parse(this.image)); | ||
| started.withExposedPorts(AGENT_PORT); | ||
| started.withEnv("ENABLED_CHECKS", join(",", this.enabledChecks)); |
There was a problem hiding this comment.
question: I was looking at gitlab file, and noticed a few other env vars
Lines 901 to 903 in e7bda6a
That got me looking at the test agent readme
https://github.com/DataDog/dd-apm-test-agent/blob/v1.64.1/README.md#testtrace_checkfailures-get
Shouldn;t those env var be added, in particular it seems that configuring ENABLED_CHECKS only is not enough, and that should be toggled on (DD_POOL_TRACE_CHECK_FAILURES is true).
https://github.com/DataDog/dd-apm-test-agent/blob/v1.64.1/README.md#testtrace_checkfailures-get
The others, like DD_DISABLE_ERROR_RESPONSES look sensible too.
Without it, I assume assertNoInvariantFailures() could return successfully even after a failed check.
There was a problem hiding this comment.
Yes we should. It’s missing from the container mode (working in CI but not failing locally).
I did not port the DD_SUPPRESS_TRACE_PARSE_ERRORS... I wonder what should we do about it 🤔
This could be reviewed in another PR (like disabling it on CI first to check the existing non compliant behavior).
Uh oh!
There was an error while loading. Please reload this page.
608f482 to
25a1645CompareThere was a problem hiding this comment.
LGTM! I think the organization is done really well and helped with understanding what's going on, e.g. separate SmokeCliApp and SmokeServerApp, each test file is relatively short and focused, backend folder is clearly organized... nice!
It was also helpful to see the stacked PRs with this framework effectively in action.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
| // FIXME: Flaky profiler exception. See PROF-11068. | ||
| "ERROR com.datadog.profiling.controller.ProfilingSystem - Fatal exception in profiling" | ||
| + " thread, trying to continue", | ||
| // FIXME: Flaky profiler exception. See PROF-11072. | ||
| "ERROR com.datadog.profiling.controller.ProfilingSystem - Fatal exception during" | ||
| + " profiling startup", | ||
| // FIXME: Flaky on Spring Boot (e.g. IastSpringBootSmokeTest) and other HTTP-client | ||
| // suites. | ||
| "I/O reactor terminated abnormally", | ||
| // FIXME: Observed in WildflySmokeTest (semeru8): a successful JMX collector exit. |
There was a problem hiding this comment.
nit: maybe better to use TODO (we have 700+ TODOs and 100+ FIXME)?
optional: WDYT if this list to be in resources? But in this case we need to think how to keep comments for each know issue.
There was a problem hiding this comment.
I don’t want to touch / changed this code. It is only moved from its original place following feedback from this PR.
By the way, I think TODO and FIXME don’t have the same meaning (and editors usually handle them differently).
To me TODO is more changes that need to be completed, whereas FIXME are known bugs or limitations.
I can set a TODO in an empty method I need to implement later, while I will add a FIXME around a collection that has a race condition for example.
Uh oh!
There was an error while loading. Please reload this page.
| * @deprecated Prefer {@link #testAgent()}, which speaks the real agent protocol. | ||
| */ | ||
| @Deprecated | ||
| public static AgentBackend mockAgent() { |
There was a problem hiding this comment.
nit: kind of not expected to see deprecated code in brand new class...
There was a problem hiding this comment.
This was added following feedback from the other reviewers.
What other alternative do you envision to convey the mock agent is for local rapid development only, to avoid spinning container?
| private static final String DEFAULT_CI_IMAGE = | ||
| "registry.ddbuild.io/images/mirror/dd-apm-test-agent/ddapm-test-agent"; | ||
| private static final String DEFAULT_PUBLIC_IMAGE = | ||
| "ghcr.io/datadog/dd-apm-test-agent/ddapm-test-agent"; | ||
| private static final String DEFAULT_VERSION = "v1.64.1"; |
There was a problem hiding this comment.
Just curious why we need ddapm-test-agent? In Groovy version we do not have it.
There was a problem hiding this comment.
I might be missing the context of the question but this is the whole point of the framework, to stop testing against ourselves but against an agent implementation.
We will stop testing our encoding and trace pipeline using our own decoding. Same for telemetry, remote config, etc... Does it answer your questions?
| this.jvmArgs = new ArrayList<>(builder.jvmArgs); | ||
| this.programArgs = new ArrayList<>(builder.programArgs); | ||
| this.placeholders = new LinkedHashMap<>(builder.placeholders); | ||
| this.extraEnv = new HashMap<>(builder.extraEnv); |
There was a problem hiding this comment.
Just curious if there any need to wrap values from builder one more time?
Or we expect builder to be reused?
There was a problem hiding this comment.
There is no way to ensure the builder won’t be re-used.
I have example where I could have re-used the builder (like the Spring Boot RabbitMQ smoke tests that creates two similar servers to interact with the queues).
Additionally, this is test code, so I’m fine allocating collection performance wise (if it can save some troubleshooting time later or avoid race conditions).
| command.add("-Ddatadog.slf4j.simpleLogger.defaultLogLevel=" + logLevel); | ||
| command.add("-Dorg.slf4j.simpleLogger.defaultLogLevel=" + logLevel); | ||
| // Trick to prevent jul preferences file lock issue on forked processes, in particular in CI | ||
| // which | ||
| // runs on Linux and have competing processes trying to write to it, including the Gradle | ||
| // daemon. | ||
| // Couldn't flush user prefs: java.util.prefs.BackingStoreException: Couldn't get file lock. | ||
| String tmpDir = System.getProperty("java.io.tmpdir"); | ||
| String uniqueLock = this.name + "_" + System.nanoTime(); | ||
| command.add("-Djava.util.prefs.userRoot=" + tmpDir + "/userPrefs/" + uniqueLock); | ||
| } | ||
| private void appendAgentArguments(List<String> command) { | ||
| if (this.agentJar != null) { | ||
| command.add("-javaagent:" + this.agentJar); | ||
| command.add("-Ddd.agent.host=" + this.backend.url().getHost()); | ||
| command.add("-Ddd.trace.agent.port=" + this.backend.port()); | ||
| command.add("-Ddd.service.name=" + SERVICE_NAME); | ||
| command.add("-Ddd.env=" + ENV); | ||
| command.add("-Ddd.version=" + VERSION); | ||
| String sessionToken = this.backend.sessionToken(); | ||
| if (sessionToken != null) { | ||
| command.add("-Ddd.test.agent.session.token=" + sessionToken); |
There was a problem hiding this comment.
Maybe we can think some simple class around command args to make code a bit more readable, like some sort of builder/utility class with handy methods to make code more readable, something like below (just idea), and maybe worth to have it as common component for building cmd line args:
command
.agent(agentJar) // -javaagent:xxx
.agentFlag("dd.agent.host", backend.url().getHost()) // -Ddd.agent.host=zzz
...
.jar(this.jar)
There was a problem hiding this comment.
Maybe we can think some simple class around command args to make code a bit more readable
Sorry, I might misunderstand the feedback but this is exactly what this code is doing.
The builder is an abstraction of the original List<String> command for the ProcessBuilder.
It has methods like javaAgent(), jar(), mainClass(), classPath(), args(), jvmArgs() to avoid users having to manually construct the tedious string array.
Uh oh!
There was an error while loading. Please reload this page.
| * <pre>{@code | ||
| * // shared: JUnit drives the lifecycle, every app reports to this one backend | ||
| * @RegisterExtension | ||
| * static final AgentBackend agent = AgentBackend.testAgent(); | ||
| * | ||
| * // owned: the app starts, resets and stops its own backend | ||
| * @RegisterExtension | ||
| * static final SmokeCliApp app = SmokeCliApp.named("my-app") | ||
| * .backend(AgentBackend.testAgent()) | ||
| * .build(); | ||
| * }</pre> |
There was a problem hiding this comment.
nitpick:
| * <pre>{@code | |
| * // shared: JUnit drives the lifecycle, every app reports to this one backend | |
| * @RegisterExtension | |
| * staticfinalAgentBackendagent = AgentBackend.testAgent(); | |
| * | |
| * // owned: the app starts, resets and stops its own backend | |
| * @RegisterExtension | |
| * staticfinalSmokeCliAppapp = SmokeCliApp.named("my-app") | |
| * .backend(AgentBackend.testAgent()) | |
| * .build(); | |
| * }</pre> | |
| * <strong>shared:</strong> JUnitdrivesthelifecycle, everyappreportstothisonebackend | |
| * <pre>{@code | |
| * @RegisterExtension | |
| * staticfinalAgentBackendagent = AgentBackend.testAgent(); | |
| * </pre> | |
| * | |
| * <strong>owned:</strong> theappstarts, resetsandstopsitsownbackend (notetheannotationappliesto {@codeSmokeCliApp}) | |
| * <pre>{@code | |
| * @RegisterExtension | |
| * staticfinalSmokeCliAppapp = SmokeCliApp.named("my-app") | |
| * .backend(AgentBackend.testAgent()) | |
| * .build(); | |
| * }</pre> |
| * <p>Testcontainers is a {@code compileOnly} dependency of the smoke base, so this class only loads | ||
| * when a test actually selects a test-agent backend (mock-only tests stay Testcontainers-free). | ||
| */ | ||
| public final class TestAgentBackend extends AgentBackend { |
There was a problem hiding this comment.
question: Would it work to make this package visible, to enforce AgentBackend use ?
There was a problem hiding this comment.
It’s not excluded that mock agent and test agent API might differ. Like test agent getting more features.
I would rather avoid enforcing to declare the behavior into the common abstraction to be reachable, leaving stub methods in the mock agent class.
bric3
left a comment
There was a problem hiding this comment.
Annother batch of comments after looking at later PRs.
I'm weighing on renames to avoid confusion when reading code, while the build rename can be skipped, I thing the assertion methods need to be more explicit on the waiting.
Anyway pre-approving.
Uh oh!
There was an error while loading. Please reload this page.
| * | ||
| * @return The built app. | ||
| */ | ||
| public abstract A build(); |
There was a problem hiding this comment.
thought: I was looking at later PR,s and one thing stroke me, it is now tad obscure if the app has finished. I understand this how JUnit works and as such clearly intended, but I wonder if this is something that may trip readers and writers of smoke tests.
I wonder if the build method can be renamed to suggest this happens before the test ? E.g.
@RegisterExtension
static final SmokeCliApp app =
SmokeCliApp.named("opentelemetry")
.jar(APPLICATION_JAR)
...
- .build();+ .startBeforeAll();
@Test
void receivesTraces() {
app.traces()
.assertTraces(...);
}WDYT ?
There was a problem hiding this comment.
I wonder if the build method can be renamed to suggest this happens before the test ?
Any field initilazer will be executed before method call.
it is now tad obscure if the app has finished
How would that solve the lack of clarity around the finish state? To me, it’s the same as @Shared or @AutoCleanup from Spock, the annotation holds the lifecycle. It’s only because we never used it that we don’t know about it. But I don’t think we changed the semantic of the @Shared field initialized methods to convey its lifecycle.
| /** | ||
| * As {@link #assertTraces(UnaryOperator, TraceMatcher...)}, overriding the timeout. | ||
| * | ||
| * @param timeoutSeconds How long to poll for a match, in seconds. | ||
| * @param options Configures the trace-collection matching. | ||
| * @param matchers The matchers to verify the received traces, one per expected trace. | ||
| */ | ||
| publicvoidassertTraces( | ||
| doubletimeoutSeconds, | ||
| UnaryOperator<SmokeTraceAssertions.Options> options, | ||
| TraceMatcher... matchers) { |
There was a problem hiding this comment.
thought: After reading an example with otel app, I'm weighing if this method should be renamed, because in essence it actually awaits for traces to be visible within a specific deadline (given by the timeout).
So what about waitForTraces ? (And same for the overloads)
There was a problem hiding this comment.
Yes, there is something off. It does not match the awaitLogLines for example…
It need to manually fix it to ensure coherence here. Thanks for catching it.
| * @param mainClass The fully-qualified main class to run. | ||
| * @return This builder, for chaining. | ||
| */ | ||
| public B mainClass(String mainClass) { |
There was a problem hiding this comment.
nitpick: Maybe adding an overload accepting a class could be useful, thus avoiding calling .getName() on it.
There was a problem hiding this comment.
suggestion: Actually looking at usage, what about passing the classpath as a second argument ? Maybe by default System.getProperty('datadog.smoketest.shadowJar.path') if it exists and it's not blank ?
| * @param config The config content as a JSON object literal (e.g. {@code | ||
| * {"asm":{"enabled":true}}}). | ||
| */ | ||
| public void setConfig(String path, String config) { |
There was a problem hiding this comment.
suggestion: What about renaming this to pushConfig, to follow what the javadoc says.
25a1645 to
9d9b24dCompareThere was a problem hiding this comment.
A zero log timeout calls Object.wait(0), which waits without a time limit. A smoke test that requests no wait can hang until the job timeout.
🤖 Datadog Autotest · Commit 9d9b24d · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
| } else { | ||
| waitStart = System.currentTimeMillis(); | ||
| waitTime = 10000; | ||
| waitTime = timeoutMillis; |
There was a problem hiding this comment.
Reject a zero timeout before waiting
A smoke test that requests no wait can hang until the job timeout.
Assertion details
- Input: Call
processTestLogLines(predicate, 0)when no matching log line is available. - Expected: When no matching log exists, a non-positive timeout must end at once with
TimeoutException. - Actual: The first wait uses
Object.wait(0). Java treats zero as an unlimited wait. A negative value throwsIllegalArgumentException.
| waitTime = timeoutMillis; | |
| if (timeoutMillis <= 0) { | |
| thrownewTimeoutException(); | |
| } | |
| waitTime = timeoutMillis; |
Was this helpful? React 👍 or 👎
🤖 Datadog Autotest · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
What Does This Do
This PR adds a JUnit 5 / Java smoke test framework that allows to write smoke tests using Java/JUnit 5 instead of Groovy/Spock.
The framework has four parts:
App runners:
AbstractSmokeAppwithSmokeServerApp(long-running/HTTP) andSmokeCliApp(batch) concrete types, registered as a@RegisterExtension.Owns the launched app's lifecycle and wires in the agent jar + test agent session token.
Backends: a common
TraceBackendfacade with two interchangeable implementations:MockAgentBackend(in-processJavaTestHttpServer, no Docker) andTestAgentBackend(a Testcontainers-managed, or external/CI, dd-apm-test-agent, scoped per test via
X-Datadog-Test-Session-Token).A test written against the facade runs unchanged on either backend but the goal is moving toward
TestAgentBackendby default.Capture surfaces:
Traces(msgpack/JSON decoded through the shared decoder),Logs(std out/error logs with clear, waitFor capabilities),Telemetry(with message-batch flattening), andRemoteConfig(push a config thetracer polls for, and read back its
/v0.7/configpolls, with products/capabilitiesdecoders).
Traces, telemetry, and remote config are all isolated per test for owned backends.
Motivation
The goal is to provide framework for testing tracing instrumentations and products.
Unlike the existing solution, it verifies the signals against an agent, not our our decoders, and it no longer relies on Groovy / Spock but Java and 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: APMLP-1247