Avoid final field modifications in Karate instrumentation#11753
Avoid final field modifications in Karate instrumentation#11753daniel-mohedano wants to merge 7 commits into
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 |
🟢 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. |
Test Environment - jolokiaJob Status: 🟢 success
Baseline: median of |
Test Environment - sonar-kotlinJob Status: 🟢 success
Baseline: median of |
Test Environment - okhttpJob Status: 🟢 success
Baseline: median of |
Test Environment - spring_bootJob Status: 🟢 success
Baseline: median of |
Test Environment - sonar-javaJob Status: 🟢 success
Baseline: median of |
Test Environment - pass4sJob Status: 🟢 success
Baseline: median of |
Test Environment - reactive-streams-jvmJob 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: 3eaec0c7b3
ℹ️ 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".
| "test-failed" | false | [TestFailedKarate] | [] | ||
| "test-retry-failed" | false | [TestFailedKarate] | [new TestFQN("[org/example/test_failed] test failed", "second scenario")] | ||
| "test-failed-then-succeed" | true | [TestFailedThenSucceedKarate] | [new TestFQN("[org/example/test_failed_then_succeed] test failed", "flaky scenario")] | ||
| "test-retry-continue-on-step-failure" | true | [TestContinueOnStepFailureKarate] | [ |
There was a problem hiding this comment.
Migrate the new retry coverage to JUnit 5
The repo instructions in AGENTS.md say, "Do not write new Groovy / Spock tests and migrate the existing one to JUnit 5 if it is written in Groovy." This new data-table case adds coverage to the existing Spock spec instead, so the change leaves the new Karate retry scenario in the framework the repo is trying to retire; please move it to a JUnit 5 test or migrate the spec.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
More details
The core behavioral change — replacing getAndResetSuppressFailures() (which reset after the first call, leaving subsequent failing steps in a continueOnStepFailure attempt unsuppressed) with shouldSuppressFailures() (no reset) — is correct and necessary. Karate 1.0.0 source confirms getFailedStep() is non-null when isFailed() is true, addStepResult() correctly propagates failure state, and the SuppressErrorAdvice is not re-triggered problematically because suppressFailures is set to false by the last retry's beforeExecute for all non-quarantined scenarios.
📊 Validated against 7 scenarios · Open Bits AI session
🤖 Datadog Autotest · Commit 3eaec0c · What is Autotest? · Any feedback? Reach out in #autotest
|
🎯 Code Coverage (details) 🔗 Commit SHA: eda96c2 | Docs | Datadog PR Page | Give us feedback! |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 2431498075535147252 took longer than expected. The current limit for the base branch 'master' is 120 minutes. |
|
/merge |
|
View all feedbacks in Devflow UI.
The expected merge time in
The merge request has been interrupted because the build 1070108256444918620 took longer than expected. The current limit for the base branch 'master' is 120 minutes. |
What Does This Do
ScenarioResultis updated through Karate's publicaddStepResultAPI, andSuppressErrorAdvicenow suppresses every failing step of a retried attempt (not just the first).Motivation
Avoid failures in future JDKs due to JEP 500: Prepare to Make Final Mean Final.
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-3867