Use pluggable clock for auto refresh cache and make unit tests fast #5767
Conversation
Signed-off-by: Jason Parraga <[email protected]>
| // If the cache tries to enqueue the item, a panic will be thrown. | ||
| time.Sleep(500 * time.Millisecond) | ||
| fakeClock.Step(testResyncPeriod) |
There was a problem hiding this comment.
These tests aren't perfect with how things are asynchronous... It's hard to verify that a sync actually happened. I wanted to add a sync counter to the auto refresh struct to use in unit tests but the constructor IMO incorrectly returns the interface instead of a pointer to the struct (which should adhere to the interface). As such, the unit tests can't access anything package level which is a bit unfortunate.
There was a problem hiding this comment.
I wanted to fix that but I'm trying to reduce the blast radius of this change.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5767 +/- ##
==========================================
+ Coverage 36.21% 36.31% +0.09%
==========================================
Files 1303 1305 +2
Lines 109644 110017 +373
==========================================
+ Hits 39710 39949 +239
- Misses 65810 65912 +102
- Partials 4124 4156 +32
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -406,3 +428,8 @@ func NewAutoRefreshCache(name string, syncCb SyncFunc, syncRateLimiter workqueue | |||
|
|
|||
| return NewAutoRefreshBatchedCache(name, SingleItemBatches, syncCb, syncRateLimiter, resyncPeriod, parallelizm, size, scope) | |||
| } | |||
|
|
|||
| func newAutoRefreshCacheWithClock(name string, syncCb SyncFunc, syncRateLimiter workqueue.RateLimiter, resyncPeriod time.Duration, | |||
There was a problem hiding this comment.
I've added these package level constructors for testing but honestly with all the arguments that can be configured on the auto refresh cache it would be nicer to have a constructor with Options. I can follow up with that in another pull request if you'd like.
There was a problem hiding this comment.
We can do that in a separate PR.
Sovietaced
changed the title
Signed-off-by: Jason Parraga <[email protected]>
| @@ -406,3 +428,8 @@ func NewAutoRefreshCache(name string, syncCb SyncFunc, syncRateLimiter workqueue | |||
|
|
|||
| return NewAutoRefreshBatchedCache(name, SingleItemBatches, syncCb, syncRateLimiter, resyncPeriod, parallelizm, size, scope) | |||
| } | |||
|
|
|||
| func newAutoRefreshCacheWithClock(name string, syncCb SyncFunc, syncRateLimiter workqueue.RateLimiter, resyncPeriod time.Duration, | |||
There was a problem hiding this comment.
We can do that in a separate PR.
Tracking issue
Closes #5711
Why are the changes needed?
Previously this unit test was relying on a real clock which makes unit tests slow and flaky due to nondeterministic execution duration. If code takes a dependency on time it should use a pluggable clock.
Before
After
What changes were proposed in this pull request?
autoRefreshand the work queue it relies were updated to use a pluggable clock. A fake clock is used for unit tests while a real clock is used under all other circumstances.Check all the applicable boxes
Related PRs
Docs link