-
Notifications
You must be signed in to change notification settings - Fork 324
Bump Groovy: 3.0.25 -> 4.0.29 #9745
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
3866a54
f128d3c
9dffccd
d88be22
51da522
fb2f81e
af5d06d
765fd92
1ca9595
226a2c5
a429354
ad15665
8fdf26b
e687aa9
6da67dd
69ccd4a
7c04a1c
5b73524
7a0bd21
e863a91
76dcdd8
ca64f39
27c46bc
ba98146
2b04db3
612eae0
0196afe
0cf4bb1
97a76dc
d3d8f43
be3215f
7ebb3db
d03fee4
bf02b3f
9a038e9
7b7df33
e9128d5
f3152e3
df4602c
aa5bcbf
9352e18
aa1c447
d5952a9
c05bf3c
0c6a9a4
4fedc8f
7d882b5
a9856a1
17998ec
6a4b28a
8b8ba20
6d030ce
314ba85
5af7b01
a647df7
7ff5d91
c692c4b
6c13302
d1c1df6
4327f62
31ebf5f
b284f24
b47a10c
ba933b1
925dc31
f5167a7
3345306
2981bdc
e0bf985
5c4823a
15d93d0
adced87
71c1ad1
7fc79c1
a312df4
8985acb
529724f
94a2407
09d4f71
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -53,12 +53,12 @@ class DirectAllocationTrackingTest extends InstrumentationSpecification { | |||||
| def sample = directAllocations.find({ it.getEventType().name.equals("datadog.DirectAllocationSample")}) | ||||||
| sample.getLong("allocated") == 20 | ||||||
| sample.getString("source") == "MMAP" | ||||||
| sample.getString("allocatingClass") == "org.codehaus.groovy.runtime.callsite.PlainObjectMetaMethodSite" | ||||||
| sample.getString("allocatingClass") == "org.codehaus.groovy.vmplugin.v8.IndyInterface" // With Groovy 3 it was "org.codehaus.groovy.runtime.callsite.PlainObjectMetaMethodSite" | ||||||
| sample.getLong("spanId") == expectedSpanId.get() | ||||||
| def total = directAllocations.find({ it.getEventType().name.equals("datadog.DirectAllocationTotal")}) | ||||||
| total.getLong("allocated") == 20 | ||||||
| total.getString("source") == "MMAP" | ||||||
| total.getString("allocatingClass") == "org.codehaus.groovy.runtime.callsite.PlainObjectMetaMethodSite" | ||||||
| total.getString("allocatingClass") == "org.codehaus.groovy.vmplugin.v8.IndyInterface" // With Groovy 3 it was "org.codehaus.groovy.runtime.callsite.PlainObjectMetaMethodSite" | ||||||
|
|
||||||
| cleanup: | ||||||
| recording.close() | ||||||
|
|
@@ -80,12 +80,12 @@ class DirectAllocationTrackingTest extends InstrumentationSpecification { | |||||
| def sample = directAllocations.find({ it.getEventType().name.equals("datadog.DirectAllocationSample")}) | ||||||
| sample.getLong("allocated") == 10 | ||||||
| sample.getString("source") == "ALLOCATE_DIRECT" | ||||||
| sample.getString("allocatingClass") == "java_nio_ByteBuffer\$allocateDirect" | ||||||
| sample.getString("allocatingClass") == "org.codehaus.groovy.vmplugin.v8.IndyInterface" // With Groovy 3 it was "java_nio_ByteBuffer\$allocateDirect" | ||||||
| sample.getLong("spanId") == expectedSpanId.get() | ||||||
| def total = directAllocations.find({ it.getEventType().name.equals("datadog.DirectAllocationTotal")}) | ||||||
| total.getLong("allocated") == 10 | ||||||
| total.getString("source") == "ALLOCATE_DIRECT" | ||||||
| total.getString("allocatingClass") == "java_nio_ByteBuffer\$allocateDirect" | ||||||
| total.getString("allocatingClass") == "org.codehaus.groovy.vmplugin.v8.IndyInterface"// With Groovy 3 it was "java_nio_ByteBuffer\$allocateDirect" | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. typo:
Suggested change
|
||||||
|
|
||||||
| cleanup: | ||||||
| recording.close() | ||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -32,6 +32,14 @@ dependencies { | |
| latestDepTestImplementation group: 'org.spockframework', name: 'spock-core', version: '2.+' | ||
| } | ||
|
|
||
| // Exclude Groovy 4 to test old Spock versions, compatible with Groovy 3. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can look into doing the upgrade in the future. After the latest Spock upgrade to 2.4 in the tracer that became the minimum Spock version being tested for the instrumentation (older versions are still tested through smoke tests and/or test environment), so we're quite up-to-date. I'll have to check if the groovy version impacts the Spock instrumentation in any way
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @daniel-mohedano Let's do it as separate PR once this will be merged?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Definitely, yes. I'll make a note to investigate the impact on the Spock instrumentation to prepare for the changes 👍 |
||
| // TODO: Check if we can either drop old versions or support both Groovy 3 & Groovy 4. | ||
AlexeyKuznetsov-DD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| configurations { | ||
| testImplementation { | ||
| exclude group: 'org.apache.groovy' | ||
| } | ||
| } | ||
|
|
||
| configurations.matching({ it.name.startsWith('test') }).configureEach({ | ||
| it.resolutionStrategy { | ||
| force group: 'org.junit.platform', name: 'junit-platform-launcher', version: libs.versions.junit.platform.get() | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,6 +8,18 @@ plugins { | |
| apply from: "$rootDir/gradle/java.gradle" | ||
| description = 'Gradle Daemon Instrumentation Smoke Tests.' | ||
|
|
||
| // Gradle 8.14.3 bundles Groovy 3 via `gradleTestKit()`, so we need Spock for Groovy 3 instead of Groovy 4. | ||
| // Gradle 9.x bundles Groovy 4, so eventually we can drop this workaround. | ||
| configurations.configureEach { | ||
| // Exclude Groovy 4. Starting from v4 it has group: `org.apache.groovy`. | ||
| exclude group: 'org.apache.groovy' | ||
|
|
||
| resolutionStrategy { | ||
| // Force Spock for Groovy 3. | ||
| force "org.spockframework:spock-core:2.4-groovy-3.0" | ||
| } | ||
| } | ||
|
Comment on lines
+11
to
+21
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. issue: Instead of doing this for all configurations, I would instead target specific ones.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will try to adjust. |
||
|
|
||
| dependencies { | ||
| testImplementation gradleTestKit() | ||
| testImplementation project(':dd-smoke-tests:backend-mock') | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -310,7 +310,7 @@ public boolean isForceKeep() { | |
| * | ||
| * @return true if root, false otherwise | ||
| */ | ||
| public final boolean isRootSpan() { | ||
| public final boolean checkRootSpan() { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❔ question: This is a rather important change to the Tracing API. Why change this API for a Groovy upgrade?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I need to verify this, but could this have to do with G4 being stricter or because it promotes this to a "property" (I.e. consumed as
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So better introduce a
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, this is a Groovy4 issue. we have 2 methods:
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If it works, that sounds like an easy win!
Thanks 🙏 |
||
| return context.getParentId() == DDSpanId.ZERO; | ||
| } | ||
|
|
||
|
|
@@ -330,7 +330,7 @@ public DDSpan getLocalRootSpan() { | |
| * | ||
| * @return {@literal true} if this span is the same as {@linkplain #getLocalRootSpan()} | ||
| */ | ||
| public boolean isLocalRootSpan() { | ||
| public boolean checkLocalRootSpan() { | ||
AlexeyKuznetsov-DD marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| return getLocalRootSpan().equals(this); | ||
| } | ||
|
|
||
|
|
@@ -382,7 +382,7 @@ private boolean isExceptionReplayEnabled() { | |
| boolean captureOnlyRootSpan = | ||
| (Config.get().isDebuggerExceptionOnlyLocalRoot() | ||
| || !Config.get().isDebuggerExceptionCaptureIntermediateSpansEnabled()); | ||
| if (captureOnlyRootSpan && !isLocalRootSpan()) { | ||
| if (captureOnlyRootSpan && !checkLocalRootSpan()) { | ||
| return false; | ||
| } | ||
| return true; | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.