-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[chore] [internal/aws] Change TestMapWithExpiry to explicitly test cleanup #25097
[chore] [internal/aws] Change TestMapWithExpiry to explicitly test cleanup #25097
Conversation
@@ -130,14 +139,14 @@ func TestMapWithExpiryCleanup(t *testing.T) { | |||
assert.Equal(t, 1, store.Size()) | |||
store.Unlock() | |||
|
|||
time.Sleep(time.Second + time.Millisecond) | |||
time.Sleep(time.Second * 2) |
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.
This is a significant increase. Why is this needed?
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.
There's actually no need. I reduced it even further by dropping the TTL and the sleep time together.
@@ -130,14 +139,14 @@ func TestMapWithExpiryCleanup(t *testing.T) { | |||
assert.Equal(t, 1, store.Size()) | |||
store.Unlock() | |||
|
|||
time.Sleep(time.Second + time.Millisecond) | |||
time.Sleep(time.Millisecond * 2) |
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.
Is this sleep still needed?
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.
Is there ever a chance that time.Now()
can return the same value? If so that would put a requirement for the sleep.
Description: Changes the behavior of the specific test case to be more aligned with its behavior pre #25066
Link to tracking Issue: #25095