Introduce RetryAttribute for test methods
#4586
Conversation
| /// <summary> | ||
| /// Gets the number of retries that the test should make in case of failures. | ||
| /// </summary> | ||
| public int MaxRetries { get; } |
| /// <summary> | ||
| /// Initializes a new instance of the <see cref="RetryAttribute"/> class with the given number of max retries. | ||
| /// </summary> | ||
| public RetryAttribute(int maxRetries) |
There was a problem hiding this comment.
I don't want to build something too complex but I think it would be good to have:
- Delay: wait time between 2 retries
- optional: some backoff strategy, I think constant and exponential are the most used.
| /// This attribute is used to set a retry count on a test method in case of failure. | ||
| /// </summary> | ||
| [AttributeUsage(AttributeTargets.Method, Inherited = false, AllowMultiple = false)] | ||
| public sealed class RetryAttribute : Attribute |
There was a problem hiding this comment.
It'd be good to have this attribute non-sealed so we allow extensibility, which means the logic should also be partially here so it can be overrided. I mainly want that so people (or we?) could ship a retry based off Polly.
There was a problem hiding this comment.
I think we have a precedens for that with data sources that don't use non-sealed class, but rather than that an interface (ITestDataSource) on the attribute, so any implementer does not have to provide the same ctor parameters we provided.
| /// Initializes a new instance of the <see cref="RetryAttribute"/> class with the given number of max retries. | ||
| /// </summary> | ||
| public RetryAttribute(int maxRetries) | ||
| => MaxRetries = maxRetries; |
There was a problem hiding this comment.
Shall we validate value here?
| /// <summary> | ||
| /// Gets the number of retries that the test should make in case of failures. | ||
| /// </summary> | ||
| public int MaxRetries { get; } |
There was a problem hiding this comment.
Let's be explicit here this is in addition to the original call so a value of 1 means call + 1 retry, so 2 attempts in total.
There was a problem hiding this comment.
Not strong opinion, but the comment already says "in case of failure", to me it is clear.
| /// </summary> | ||
| /// <returns> | ||
| /// The number of retries, which is always greater than or equal to 1. | ||
| /// If RetryAttribute is not present, returns 1. |
There was a problem hiding this comment.
I guess it's how you interpret language, but in TUnit, lack of a retry attribute means a retry count of 0;
Because if I do [Retry(1)] I'd expect it to try again after a failure, once.
If I'm reading this PR correctly, [Retry(1)] currently means, run the test normally as you would and never retry? So that's retrying 0 times.
Does that make sense? The number doesn't work with the wording. This usage would work better with [Attempt(1)] or [Try(1)] or something.
There was a problem hiding this comment.
@thomhurst Yup indeed. I'll be working on a fix so that [Retry(n)] means it can run 1 + n times at max (normal first attempt + n retries).
There was a problem hiding this comment.
Perfect, NUnit imho does it the way when Retry(1) means normal run, and I only see that being source of confusion.
|
FYI From previous talks with Playwright where they have good retries, that imho were attempted to be duplicated in mstest.playwright, Max also mentioned that they wanted:
|
Not sure what shape would you be expecting. Currently, every application of the attribute is specifying the number of retries
This is currently info specific to the test method itself and is irrelevant to assembly initialize. I think you are talking of a completely different design that allows a "global" retry count, not a Retry attribute on test methods which I'm implementing here.
We are going to have a design that allows that, but that is unlikely to be implemented in this PR though, unfortunately. We need to know better how to communicate that information to tools that will make use of it. |
Fixes #3161