Repository navigation
Add smoke test JUnit framework - #12111
gh-worker-dd-mergequeue-cf854d[bot] merged 1 commit into
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
11530c9
Compare
|
Bits Code status: ✅ Done Comment @DataDog to request changes |
11530c9 to
4abdba0
Compare
|
@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".
| new PollingConditions(timeoutSeconds) | ||
| .eventually(() -> SmokeTraceAssertions.assertTraces(getTraces(), options, matchers)); |
There was a problem hiding this comment.
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 👍 / 👎.
b169c19 to
9c11764
Compare
|
@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".
| @Override | ||
| public void beforeEach(ExtensionContext context) { | ||
| if (clearsBetweenTests()) { | ||
| clear(); | ||
| } |
There was a problem hiding this comment.
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 👍 / 👎.
9c11764 to
b6b0809
Compare
|
@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".
b6b0809 to
20e23f3
Compare
|
@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.
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 👍 / 👎.
This comment has been minimized.
This comment has been minimized.
20e23f3 to
76991be
Compare
There 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
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".
76991be to
0f9f704
Compare
0f9f704 to
5080400
Compare
|
As using stacked PR, I will keep rebasing the PR multiple times a day. |
2932b08 to
9a07a24
Compare
9a07a24 to
608f482
Compare
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.
| } 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).
608f482 to
25a1645
Compare
There 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.
| // 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.
| * @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.
| * <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 | |
| * 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> | |
| * <strong>shared:</strong> JUnit drives the lifecycle, every app reports to this one backend | |
| * <pre>{@code | |
| * @RegisterExtension | |
| * static final AgentBackend agent = AgentBackend.testAgent(); | |
| * </pre> | |
| * | |
| * <strong>owned:</strong> the app starts, resets and stops its own backend (note the annotation applies to {@code SmokeCliApp}) | |
| * <pre>{@code | |
| * @RegisterExtension | |
| * static final SmokeCliApp app = SmokeCliApp.named("my-app") | |
| * .backend(AgentBackend.testAgent()) | |
| * .build(); | |
| * }</pre> |
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.
| * | ||
| * @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.
There was a problem hiding this comment.
How would that solve the lack of clarity around the finish state?
That's for debate, I'm not sure myself, that's why I threw startBeforeAll as an idea yet I understand it's a bit vague too. Not sure how to convey shared meaning yet.
| * @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 ?
There was a problem hiding this comment.
Indeed, that's way better using a Class than a String.
I'll keep the String version (as suggested) because it might be needed for the cases where the app to run in not part of the test classpath (like a server fetch and inflated at build time).
So I'll end up with 4 methods (String mainClass) / (Class mainClass) and (String mainClass, String classpath) / (String mainClass, String classpath). This is a bit confusing but that conveys more clearly classpath can only be used with mainClass (not with jar).
About having the default values as System.getProperty('datadog.smoketest.shadowJar.path'), I would not enforce it as 1. there is already a sensible default value (the current - parent VM - classpath) and I would consider only if we can define this property automatically (gradle convention plugin?) as it requires manual setup for every smoke test build configuration for now 😞
There was a problem hiding this comment.
I would consider only if we can define this property automatically (gradle convention plugin?)
Makes sense.
25a1645 to
9d9b24d
Compare
There 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
b60f339 to
468a299
Compare
468a299 to
5922fd3
Compare
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
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