Skip to content

Commit 82fac88

Browse files
authored
Merge branch 'master' into alejandro.gonzalez/APPSEC-61554-flaky-test
2 parents 86745ad + 1108a4f commit 82fac88

16 files changed

Lines changed: 524 additions & 224 deletions

File tree

.claude/skills/migrate-groovy-to-java/SKILL.md

Lines changed: 13 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -5,16 +5,19 @@ description: migrate test groovy files to java
55

66
Migrate test Groovy files to Java using JUnit 5
77

8-
1. List all groovy files of the current gradle module
9-
2. convert groovy files to Java using Junit 5
10-
3. make sure the tests are still passing after migration
11-
4. remove groovy files
8+
1. List all Groovy files of the current Gradle module
9+
2. Convert Groovy files to Java using JUnit 5
10+
3. Make sure the tests are still passing after migration and that the test count has not changed
11+
4. Remove Groovy files
12+
5. Add the migrated module path(s) to `.github/g2j-migrated-modules.txt`
1213

13-
When converting groovy code to java code make sure that:
14-
- the Java code generated is compatible with JDK 8
15-
- when translating Spock test, favor using `@CsvSource` with `|` delimiters
16-
- when using a `@MethodSource`, use the test method name, and suffix it with `_arguments`
17-
- when converting tuples, create light dedicated structure instead to keep the typing system
14+
When converting Groovy code to Java code, make sure that:
15+
- The Java code generated is compatible with JDK 8
16+
- When translating Spock tests, favor using `@CsvSource` with `|` delimiters
17+
- When using `@MethodSource`, name the arguments method by appending `Arguments` using camelCase to the test method name (e.g. `testMethodArguments`)
18+
- Ensure parameterized test names are human-readable (i.e. no hashcodes); instead add a description string as the first `Arguments.of(...)` value or index the test case
19+
- When converting tuples, create a light dedicated structure instead to keep the typing system
1820
- Instead of checking a state and throwing an exception, use JUnit asserts
19-
- Do not wrap checked exception and throwing a Runtime exception, prefer adding a throws clause at method declaration
21+
- Do not wrap checked exceptions and throw a Runtime exception; prefer adding a throws clause at method declaration
2022
- Do not mark local variables `final`
23+
- Ensure variables are human-readable; avoid single-letter names and pre-define variables that are referenced multiple times

.github/g2j-migrated-modules.txt

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,9 @@
1+
# This file lists modules that have been migrated from Groovy to Java / JUnit 5.
2+
# New *.groovy files under any src/<*test*>/groovy/ path
3+
# in these modules will fail the 'enforce-groovy-migration' PR check.
4+
#
5+
# After a module is migrated, add it on a new line here.
6+
# Use the filesystem path prefix as seen below.
7+
8+
buildSrc/call-site-instrumentation-plugin
9+
components/json

.github/workflows/README.md

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -123,6 +123,19 @@ _Action:_
123123

124124
_Recovery:_ Check at the milestone for the related issues and update them manually.
125125

126+
### enforce-groovy-migration [🔗](enforce-groovy-migration.yaml)
127+
128+
_Trigger:_ When creating or updating a pull request targeting `master`, or when labels are updated.
129+
130+
_Actions:_
131+
132+
* Fail the PR if a new Groovy test file is added to a module listed in [`.github/g2j-migrated-modules.txt`](../g2j-migrated-modules.txt) (hard enforcement),
133+
* Post a warning comment on the PR if a new Groovy test file is added to any other non-exempt module (soft warning). Instrumentation (`dd-java-agent/instrumentation/`) and smoke-test (`dd-smoke-tests/`) modules are exempt from this warning.
134+
135+
_Recovery:_ Re-write the Groovy test files in Java / JUnit 5. To override this check entirely, add the `tag: override-groovy-enforcement` label to the PR. Remove the label to re-enable enforcement.
136+
137+
_Notes:_ The migrated modules list is always read from `master`. Add a new entry to `.github/g2j-migrated-modules.txt` each time a module is migrated to Java / JUnit 5.
138+
126139
## Code Quality and Security
127140

128141
### analyze-changes [🔗](analyze-changes.yaml)
Lines changed: 169 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,169 @@
1+
name: Enforce Groovy Migration
2+
on:
3+
pull_request:
4+
types: [opened, edited, ready_for_review, labeled, unlabeled, synchronize]
5+
branches:
6+
- master
7+
8+
concurrency:
9+
group: ${{ github.workflow }}-${{ github.ref }}
10+
cancel-in-progress: true
11+
12+
jobs:
13+
enforce_groovy_migration:
14+
name: Enforce Groovy migration
15+
permissions:
16+
issues: write # Required to create a comment on the pull request
17+
pull-requests: write # Required to create a comment on the pull request
18+
contents: read # Required to read migrated modules file
19+
runs-on: ubuntu-latest
20+
steps:
21+
- name: Check for Groovy regressions
22+
uses: actions/github-script@ed597411d8f924073f98dfc5c65a23a2325f34cd # 8.0.0
23+
with:
24+
github-token: ${{ secrets.GITHUB_TOKEN }}
25+
script: |
26+
// Skip draft pull requests
27+
if (context.payload.pull_request.draft) {
28+
return
29+
}
30+
31+
// Check for override label — skip all checks if label present
32+
const labels = context.payload.pull_request.labels.map(l => l.name)
33+
if (labels.includes('tag: override-groovy-enforcement')) {
34+
console.log('tag: override-groovy-enforcement label detected — skipping all checks.')
35+
return
36+
}
37+
38+
// Read migrated modules list from master
39+
const migratedMods = await github.rest.repos.getContent({
40+
owner: context.repo.owner,
41+
repo: context.repo.repo,
42+
path: '.github/g2j-migrated-modules.txt',
43+
ref: 'master'
44+
})
45+
const migratedPrefixes = Buffer.from(migratedMods.data.content, 'base64')
46+
.toString()
47+
.split('\n')
48+
.map(l => l.trim())
49+
.filter(l => l && !l.startsWith('#'))
50+
51+
// Get all files changed in this PR
52+
const allFiles = await github.paginate(github.rest.pulls.listFiles, {
53+
owner: context.repo.owner,
54+
repo: context.repo.repo,
55+
pull_number: context.payload.pull_request.number
56+
})
57+
58+
// Filter these changed files to newly added Groovy files in any test source set
59+
const addedGroovy = allFiles.filter(f =>
60+
f.status === 'added' &&
61+
/\/src\/[^/]*[tT]est[^/]*\/groovy\/.*\.groovy$/.test(f.filename)
62+
)
63+
64+
// Extract module prefix from file path (everything before /src/(test|testFixtures)/groovy/)
65+
const moduleOf = path => {
66+
const m = path.match(/^(.*?)\/src\/(test|testFixtures)\/groovy\//)
67+
return m ? m[1] : null
68+
}
69+
70+
// Classify each added Groovy file
71+
const regressions = []
72+
const warnings = []
73+
for (const file of addedGroovy) {
74+
const path = file.filename
75+
const mod = moduleOf(path)
76+
if (migratedPrefixes.some(prefix => path.startsWith(prefix + '/'))) {
77+
regressions.push({ path, mod })
78+
} else if (
79+
path.startsWith('dd-java-agent/instrumentation/') ||
80+
path.startsWith('dd-smoke-tests/')
81+
) {
82+
// ignore Groovy file additions to instrumentations and smoke-tests for now
83+
} else {
84+
warnings.push({ path, mod })
85+
}
86+
}
87+
88+
// Fetch existing comments once
89+
const comments = await github.rest.issues.listComments({
90+
issue_number: context.payload.pull_request.number,
91+
owner: context.repo.owner,
92+
repo: context.repo.repo
93+
})
94+
95+
const regressionMarker = '<!-- dd-trace-java-groovy-regression -->'
96+
const warningMarker = '<!-- dd-trace-java-groovy-warning -->'
97+
const existingRegressionComment = comments.data.find(c => c.body.includes(regressionMarker))
98+
const existingWarningComment = comments.data.find(c => c.body.includes(warningMarker))
99+
100+
// Handle regression comment
101+
if (regressions.length > 0) {
102+
const fileList = regressions
103+
.map(({ path, mod }) => `- \`${path}\` (module: \`${mod}\`)`)
104+
.join('\n')
105+
const body = `**❌ Groovy Test Regression Detected**\n\n` +
106+
`The following files add Groovy tests to modules that have been fully migrated to Java / JUnit 5:\n\n` +
107+
`${fileList}\n\n` +
108+
`These modules no longer accept Groovy test files. Please rewrite the test in Java / JUnit 5 instead.\n\n` +
109+
regressionMarker
110+
if (existingRegressionComment) {
111+
await github.rest.issues.updateComment({
112+
comment_id: existingRegressionComment.id,
113+
owner: context.repo.owner,
114+
repo: context.repo.repo,
115+
body
116+
})
117+
} else {
118+
await github.rest.issues.createComment({
119+
issue_number: context.payload.pull_request.number,
120+
owner: context.repo.owner,
121+
repo: context.repo.repo,
122+
body
123+
})
124+
}
125+
} else if (existingRegressionComment) {
126+
await github.rest.issues.deleteComment({
127+
comment_id: existingRegressionComment.id,
128+
owner: context.repo.owner,
129+
repo: context.repo.repo
130+
})
131+
}
132+
133+
// Handle warning comment
134+
if (warnings.length > 0) {
135+
const fileList = warnings
136+
.map(({ path, mod }) => `- \`${path}\` (module: \`${mod}\`)`)
137+
.join('\n')
138+
const body = `**⚠️ New Groovy Test Files Added**\n\n` +
139+
`The following files add Groovy tests to modules that are candidates for migration to Java / JUnit 5:\n\n` +
140+
`${fileList}\n\n` +
141+
`Consider writing these tests in Java / JUnit 5 instead to help with the ongoing migration effort.\n\n` +
142+
warningMarker
143+
if (existingWarningComment) {
144+
await github.rest.issues.updateComment({
145+
comment_id: existingWarningComment.id,
146+
owner: context.repo.owner,
147+
repo: context.repo.repo,
148+
body
149+
})
150+
} else {
151+
await github.rest.issues.createComment({
152+
issue_number: context.payload.pull_request.number,
153+
owner: context.repo.owner,
154+
repo: context.repo.repo,
155+
body
156+
})
157+
}
158+
} else if (existingWarningComment) {
159+
await github.rest.issues.deleteComment({
160+
comment_id: existingWarningComment.id,
161+
owner: context.repo.owner,
162+
repo: context.repo.repo
163+
})
164+
}
165+
166+
// Fail the check if there are regressions
167+
if (regressions.length > 0) {
168+
core.setFailed(`${regressions.length} Groovy regression(s) detected in migrated module(s). See PR comment for details. To skip this check entirely, add the 'tag: override-groovy-enforcement' label.`)
169+
}
Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,11 @@
11
package datadog.gradle.plugin.instrument
22

3+
import org.gradle.api.file.ConfigurableFileCollection
34
import org.gradle.api.file.DirectoryProperty
45
import org.gradle.api.provider.ListProperty
56

67
abstract class BuildTimeInstrumentationExtension {
78
abstract ListProperty<String> getPlugins()
89
abstract ListProperty<DirectoryProperty> getAdditionalClasspath()
10+
abstract ConfigurableFileCollection getIncludeClassDirectories()
911
}

buildSrc/src/main/groovy/datadog/gradle/plugin/instrument/BuildTimeInstrumentationPlugin.groovy

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,6 +126,7 @@ class BuildTimeInstrumentationPlugin implements Plugin<Project> {
126126
it.inputs.property("javaVersion", javaVersion)
127127
it.inputs.property("plugins", extension.plugins)
128128
it.inputs.files(extension.additionalClasspath)
129+
it.inputs.files(extension.includeClassDirectories)
129130

130131
// Temporary location for raw (un-instrumented) classes
131132
DirectoryProperty tmpUninstrumentedClasses = project.objects.directoryProperty().value(
@@ -149,7 +150,8 @@ class BuildTimeInstrumentationPlugin implements Plugin<Project> {
149150
extension.plugins,
150151
instrumentingClassPath,
151152
it.destinationDirectory,
152-
tmpUninstrumentedClasses
153+
tmpUninstrumentedClasses,
154+
extension.includeClassDirectories
153155
)
154156
)
155157
logger.info("[BuildTimeInstrumentationPlugin] Configured post-compile instrumentation for $compileTaskName for source-set $sourceSetName")

buildSrc/src/main/groovy/datadog/gradle/plugin/instrument/InstrumentAction.groovy

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,15 @@ abstract class InstrumentAction implements WorkAction<InstrumentWorkParameters>
4444
from(classesDirectory)
4545
into(tmpUninstrumentedDir)
4646
}
47+
// Merge any additional class directories (e.g. unpacked dependency JARs) to be processed
48+
parameters.includeClassDirectories.files.each { classesDir ->
49+
if (classesDir.exists()) {
50+
fileSystemOperations.copy {
51+
from(classesDir)
52+
into(tmpUninstrumentedDir)
53+
}
54+
}
55+
}
4756
fileSystemOperations.delete {
4857
delete(objects.fileTree().from(classesDirectory))
4958
}

buildSrc/src/main/groovy/datadog/gradle/plugin/instrument/InstrumentPostProcessingAction.groovy

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -35,30 +35,33 @@ abstract class InstrumentPostProcessingAction implements Action<AbstractCompile>
3535
final FileCollection instrumentingClassPath
3636
final DirectoryProperty compilerOutputDirectory
3737
final DirectoryProperty tmpDirectory
38+
final FileCollection includeClassDirectories
3839

3940
@Inject
4041
InstrumentPostProcessingAction(
4142
String javaVersion,
4243
ListProperty<String> plugins,
4344
FileCollection instrumentingClassPath,
4445
DirectoryProperty compilerOutputDirectory,
45-
DirectoryProperty tmpDirectory
46+
DirectoryProperty tmpDirectory,
47+
FileCollection includeClassDirectories
4648
) {
4749
this.javaVersion = javaVersion == BuildTimeInstrumentationPlugin.DEFAULT_JAVA_VERSION ? JavaLanguageVersion.current() : JavaLanguageVersion.of(javaVersion)
4850
this.plugins = plugins
4951
this.instrumentingClassPath = instrumentingClassPath
5052
this.compilerOutputDirectory = compilerOutputDirectory
5153
this.tmpDirectory = tmpDirectory
54+
this.includeClassDirectories = includeClassDirectories
5255
}
5356

5457
@Override
5558
void execute(AbstractCompile task) {
5659
logger.info(
5760
"""
58-
[InstrumentPostProcessingAction] About to instrument classes
59-
javaVersion=${javaVersion},
60-
plugins=${plugins.get()},
61-
instrumentingClassPath=${instrumentingClassPath.files},
61+
[InstrumentPostProcessingAction] About to instrument classes
62+
javaVersion=${javaVersion},
63+
plugins=${plugins.get()},
64+
instrumentingClassPath=${instrumentingClassPath.files},
6265
rawClassesDirectory=${compilerOutputDirectory.get().asFile}
6366
""".stripIndent()
6467
)
@@ -72,6 +75,7 @@ abstract class InstrumentPostProcessingAction implements Action<AbstractCompile>
7275
parameters.instrumentingClassPath.setFrom(postCompileAction.instrumentingClassPath)
7376
parameters.compilerOutputDirectory.set(postCompileAction.compilerOutputDirectory)
7477
parameters.tmpDirectory.set(postCompileAction.tmpDirectory)
78+
parameters.includeClassDirectories.setFrom(postCompileAction.includeClassDirectories)
7579
})
7680
}
7781

buildSrc/src/main/groovy/datadog/gradle/plugin/instrument/InstrumentWorkParameters.groovy

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,4 +14,5 @@ interface InstrumentWorkParameters extends WorkParameters {
1414
ConfigurableFileCollection getInstrumentingClassPath()
1515
DirectoryProperty getCompilerOutputDirectory()
1616
DirectoryProperty getTmpDirectory()
17+
ConfigurableFileCollection getIncludeClassDirectories()
1718
}

0 commit comments

Comments
 (0)