Make IgnoreAttribute.Reason public

[RenderMichael]

fixes #4777

test.Properties.Set will throw on a null reason:

https://github.com/nunit/nunit/blob/main/src/NUnitFramework/framework/Internal/PropertyBag.cs#L51

So I'm adding a null-check up-front. If that's not necessary or desirable I can remove it.

[RenderMichael]

I was worried the stack trace might suffer from the earlier null check, but in my opinion it's been improved :)

Before:

After:

[manfred-brands]

Thanks @RenderMichael
I think throwing an exception at the earliest opportunity is normally an improvement, but I'm not sure here.

There is a slight chance of an unexpected exception when a user did:
[Ignore(null!, Until = "2025-01-06")]

The other issues is throwing an exception in an Attribute constructor.
We had issues in the past where this stopped test discovery, instead of failing the tests itself.

[RenderMichael]

That reasoning makes sense. My initial motivation was to have argument validation when I'm scanning through [Ignore]-ed methods without executing them.

I am in fact noticing a problem with the assembly scanning when adding [Ignore(null!)] on a test with a [TestCaseSource(Type, string)] attribute:

This is happening on build, even before any test has been run. I'm not sure if this is a bug that's logged somewhere.

However, this is still happening without my changes. I will remove the argument validation either way, to minimize any possible issues.

[RenderMichael]

There is a slight chance of an unexpected exception when a user did:
[Ignore(null!, Until = "2025-01-06")]

Doesn't this mean that reason can be null? There's even special handling for a null reason, and [Ignore(null, Until = ...)] is the only way to reach it:

Can we make reason nullable then?

[manfred-brands]

Can we make reason nullable then?

I don't think it should be nullable, but don't know the reasoning behind the null handling.
Hey, I didn't even new there was an Ignore Until.
Maybe someone else from the @nunit/framework-team can remember.

Note that even if we declare it as non-nullable, if the consuming project is not nullable aware, they can specify [Ignore(null)] without getting a compile warning.

[RenderMichael]

A null reason seems like an error in some cases, but valid in others.

I propose changing the IgnoreAttribute constructor to do Reason = reason ?? "", and change the check in AddIgnoreUntilReason to string.IsNullOrEmpty to retain existing behavior of avoiding a whitespace at the end.

This will change existing code which does [Ignore("", Until = "2025-01-06")] to avoid a trailing whitespace.

[OsirisTerje]

I believe that will trigger a roslyn warning. Which then has to be suppressed.
IMHO, the task was to expose reason as a property. I dont think there is a need to change behaviour, even if well intended.

PS. Not at pc right now

[OsirisTerje]

@CharliePoole Do you remember the Until option and code around that? Never used it myself, not even aware of it, and not seen it used either.

[RenderMichael]

the task was to expose reason as a property. I dont think there is a need to change behaviour, even if well intended.

In that case the PR is ready for review.

[CharliePoole]

@OsirisTerje I think I was involved when it was first added but lots has changed with the introduction of nullables.

Basically, however, the intention of Ignore was to always require a reason. We got some pushback on that from users who wanted an easy way to skip a test without leaving a note behind, but we resisted it. I think the loophole that @manfred-brands points out was probably unintended, but it's possible someone thought that the existence of a date served as a reason... i.e. that something would be fixed by that date. IMO, that "something" should be given as the reason.

This, btw, is why Ignore is treated as a type of warning as opposed to tests we always want to skip, e.g. on a particular platform. A warning without any text isn't much of a warning!

[OsirisTerje]

Lgtm