-
Notifications
You must be signed in to change notification settings - Fork 96
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
Get correct leader election holder ID in upgrade test #1994
Conversation
I mentioned possibly re-running the other automated tests to see if they are still functioning correctly in the linked issue, do you think that would be unnecessary / out of scope for this issue? |
@bjee19 I'll rerun the whole suite and open tickets if there are any other failures 👍🏼 |
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
Thanks @ciarams87! If you could list the tests you re-ran somewhere I think that'll be good to have verified at least. You're probably not going to do the extended duration tests, so knowing which ones you did I think would be good for release time. |
e1f63cd
to
1eb63f4
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1994 +/- ##
==========================================
+ Coverage 87.66% 95.00% +7.33%
==========================================
Files 93 1 -92
Lines 6557 220 -6337
Branches 50 50
==========================================
- Hits 5748 209 -5539
+ Misses 753 11 -742
+ Partials 56 0 -56 ☔ View full report in Codecov by Sentry. |
Proposed changes
Problem: The upgrade NFR test is not working.
Solution: The issue is around the leader election lease holder ID not being equal to one of the upgraded pod names, which is what the test expects. Instead, the holder ID is in the format
<pod-name_uid>
. The fix is to split the holder ID on the underscore, and then instead make sure the first part of the string matches one of the upgraded pod names.Also added
defer GinkgoRecover()
so that Ginkgo handles its internal panic correctly when an assertion fails within this nested function. See https://onsi.github.io/ginkgo/#mental-model-how-ginkgo-handles-failure for more info.Testing: Ran the test locally and confirmed it now passes successfully.
Closes #1891
Checklist
Before creating a PR, run through this checklist and mark each as complete.
Release notes
If this PR introduces a change that affects users and needs to be mentioned in the release notes,
please add a brief note that summarizes the change.