test(e2e): use DefaultPollInterval in remaining wait.For calls#7547
Conversation
e8a2427 to
63224e7
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThree e2e helper waits now pass ChangesWait polling updates
Estimated code review effort: 1 (Trivial) | ~5 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@bobh66 wires DefaultPollInterval into the remaining wait.For calls in the e2e helpers. mind taking a look? |
jbw976
left a comment
jbw976
left a comment
There was a problem hiding this comment.
Thanks for the contribution @amarkdotdev, this looks reasonable and I don't see any reason that these checks wouldn't want to use our default poll interval vs. the e2e-framework default, so they must have been oversights.
Could you please update the PR body to use the PR template though? especially the checklist there - the current checklist in this PR looks like an agent generated one.
https://github.com/crossplane/crossplane/blob/main/.github/PULL_REQUEST_TEMPLATE.md
|
Hi @jbw976 sorry, I totally missed that template. Will fix it later when I get home, thanks for taking the time having a look at this |
|
@jbw976 Done, also ran |
63224e7 to
cb8f085
Compare
|
closing and re-opening this PR to kick CI 🙏 |
Three e2e helper functions were missing wait.WithInterval(DefaultPollInterval), unlike the rest of feature.go. Aligning them reduces inconsistent polling behavior that can contribute to flaky tests. Refs crossplane#5671 Signed-off-by: amarkdotdev <[email protected]> Signed-off-by: Jared Watts <[email protected]> Co-authored-by: Moritz Röseler <[email protected]>
jbw976
left a comment
jbw976
left a comment
There was a problem hiding this comment.
Thanks for following up @amarkdotdev, this looks good to me! CI is all green now too ✅
I actually just noticed this morning that the same fix was proposed in #7285, which we missed reviewing and the stalebot just closed today (that's how i noticed it).
Since we got traction on this PR, we will still go ahead and merge it instead of the older one. But to be fair to the original contributor @moeritze I am going to amend the commit to include them as a co-author too. Hopefully that's reasonable for you and everyone gets credit for the contribution here.
Thank you both for your efforts (and patience)! 🙇♂️
cb8f085 to
2ca15a8
Compare
2 participants
Description of your changes
Three e2e helpers in
test/e2e/funcs/feature.gocalledwait.Forwithoutwait.WithInterval(DefaultPollInterval):ClaimUnderTestMustNotChangeWithinCompositeUnderTestMustNotChangeWithinCompositeResourceMustMatchWithinEvery other
wait.Forin this file already uses the shared 500ms interval. This aligns the remaining helpers with the rest of the file so composition/claim tests don't rely on the e2e-framework default polling interval.Refs #5671
I have:
./nix.sh flake checkto ensure this PR is ready for review.Added or updated unit tests.Linked a PR or a docs tracking issue to document this change.Addedbackport release-x.ylabels to auto-backport this PR.Followed the API promotion workflow if this PR introduces, removes, or promotes an API.Need help with this checklist? See the cheat sheet.