-
Notifications
You must be signed in to change notification settings - Fork 236
Improve Expectation integration test #3084
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
Improve Expectation integration test #3084
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves the Expectation integration test by fixing an anti-pattern where the reconciliation context was incorrectly captured in the createExpectation() closure. The fix prevents potential memory issues and bugs related to stale primary resource data.
Key changes:
- Removed context parameter from
deploymentReadyExpectation()method to use lambda-provided context instead of captured outer context - Fixed spelling error: renamed
pathchStatusWithMessagetopatchStatusWithMessage - Improved code quality: better variable naming (
d→deployment), long literal fix (30000l→30000L), comment improvements, method reordering
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| operator-framework/src/test/java/io/javaoperatorsdk/operator/baseapi/expectation/ExpectationReconciler.java | Fixed expectation method to use lambda-provided context, corrected spelling, improved variable names and comments, reordered methods |
| docs/content/en/docs/documentation/reconciler.md | Updated documentation to remove context parameter from deploymentReadyExpectation() call |
| docs/content/en/blog/releases/v5-2-release.md | Updated blog post example to remove context parameter from deploymentReadyExpectation() call |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ork/src/test/java/io/javaoperatorsdk/operator/baseapi/expectation/ExpectationReconciler.java
Outdated
Show resolved
Hide resolved
Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
…a specific context/primary instance Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
9263ca3 to
204d438
Compare
Signed-off-by: David Sondermann <david.sondermann@hivemq.com>
204d438 to
12284d6
Compare
csviri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Absolutely, nice catch, thank you! |
From my understanding it's not optimal to use the
contextfrom the current reconciliation inExpectation.createExpectation(). This will hog on that specific instance, including the (cloned) primary resource instance that is internally stored inDefaultContext. This could have a bad impact on GC, and I think it could even lead to bugs whencontext.getPrimaryResource()is used in the expectation (since that primary resource might have outdated data).The
Expectation.createExpectation(name, predicate)provides the primary and context, that should be preferred to be used.Fixed the integration test and references in the documentation and blog post. Also fixed all warnings in the test and re-ordered the methods by their usage in the code. Fixed the time unit for the timeout (from seconds to millis).