fix(compat): restore junit5 compatibility to LocallyRunOperatorExtension by k-wall · Pull Request #3355 · operator-framework/java-operator-sdk
As described by #3354, this change restores Junit5 compatibility for users using LocallyRunOperatorExtension in a stack that has not upgraded to Junit6.
Copilot AI review requested due to automatic review settings
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR adds a runtime fallback for registering CrdCleanup to retain compatibility across different JUnit 5 ExtensionContext.Store API versions.
Changes:
- Introduced a
try/catch (NoSuchMethodError)fallback fromcomputeIfAbsenttogetOrComputeIfAbsent. - Refactored store access into a local
storevariable for reuse.
Comment on lines +366 to +371
| try { | ||
| store.computeIfAbsent(CrdCleanup.class, ignored -> new CrdCleanup(deleteCRDs)); | ||
| } catch (NoSuchMethodError nsme) { | ||
| // Exists to retains compatibility with Junit5. | ||
| store.getOrComputeIfAbsent(CrdCleanup.class, ignored -> new CrdCleanup(deleteCRDs)); | ||
| } |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not too convinced by Copilot's advice here. The reminder why the call to the deprecated method is made is helpful IMO. Using reflection would be overkill.
Signed-off-by: Keith Wall <kwall@apache.org>
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, since this should not break JUnit 6, I don't see why not merge it.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
