-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
[Storage] Deal with RBAC replication in live tests. #20559
[Storage] Deal with RBAC replication in live tests. #20559
Conversation
namespace Azure.Storage.Tests.Shared | ||
{ | ||
[AttributeUsage(AttributeTargets.Method, AllowMultiple = false, Inherited = false)] | ||
public class RetryOnFailedRequestAttribute : NUnitAttribute, IRepeatTest |
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 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.
I'm afraid of attributes like this :) Can we just have the RetryOnAuthorizationPermissionMismatch
and avoid giving people a gun?
return context.CurrentResult; | ||
} | ||
|
||
private bool ShouldRetry(TestResult testResult) |
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 borrowed from
private static bool IsTestFailedWithRecordingMismatch(TestExecutionContext context) |
Wonder if this is something we can address in our live resource deployment framework. Add a post-deploy script that would try to access the resource until it's available. |
We already have a bad version of it https://github.com/Azure/azure-sdk-for-net/blob/master/sdk/eventhub/test-resources-post.ps1#L15-L16 :) |
Yeah, I think it's a common enough problem that it should be solved outside the storage project. |
@pakrym lol, looks like my old workaround got copied , see azure-sdk-for-net/sdk/storage/test-resources-post.ps1 Lines 177 to 179 in bf4b77e
Here are few considerations:
|
Yeah, one conclusion might be that we put something in the test framework and maybe add a method to the TestEnvironment to verify permissions that would get executed before the assembly runs. |
Should be something where test assembly can provide a "sampling scenario" to the framework. Unfortunately, each service will have it different and the easiest way to code it is with service client in hand. I'm going to try build a simple |
Sorry, I meant a virtual placeholder that libraries can choose to override with their sampling function. |
We might not even need a fixture as we can call the method from RecorderTestBase.SetUp |
This is to address #17384 .
The idea is to annotate problematic tests with attribute that will keep retrying on
AuthorizationPermissionMismatch
.Before this I tried to use this:
azure-sdk-for-net/sdk/storage/Azure.Storage.Common/tests/Shared/StorageTestBase.cs
Lines 415 to 428 in bf4b77e
However, the amount of code involved made me cry... and we'll have more tests dependent on oauth in the future so looking for something more concise.
Scale of the problem can be seen in https://dev.azure.com/azure-sdk/internal/_build/results?buildId=849732&view=ms.vss-test-web.build-test-results-tab&runId=18450850&resultId=105746&paneView=debug .