Add Current Attempt indicator in TestContext for use with RetryAttribute

[bicarbon8]

the RetryAttribute https://github.com/nunit/nunit/blob/638b4011b102e506a42f65baa515abb010710a2f/src/NUnitFramework/framework/Attributes/RetryAttribute.cs currently uses an internal count variable within its Execute method to track the remaining number of Attempts that can / should be made before giving up (in the case of a constantly failing test).

I would like to request that this instead be a currentAttempt variable that is added to the TestContext so I can read the value from within my test. This would allow me to indicate in my test logs which attempt I am on thereby making debugging via logs an easier process. Additionally, I could use the field to conditionally try additional steps if the number of failures exceeds a certain threshold thereby allowing for additional mechanisms for ensuring robustness of tests

[CharliePoole]

I see you've done a PR. It's usually a good idea to wait for the issue to be accepted first, in order to avoid wasting your time. It's possible that the team may not want to make this change or may want to make it in a different way from what you propose.

Personally, I like this idea, although I can see some dangers in it. If tests have the retry count available, they may end up written to do something entirely different on each attempt. While that may be considered a feature, I'd call it an anti-pattern. Nevertheless, the benefit of being able to log the count probably outweighs the dangers.

I made this a low-priority feature. If any @nunit/framework-team member disagrees we should discuss.

That said, I note that your PR does not implement this as proposed here. Rather than make the count a property of the context, you made it a property of the test. I'll comment on the implementation in #2648.

[jnm2]

Rather than exposing the value programmatically, what about automatically logging when retries (and repeats) happen?
Either way I'm okay with the idea.

[CharliePoole]

@jnm2 Could you expand on that idea? We don't really log anything else automatically, so it's a bit unclear to me.

[jnm2]

We could start automatically adding lines to the log such as "Try 1 of 3 [RetryAttribute]" or we could prefix every line "[retry 2]" though that doesn't scale as well. That solves the logging problem without making the antipattern easy.

[CharliePoole]

You mean the internal trace log?

[jnm2]

I was thinking TestContext.WriteLine. What if RetryAttribute defaulted to doing TestContext.WriteLine("Try 2 of 3 [RetryAttribute]") before each retry?

[bicarbon8]

@jnm2 , while the additional logging you've suggested wouldn't be a bad thing, our tests use custom log files (one for each test method) and so the addition of logging to the TestContext output buffer wouldn't solve the case we need whereby I'd like to have each retry / attempt log to it's own file. To do so I need to know which retry / attempt we are on from within the TestContext

[jnm2]

Cool, making sure we need it. Sounds good to expose the proposed property for the sake of custom logging.

[CharliePoole]

Ah, question of language. The trace log is the only thing we have called a log in the past... 😄

Automatic writing to console is not something we have ever done in the framework. It's something we reserve for users to decide about. If you think about it, we would have to decide whether to use stdout, stderr or the custom "progress" output supported by TestContext. We would either be removing that choice (and the choice of whether to write at all) from the user or would have to develop a set of options to control how and when we did it.

That's why we have up to now merely provided a set of events, which each runner can interpret as they like - possibly according to user settings. IMO, that's what we would have to to here: expose a retry event to the outside world and let runners decide how to handle it. However, it seems like a bit of trouble and I'm not sure whether retrying should be an external event at the same level as starting and ending a test. Open to discussion as an issue, of course.

[bicarbon8]

closing issue as this will be included in the next release to include #2648 which was merged