test: add integration tests for work applier exponential backoff behavior #191
Conversation
Signed-off-by: michaelawyu <[email protected]>
| // Track the observation timestamp of the diff details. | ||
| lastDiffObservedTime = &work.Status.ManifestConditions[0].DiffDetails.ObservationTime | ||
| return nil | ||
| }, eventuallyDuration, eventuallyInterval).Should(Succeed(), "Failed to update Work object status") |
There was a problem hiding this comment.
eventuallyInterval is 1 second, I wonder if it will make it flaky?
There was a problem hiding this comment.
Hi Ryan! These tests run with a special exponential backoff setup (10 sec delay first, then 20 secs with 1.5x slow backoff, and eventually 30 seconds with 2x fast backoff). The results are expected to stay the same for at least 10 seconds after first apply, so it should be alright.
| // Need to poll a bit longer to avoid flakiness. | ||
| Eventually(diffObservationTimeChangedActual(workName, wantWorkStatus, curDiffObservedTime, lastDiffObservedTime), eventuallyDuration*2, eventuallyInterval).Should(Or(Succeed())) | ||
|
|
||
| // Fixed delay is set to 10 seconds. Give a one-sec leeway to avoid flakiness. |
There was a problem hiding this comment.
I thought it was 5 sec? Where is 20 second set?
There was a problem hiding this comment.
Hi Ryan! Yeah, for the actual setup the default value is 5 seconds; this one has to use a longer one as the default values (not the initial 5 second part, but the slow/fast backoffs do not play well with our integration tests -> can be flaky sometimes).
There was a problem hiding this comment.
Hi Ryan! I've pinpointed the test-specific setup in the comment down below.
| Expect(err).ToNot(HaveOccurred()) | ||
|
|
||
| workApplier = NewReconciler( | ||
| superLongExponentialBackoffRateLimiter := NewRequeueMultiStageWithExponentialBackoffRateLimiter( |
There was a problem hiding this comment.
The test specific setup is added here; PTAL.
|
Resolved the conflict. |
Signed-off-by: michaelawyu <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: michaelawyu <[email protected]>
Signed-off-by: michaelawyu <[email protected]>
| ## workaround to bypass the pkg/controllers/workv1alpha1 tests failure | ||
| ## rollout controller tests need a bit longer to complete, so we increase the timeout | ||
| ## | ||
| # Set up the timeout parameters as some of the test lengths have exceeded the default 10 minute mark. |
There was a problem hiding this comment.
where is the timeout set in this PR?
There was a problem hiding this comment.
Hi Ryan! A previous PR has already set the timeout to 20 mins, so the changes have been absolved.
There was a problem hiding this comment.
The default is 10 minutes, #182 has set it to 20 minutes and got merged before this PR
| StopTrying("the work object status has changed unexpectedly").Wrap(fmt.Errorf("work status diff (-got, +want):\n%s", diff)).Now() | ||
| } | ||
|
|
||
| curDriftObservedTime.Time = work.Status.ManifestConditions[0].DriftDetails.ObservationTime.Time |
There was a problem hiding this comment.
Why pass in the curDriftObservedTime when its value is set inside the function. Looks like this is more of an "out" parameter or return value?
There was a problem hiding this comment.
Hi Ryan! Some of test steps later verify the currently observed timestamp:
It("should wait for the fixed delay period of time before next reconciliation", func() {
curDiffObservedTime := &metav1.Time{}
// Need to poll a bit longer to avoid flakiness.
Eventually(diffObservationTimeChangedActual(workName, wantWorkStatus, curDiffObservedTime, lastDiffObservedTime), eventuallyDuration*2, eventuallyInterval).Should(Or(Succeed()))
// Fixed delay is set to 10 seconds. Give a one-sec leeway to avoid flakiness.
Expect(curDiffObservedTime.Sub(lastDiffObservedTime.Time)).To(And(
BeNumerically(">=", time.Second*9),
BeNumerically("<=", time.Second*11),
), "the interval between two observations is not as expected")
// Update the tracked observation time.
lastDiffObservedTime = curDiffObservedTime
})
You are right that it works more like a returned result rather than an passed-in arguement, but the Ginkgo Eventually blocks make it a bit difficult to format that way 😞
|
Merged the PR to unblock progress -> can open new PRs if changes are needed. |
Description of your changes
This PR adds additional integration tests for verifying the work applier exponential backoff behavior.
Fixes #
I have:
make reviewableto ensure this PR is ready for review.How has this code been tested
N/A
Special notes for your reviewer
N/A