-
Notifications
You must be signed in to change notification settings - Fork 10.1k
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
Add retry support #33230
Add retry support #33230
Changes from 8 commits
cc61999
61f740f
451bb7e
0dadff4
a962382
a96c459
0aff7ef
6cc7b9b
57a1750
e45c0e5
ab97017
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
@@ -0,0 +1,27 @@ | ||||
// Copyright (c) .NET Foundation. All rights reserved. | ||||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||||
|
||||
using System; | ||||
using System.ComponentModel; | ||||
|
||||
namespace Microsoft.AspNetCore.Testing | ||||
{ | ||||
/// <summary> | ||||
/// Runs a test multiple times when it fails | ||||
/// This can be used on an assembly, class, or method name. Requires using the AspNetCore test framework. | ||||
/// </summary> | ||||
[EditorBrowsable(EditorBrowsableState.Never)] | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this because this ships in a user visible package? Making it Never makes it really hard to type this in regular code and gives some people PTSD (@rynowak). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I just copied
|
||||
[AttributeUsage(AttributeTargets.Method | AttributeTargets.Class | AttributeTargets.Assembly, AllowMultiple = false)] | ||||
public class RetryAttribute : Attribute | ||||
HaoK marked this conversation as resolved.
Show resolved
Hide resolved
|
||||
{ | ||||
public RetryAttribute(int maxRetries = 3) | ||||
{ | ||||
MaxRetries = maxRetries; | ||||
} | ||||
|
||||
/// <summary> | ||||
/// The maximum number of times to retry a failed test. Defaults to 3. | ||||
/// </summary> | ||||
public int MaxRetries { get; } | ||||
} | ||||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -45,7 +45,16 @@ await Aggregator.RunAsync(async () => | |
} | ||
}); | ||
|
||
var time = await base.InvokeTestMethodAsync(testClassInstance); | ||
var retryAttribute = GetRetryAttribute(TestMethod); | ||
var time = 0.0M; | ||
if (retryAttribute == null) | ||
{ | ||
time = await base.InvokeTestMethodAsync(testClassInstance); | ||
} | ||
else | ||
{ | ||
time = await RetryAsync(retryAttribute, testClassInstance); | ||
} | ||
|
||
await Aggregator.RunAsync(async () => | ||
{ | ||
|
@@ -59,6 +68,45 @@ await Aggregator.RunAsync(async () => | |
return time; | ||
} | ||
|
||
protected async Task<decimal> RetryAsync(RetryAttribute retryAttribute, object testClassInstance) | ||
{ | ||
var attempts = 0; | ||
var timeTaken = 0.0M; | ||
for (attempts = 0; attempts < retryAttribute.MaxRetries; attempts++) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please remind me: Will this loop retry an entire There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is a per test case/fact (at least that's the intent), I'm pretty certain of that, since there was a different extensibility point I was looking at in a different iteration of trying to implement this. |
||
{ | ||
timeTaken = await base.InvokeTestMethodAsync(testClassInstance); | ||
if (!Aggregator.HasExceptions) | ||
JamesNK marked this conversation as resolved.
Show resolved
Hide resolved
|
||
{ | ||
return timeTaken; | ||
} | ||
else if (attempts < retryAttribute.MaxRetries - 1) | ||
{ | ||
_testOutputHelper.WriteLine($"Retrying test, attempt {attempts} of {retryAttribute.MaxRetries} failed."); | ||
await Task.Delay(5000); | ||
Aggregator.Clear(); | ||
HaoK marked this conversation as resolved.
Show resolved
Hide resolved
HaoK marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
} | ||
|
||
return timeTaken; | ||
} | ||
|
||
private RetryAttribute GetRetryAttribute(MethodInfo methodInfo) | ||
{ | ||
var attributeCandidate = methodInfo.GetCustomAttribute<RetryAttribute>(); | ||
if (attributeCandidate != null) | ||
{ | ||
return attributeCandidate; | ||
} | ||
|
||
attributeCandidate = methodInfo.DeclaringType.GetCustomAttribute<RetryAttribute>(); | ||
if (attributeCandidate != null) | ||
{ | ||
return attributeCandidate; | ||
} | ||
|
||
return methodInfo.DeclaringType.Assembly.GetCustomAttribute<RetryAttribute>(); | ||
} | ||
|
||
private static IEnumerable<ITestMethodLifecycle> GetLifecycleHooks(object testClassInstance, Type testClass, MethodInfo testMethod) | ||
{ | ||
foreach (var attribute in testMethod.GetCustomAttributes(inherit: true).OfType<ITestMethodLifecycle>()) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,31 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
using Xunit; | ||
|
||
namespace Microsoft.AspNetCore.Testing | ||
{ | ||
[Retry] | ||
public class RetryTest | ||
{ | ||
private static int _retryFailsUntil3 = 0; | ||
|
||
[Fact] | ||
public void RetryFailsUntil3() | ||
{ | ||
_retryFailsUntil3++; | ||
if (_retryFailsUntil3 != 2) throw new Exception("NOOOOOOOO"); | ||
} | ||
|
||
private static int _canOverrideRetries = 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: Move to the top of the class There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I departed from the usual pattern here because these are doing a bad thing in that each test has its own static int, but I felt that was cleaner than adding some kind of internal context that we can look at since this is just test code. So the grouping is really a static field per test, it would be really bad if the tests mucked with another one of these fields. |
||
|
||
[Fact] | ||
[Retry(5)] | ||
public void CanOverrideRetries() | ||
{ | ||
_canOverrideRetries++; | ||
if (_canOverrideRetries != 5) throw new Exception("NOOOOOOOO"); | ||
} | ||
} | ||
} |
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.
Isn't
TemplateOutputDir
already very unique❔ IfPath.GetRandomFileName()
isn't unique enough useGuid.NewGuid()
inProjectFactoryFixture.GetOrCreateProject(...)
. If instead this is new needed because the retries reuse existing projects, we may want to completely remove that aspect of the fixture i.e. switch toProjectFactoryFixture.CreateProject(...)
.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 can file a separate issue to track changing that if you want, but I don't really want to do additional major surgery (prefer making this PR just about adding/using retries)
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 additional complications about switching from reusing projects since our longer term hope was to actually switch to one test creating/building the template project, and a second (ordered) test would be in charge of running/verifying the tests, we were hoping that would also reduce the flakiness, or at least help us isolate which part of the test is flaky. Unfortunately we didn't get to that point yet, but that was the longer term goal