tests: Fix Storage flakes #12605
tests: Fix Storage flakes #12605
Conversation
| // as that's meant to be the time for the change to be eventually consistent. | ||
| DateTimeOffset shouldSucceedAfter = DateTimeOffset.UtcNow | ||
| + delayUntilGuaranteedConsistency | ||
| // We add 3 seconds just to be on the safe side of clock skews. |
There was a problem hiding this comment.
Clock skews shouldn't affect this, because it's all relative to the same (local) clock, right? I think we should be able to get rid of that. It doesn't do any harm though...
There was a problem hiding this comment.
Oh yes, you are right.
| backoffMs = Math.Max(2 * backoffMs, maxBackoffMs); | ||
| } | ||
|
|
||
| }while (retry); |
There was a problem hiding this comment.
Nit: space after } and remove blank line before.
| private readonly StorageFixture _fixture; | ||
|
|
||
| public HmacKeysTest(StorageFixture fixture) => _fixture = fixture; | ||
|
|
||
| [Fact] | ||
| public void Lifecycle() | ||
| // This test is async because of the helpers for testing eventually consistent changes. | ||
| // We are testing sync production code though. |
There was a problem hiding this comment.
Presumably we could just test the async code instead? Remove a Task.CompletedTask from each lambda...
There was a problem hiding this comment.
Alternatively, we could add an overload of EventuallyAsync that accepts an Action instead of a Func<Task>
There was a problem hiding this comment.
Added the overload and updated these.
| @@ -288,6 +288,41 @@ private void CreateAndPopulateReadBucket() | |||
|
|
|||
| internal void UnregisterBucket(string bucket) => _bucketsToDelete.Remove(bucket); | |||
|
|
|||
| internal async Task EventuallyAsync(Func<Task> action, TimeSpan delayUntilGuaranteedConsistency) | |||
There was a problem hiding this comment.
Having looked at later commits:
- The calling code might be clearer if we put the timespan first
- We might want to add an overload taking an Action. (That could call this overload just by calling the action and then returning a completed task.)
There was a problem hiding this comment.
Yep, done on both.
| @@ -288,6 +288,41 @@ private void CreateAndPopulateReadBucket() | |||
|
|
|||
| internal void UnregisterBucket(string bucket) => _bucketsToDelete.Remove(bucket); | |||
|
|
|||
| internal async Task EventuallyAsync(Func<Task> action, TimeSpan delayUntilGuaranteedConsistency) | |||
There was a problem hiding this comment.
Yep, done on both.
| // as that's meant to be the time for the change to be eventually consistent. | ||
| DateTimeOffset shouldSucceedAfter = DateTimeOffset.UtcNow | ||
| + delayUntilGuaranteedConsistency | ||
| // We add 3 seconds just to be on the safe side of clock skews. |
There was a problem hiding this comment.
Oh yes, you are right.
| backoffMs = Math.Max(2 * backoffMs, maxBackoffMs); | ||
| } | ||
|
|
||
| }while (retry); |
| private readonly StorageFixture _fixture; | ||
|
|
||
| public HmacKeysTest(StorageFixture fixture) => _fixture = fixture; | ||
|
|
||
| [Fact] | ||
| public void Lifecycle() | ||
| // This test is async because of the helpers for testing eventually consistent changes. | ||
| // We are testing sync production code though. |
There was a problem hiding this comment.
Added the overload and updated these.
amanda-tarafa
force-pushed
the
storage-flakes
branch
from
90f1b95
to
a32c9a4
Compare
|
After addressing all comments, these are all green, also locally. Squashing and merging. Thanks! |
This is so we can retry to avoid flakes.
Given that changes are only eventually consistent.
Given that metadata changes are only eventually effective.
To avoid reaching bucket modification quota.
An HMAC key is not available for use up to 30 seconds after creation.
So that we know what exactly happened if there's a failure.
amanda-tarafa
force-pushed
the
storage-flakes
branch
from
a32c9a4
to
f31cf5c
Compare
@jskeet as always, one commit at a time.