Enable setting IgnoreUntilDate in TestCaseData.Ignore

[abborg]

Currently, there is no easy method to set an IgnoreUntilDate property on a test case when using TestCaseData. Similar to the optional Until property on the IgnoreAttribute, an overload of the Ignore(string) method can be added to that takes in a reason string as well as an until date as a string. The signature would be something along the lines of:

public TestCaseData Ignore(string reason, string until);

Here's an example usage

private static IEnumerable<TestCaseData> _cases =  new [] { new TestCaseData("test").Ignore("Skip this test", until: "2019-06-30") };

[mikkelbu]

I don't see any problem with adding this functionality to TestCaseData. Current workaround is that the client code that creates the TestCaseData implements the same logic as in IgnoreAttribute.ApplyToTest.

[CharliePoole]

Sorry not to have brought this up sooner, but I was on the road. Let's say we go with this API. In that case, a user can ignore a test case until a certain date...

using IgnoreAttribute on a simple test

[Test, Ignore("Some reason", Until="1942-10-12")]

using TestCaseSourceAttribute

static TestCaseData[] MyData {
    new TestCaseData(arg1, arg2).Ignore("Some Reason", "1942-10-12")
};

[TestCaseSource("MyData")]

using TestCaseAttribute

[TestCase(arg1, arg2, Ignore="Some reason")] // No way to specify until

So, I think that if we implement Until for TestCaseSource we should also do it for TestCase, either now or in the near future. For consistency, we should pick a similar approach in all three.

The proposed method for TestCaseData introduces a new form, which is different from the existing IgnoreAttribute and not extensible to TestCaseAttribute. On attributes, specifying single properties is the best we can do.

If we did this for TestCaseAttribute, we would have to use a named property like

[TestCase(arg1, arg2, Ignore="Some reason", Until="1942-10-12")]

Therefore, I'd like to make the TestCaseData form look as close to that as possible. For example,

static TestCaseData[] MyData {
    new TestCaseData(arg1, arg2).Ignore("Some Reason").Until("1942-10-12")
};

[TestCaseSource("MyData")]

NOTE: for both TestCaseAttribute and TestCaseData we need to avoid the situation where Until is specified without Ignore. I imagine that's why @abborg chose the approach he used. But for TestCaseData it's easy enough to create a derived class that only accepts the Until property. For TestCaseAttribute, we would be forced to rely on runtime checking.

@abborg @mikkelbu What do you think?

[abborg]

I agree that there can improvements to the API. Considering the suggestion made by @rprouse on the pull request and your comment here, I think a fluent API makes more sense than the current implementation. My concept for this API would be something along these lines - borrowing from your example:

static TestCaseData[] MyData {
    new TestCaseData(arg1, arg2).Ignore("Some Reason").Until(new DateTime(1942, 10, 12))
};

[TestCaseSource("MyData")]

As for adding the API to the TestCaseData in a way that allows for compile-time checks, I like the approach of creating a derived class that exposes that method and returns back a TestCaseData. For this use case, I think it would be best to expose a single overload as to avoid confusion and keep the API simple. Something along these lines:

// TestCaseData
...
public IgnoredTestCaseData Ignore(string reason);
...

// IgnoredTestCaseData
public IgnoredTestCaseData : TestCaseData
{
    public TestCaseData Until(DateTime date);
}

I'm interested in hearing other suggestions and retooling my implementation.

[oznetmaster]

As NUnit is updated to current standards, it would seem that DateTime should be updated to DateTimeOffset wherever possible.

DateTime is considered obsolete, and is fraught with issues.

https://blogs.msdn.microsoft.com/davidrickard/2012/04/06/datetime-and-datetimeoffset-in-net-good-practices-and-common-pitfalls/

Maybe there should even be a PR to update all of NUnit to DateTimeOffset?

[CharliePoole]

@abborg Your approach sounds great. I think we have often done this by using a nested public class to avoid cluttering the namespace. It can be made non-browsable soit doesn't show up in intellisense.

[CharliePoole]

@oznetmaster We can update our internals, of course, but it's not up to us to update users! We would have to maintain both options in public APIs.

[oznetmaster]

A desirable goal is to get people to convert to DateTimeOffset, since it remove the ambiguity, and issues, of DateTime.

Since there is an implicit conversion from DateTime to DateTimeOffset, it should be fully compatible to use DateTimeOffset as parameters in new API's, and even to update old API's to use it in their parameters.

API's which return DateTime values are more difficult, but I cannot think of any offhand? Are there any?

[CharliePoole]

I agree that it's a desirable goal for most projects, but since they are not my projects, it's really not my business. If we were Microsoft, we might have some standing to tell people how to use .NET and C#, but we aren't, and I don't think this is a role we want to get into.

Of course, if there is a non-breaking way to change our APIs, we can do that. Maybe an issue along these lines should be filed.

[oznetmaster]

That was all that I was suggesting. It appears that DateTimeOffset can be used in a non-breaking way to update our API's.

I was also asking if it warranted an issue/PR to do so.

It would certainly seem that any new API should use DateTimeOffset exclusively rather then DateTime.

[rprouse]

@abborg I like your proposal. It fits nicely with our current fluent syntax.

@oznetmaster I actually originally wrote DateTimeOffset in my comments but changed it to DateTime to match the rest of our APIs. I agree strongly with your sentiment, but think that it is beyond the scope of this one PR. If we are going to do the switch, I would rather do it separately as one issue/PR.

[rprouse]

That said, since there is an explicit conversion for DateTime -> DateTimeOffset, I think it would be okay to use it in this PR.

[oznetmaster]

And implicit as well.

[jnm2]

Looks good to me. Is there a way to signal to Andrew that we've reached a consensus and are ready for him to go ahead?

[CharliePoole]

@abborg are you following this?

[abborg]

Yes, I've been following this. I'll begin implementing based on the earlier discussion. It may take me a bit of time to finish implementing the changes as I'm busy with other work for the next week, but I should hopefully have something for review within the next two weeks.