Avoid final field modifications in Cucumber and Spock instrumentations#11851
Conversation
Test Environment - sbt-scalatestJob Status: 🟢 success
Baseline: median of |
Test Environment - nebula-release-pluginJob Status: 🟢 success
Baseline: median of |
Test Environment - netflix-zuulJob Status: 🟢 success
Baseline: median of |
Test Environment - pass4sJob Status: 🟢 success
Baseline: median of |
Test Environment - reactive-streams-jvmJob Status: 🟢 success
Baseline: median of |
Test Environment - sonar-kotlinJob Status: 🟢 success
Baseline: median of |
Test Environment - jolokiaJob Status: 🟢 success
Baseline: median of |
Test Environment - okhttpJob Status: 🟢 success
Baseline: median of |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f55771bf7d
ℹ️ 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".
| CONSTRUCTOR_7_7, new EmptyConfigurationParameters(), newId, name, source, pickle); | ||
| } | ||
| if (CONSTRUCTOR_6_0 != null) { | ||
| return METHOD_HANDLES.invoke( | ||
| CONSTRUCTOR_6_0, new EmptyConfigurationParameters(), newId, name, source, pickle); |
There was a problem hiding this comment.
Preserve Cucumber parameters for retry descriptors
For Cucumber 6.0–7.23, PickleDescriptor derives its execution mode and tag-based exclusive resources from the ConfigurationParameters passed to this constructor (for example, the 7.23 source computes both from parameters: https://github.com/cucumber/cucumber-jvm/blob/v7.23.0/cucumber-junit-platform-engine/src/main/java/io/cucumber/junit/platform/engine/NodeDescriptor.java). Passing EmptyConfigurationParameters here means retries for suites that configure non-default feature execution mode or cucumber.execution.exclusive-resources.* are rebuilt with default concurrent mode and no resource locks, while the previous shallow clone preserved those fields; those retries can therefore be scheduled differently from the original scenario.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Seems like the execution mode and exclusive resources are read by the HierarchicalTestExecutorService when planning on how to run a test task. But the retry descriptors are only ever executed by us manually in the execution instrumentation through currentTask.execute() without scheduling, so the fields are never used.
Test Environment - spring_bootJob Status: 🟢 success
Baseline: median of |
Test Environment - sonar-javaJob Status: 🟢 success
Baseline: median of |
|
/datadog autotest review |
There was a problem hiding this comment.
More details
PR implements JEP 500-compliant descriptor reconstruction for Spock and Cucumber tests by avoiding final field mutations. Code is safe and correct: all null cases handled via defensive MethodHandles patterns, fallback to legacy cloning preserved, registration timing correct, and no behavioral regressions introduced.
🤖 Datadog Autotest · Commit f55771b · What is Autotest? · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: 4e8269d | Docs | Datadog PR Page | Give us feedback! |
Test Environment - heliboardJob Status: 🟢 success
Baseline: median of |
🟢 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. |
There was a problem hiding this comment.
More details
The retry-descriptor factory dispatch chain (RetryDescriptorFactories → CucumberRetryDescriptorFactory / SpockRetryDescriptorFactory → legacyCopy) is correctly wired. The legacyCopy guard condition (descriptorClone != testDescriptor && !updatedId.equals(...)) correctly prevents modifying the live descriptor on Unsafe failure. The CallDepthThreadLocalMap depth check ensures the ExecutionAdvice does not re-enter itself when the fresh retry NodeTestTask calls execute(). Seven targeted unit tests covering the registry, null-engine guard, overwrite semantics, id-suffix walk, and factory null-return fallback all pass.
📊 Validated against 7 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit f55771b · What is Autotest? · Any feedback? Reach out in #autotest
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
|
What Does This Do
RetryDescriptorFactoryfor JUnit5-based framework instrumentations.Motivation
#11752 approach to avoid final field modifications in JUnit5 only applied to
JupiterTestDescriptor, which is only used by JUnit5. Both Spock and Cucumber implement their own kind of descriptors, which still depended on the non-JEP500-compliant approach to enable execution policies.Additional Notes
Running an example project on JDK26 with
JAVA_TOOL_OPTIONS=--illegal-final-field-mutation=debugproduced the following logs:The build containing the fix doesn't produce the logs.
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 issue/merge. You can also:/merge --commit-message "..."/merge -c/merge -f --reason "reason"; please use this judiciously, as some checks do not run at the PR-level (note: the PR still needs to be mergeable, this will only skip the pre-merge build)Jira ticket: SDTEST-3892