@dotnet-policy-service agree

This PR is for the main branch, which is the v3 source code. I'm happy to merge that into there.

However, I cannot back-port the change to the v2 branch. Using tuples in v2 isn't possible due to the netstandard1.1 target framework for xunit.core.

(The alternative way to do this, via a polyfill'd CollectionBuilderAttribute, isn't available to v2 either, because it requires support for ReadOnlySpan<T> which is also not in .NET Standard 1.1.)

Ignore the CI failures (that's a separate issue that I'll resolve later).

I think I've discovered an issue with this, which is: by overwriting the GetEnumerator with new, I can no longer use the collection syntax for TheoryDataRow. This works today:

var TheoryData<string, int> =
[
    new TheoryDataRow<string, int>("Hello", 42),
    new TheoryDataRow<string, int>("World", 2112),
];

It complains about not being able to convert TheoryDataRow<string, int> into (string, int). I verified that doing an explicit implementation of GetEnumerator is not sufficient for the compiler, because it complains about the opposite problem (cannot convert (string, int) into TheoryDataRow<string, int>).

I'm going to run a couple experiments to see if there's an easy fix. First will be to see if an implicit conversion on TheoryDataRow will make everybody happy. If not, then I'll experiment to see whether using CollectionBuilderAttribute would allow this to work.

So, here's where I landed.

After playing around with implicit conversions, I decided that the most type-safe way to go about this is to leave TheoryData<> accepting TheoryDataRow<> as the primary data mechanism, and use implicit conversions from tuples to TheoryDataRow<>. This let me keep things much more compile-time type safe in the end, as well as simplifying the implementation of the 10 TheoryData<> classes because I could hoist the type-specific stuff Add and AddRange methods up into the base class.

The compiler error message when you have the wrong types does talk about the implicit conversion, but the types being there should let you know exactly what the problem is:

This conversion message would be present regardless of which direction of implicit conversion is involved.

Thanks!

Great!

Should I see if the implementation for a single argument can be added to 2.x branch? It would allow to fix IDE030* ("Use collection expression for ...") warnings. I see it's a breaking change as it changes foreach semantics and it may require changes to MemberDataAttributeBase or MemberDataAttribute.

So far I only managed to get it to work by replacing .Cast().Select() in MemberDataAttributeBase with enumerating dataItems as IEnumerable.

IEnumerable dataItems = ...;
...

// Current version: return dataItems.Cast<object>().Select(item => ConvertDataItem(testMethod, item));

foreach(var item in dataItems)
{
    yield return ConvertDataItem(testMethod, item);
}

Actually, that's an interesting edge case that's always been there for any type which multiply implements IEnumerable, and the answer is testing for IEnumerable<object[]> explicitly first, and using that when it is, and then only fall back to IEnumerable with the Cast<object>() when necessary:

var obj = accessor();
if (obj == null)
    return null;
if (obj is IEnumerable<object[]> arrayEnumerable)
    return arrayEnumerable;
if (obj is not IEnumerable enumerable)
    throw new ArgumentException(string.Format(CultureInfo.CurrentCulture, "Property {0} on {1} did not return IEnumerable", MemberName, type.FullName));

return enumerable.Cast<object>().Select(item => ConvertDataItem(testMethod, item));

(Also interesting to note that the member data error messages always say "Property" regardless of the type of member that's being used.)

Anyways, it's a simple change, so I just pushed it: 4850a0d

This is available now in v2 2.9.1-pre.4 https://xunit.net/docs/using-ci-builds

Thanks!