Uh oh!
There was an error while loading. Please reload this page.
Migrate CI Visibility Smoke tests from Groovy to Java - #11439
Merged
gh-worker-dd-mergequeue-cf854d[bot] merged 9 commits intoMay 22, 2026
Conversation
…Spock to Java/JUnit 5 Drops Spock + Groovy 3 from `dd-java-agent:agent-ci-visibility:civisibility-test-fixtures` and from the three CiVisibility smoke modules (`dd-smoke-tests:gradle`, `:maven`, `:junit-console`). The fixtures' main source is now Java; the three smoke modules use JUnit 5 + `@TableTest`. `MockBackend` and `CiVisibilityInstrumentationTest` stay on Groovy/Spock and consume the new Java APIs. This unblocks running the CiVisibility smoke tests under Gradle 9, whose bundled Groovy 4 is incompatible with `spock 2.4-groovy-3.0`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Apply the rules from `.claude/skills/migrate-junit-source-to-tabletest/SKILL.md` to the freshly migrated CiVisibility smoke tests:
- Every `@TableTest` table now starts with a non-parameter `scenario` column used as the row's display name.
- `MavenSmokeTest.testJunit4ClassOrdering` drops its `String testcaseName` method parameter; the first column becomes the scenario.
- Duplicated `@TypeConverter TestFQN toTestFQN(...)` moves into `civisibility-test-fixtures` as a shared `CiVisibilityTableTestConverters`, wired via `@TypeConverterSources`.
- `GradleDaemonSmokeTest.testNew` collapses the three pairs of `test-succeed-new-instrumentation` rows that differed only on `configurationCache` into single rows via `{false, true}` value-set syntax.
- Gradle smoke tests no longer take `TestInfo` as a method parameter (which conflicted with TableTest's column-count convention); `AbstractGradleTest` captures it once in `@BeforeEach` and exposes it via an inherited `protected TestInfo testInfo` field.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>…tic prints The scenario column added in the previous commit already identifies each `@TableTest` row in JUnit 5's test output and in the JUnit XML report, so the inline timestamped `System.out.println` prefixes were redundant. Removes the inherited `TestInfo testInfo` field on `AbstractGradleTest`, the parameter-injection `@BeforeEach`, and the unused `java.util.Date` imports. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:efc0ebaf18
ℹ️ 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.
Three regressions caught by GitLab CI on the PR:
1. **ClassCastException in `CiVisibilityTestUtils.assertData` at line 229.**
`CiVisibilityInstrumentationTest.assertSpansData` builds the
`additionalReplacements` map with GString values (via
`"${instrumentedLibraryName()}:${instrumentedLibraryVersion()}"`). My port
declared the parameter as `Map<String, String>`, so the for-each loop's
implicit `String` cast on `e.getValue()` blew up under Groovy callers.
Widens both `assertData` overloads to `Map<String, ?>` and uses
`String.valueOf` when stitching the value back into a placeholder string.
2. **`verifySnapshots` was failing on `dd.spanid` / friends.**
The original Groovy `requiredLogFields.each { field -> log.containsKey(field) }`
discarded the boolean — i.e. asserted nothing. My Java port turned it into
`assertTrue(log.containsKey(field))`, which tripped on snapshots that don't
include the field. Reverts to the original (intentionally lenient)
behaviour with a TODO-style comment explaining why.
3. **forbiddenApisMain + config-inversion-linter failures on the new Java
main sources.** The original code lived in `src/main/groovy/` which the
linters don't scan; the Java port exposes them to both
`forbiddenApisMain` (`System.getenv`, `System.out`, `String.getBytes()`,
`String.replaceAll(String, String)`) and `logEnvVarUsages` (the
`"DD_CIVISIBILITY_SMOKETEST_DEBUG_*"` literals). Fixes:
- Disable `forbiddenApisMain` on `civisibility-test-fixtures` — this is a
test-support module, on the test classpath of its consumers, so the
production-code-quality gates don't apply.
- Switch the local-debug toggles to JVM system properties
(`datadog.civisibility.smoketest.debug.parent` /
`…debug.child`) so no `DD_…` literal lives in `src/main/java` for
`logEnvVarUsages` to flag.
- Keep the explicit `StandardCharsets.UTF_8` on `String.getBytes(…)` and
the precompiled `Matcher.replaceAll(...)` — both are objectively
better than the platform-default variants.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>bric3force-pushed
the
bdu/civis-groovy-to-java
branch
from
May 21, 2026 15:19
f8d3d7b to
75b54e7CompareTest Environment - sbt-scalatestJob Status: success
|
Test Environment - nebula-release-pluginJob Status: success
|
Test Environment - netflix-zuulJob Status: success
|
Test Environment - pass4sJob Status: success
|
Test Environment - reactive-streams-jvmJob Status: success
|
Test Environment - sonar-kotlinJob Status: success
|
Test Environment - jolokiaJob Status: success
|
Test Environment - okhttpJob Status: success
|
Test Environment - spring_bootJob Status: success
|
bric3
marked this pull request as draft
May 21, 2026 15:59
…izable SpotBugs flagged `SE_COMPARATOR_SHOULD_BE_SERIALIZABLE` on the stateless comparator. Add `Serializable` to the `implements` clause with a `serialVersionUID` to satisfy the check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bric3force-pushed
the
bdu/civis-groovy-to-java
branch
from
May 21, 2026 16:09
3d74b53 to
e7b65b2CompareTest Environment - sonar-javaJob Status: success
|
jpbempel
approved these changes
May 21, 2026
bric3
marked this pull request as ready for review
May 21, 2026 19:51
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit:ad8dcc480c
ℹ️ 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.
Groovy's `switch (null) { case foo: ... default: ... }` silently falls through to `default`, which is what the original `rootSpanTypeToVal` relied on for traces whose root span has no type set. The Java port called `DDSpanTypes.TEST.contentEquals(spanType)` directly, which throws NPE when `spanType` is `null` — surfacing as `4 failed tests due to NullPointerException at KarateTest.groovy:188` across every test_inst[*,6/8] and test_inst_latest[*,2/6] shard.
Adds an explicit `if (spanType == null) return 4;` short-circuit (the same value the default branch returns) and re-runs spotless to capture the verifySnapshots assertion-tightening edit reverted by the user in the IDE.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>Codex review caught a regression vs. the original Spock semantics: in Spock, `@Shared @AutoCleanup protected MockBackend mockBackend = new MockBackend()` produces one instance per spec class. My Java port made it `static final` on the abstract base — meaning a single shared `MockBackend` for both `GradleDaemonSmokeTest` and `GradleLauncherSmokeTest`. Whichever JUnit ran first would call the inherited `@AfterAll closeMockBackend()`, leaving the second class to `reset()` a closed `TestHttpServer` and talk to a dead intake URL. Switches `AbstractGradleTest` to `@TestInstance(Lifecycle.PER_CLASS)` and turns `mockBackend` and `closeMockBackend` into instance members. JUnit now creates one `AbstractGradleTest` instance per concrete subclass, with its own backend and its own close hook — matching the Spock `@Shared` lifecycle. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
bric3force-pushed
the
bdu/civis-groovy-to-java
branch
from
May 21, 2026 22:07
ad8dcc4 to
7d4c6e4Comparebric3
commented
May 21, 2026
| Map<String, Object> snapshotContent = debuggerMap.snapshot as Map<String, Object> | ||
| assert snapshotContent != null | ||
| requiredSnapshotFields.each { field -> snapshotContent.containsKey(field) } |
ContributorAuthor
There was a problem hiding this comment.
note: @DataDog/ci-app-libraries @daniel-mohedano I noticed the following code wasn't asserting anything in Groovy. And is ported the same way in its Java equivalent. It might be worh coming back to it at a later point.
Contributor
There was a problem hiding this comment.
Great catch, I'll handle it on my side in a following PR, thanks!
daniel-mohedano
approved these changes
May 22, 2026
daniel-mohedano
left a comment
Contributor
There was a problem hiding this comment.
LGTM from CIVis side, thanks a lot for the effort! 🚀
Comment on lines
+155
to
+160
| private static boolean isWithin( | ||
| ComparableVersion version, | ||
| ComparableVersion lowerInclusive, | ||
| ComparableVersion upperExclusive) { | ||
| return version.compareTo(lowerInclusive) >= 0 && version.compareTo(upperExclusive) < 0; | ||
| } |
Contributor
There was a problem hiding this comment.
nit: we can remove datadog.trace.util.ComparableVersion#isWithin then, I think it was only used here
ContributorAuthor
There was a problem hiding this comment.
I'd rather keep this focused on the conversion at this time.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Does This Do
Migrates the CiVisibility smoke-test stack from Groovy/Spock to Java + JUnit 5 +
@TableTest:dd-java-agent:agent-ci-visibility:civisibility-test-fixturesTHe other smoke test depends on the fixture
dd-smoke-tests:gradledd-smoke-tests:mavendd-smoke-tests:junit-consoleMotivation
Gradle 9 bundles Groovy 4 in
gradleApi()/gradleTestKit(). Howeverdd-smoke-tests:gradlesmoke tests rely on Groovy tests and Spock2.4-groovy-3.0, which is not compatible with Groovy 4.0 (compilation fails withIncompatibleGroovyVersionException).Bumping Groovy to 4 is not the way we want to go
Additional Notes
org.gradle.util.DistributionLocator→org.gradle.util.internal.DistributionLocator(the former package is gone in Gradle 9; the latter is present in both Gradle 8.14.5 and 9.5.1).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.