Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CombinatorialMemberData cannot be of type IEnumerable<object[]> #60

Closed
siewers opened this issue Feb 17, 2023 · 18 comments · Fixed by #98
Closed

CombinatorialMemberData cannot be of type IEnumerable<object[]> #60

siewers opened this issue Feb 17, 2023 · 18 comments · Fixed by #98
Assignees

Comments

@siewers
Copy link
Contributor

siewers commented Feb 17, 2023

The CombinatorialMemberDataAttribute requires the member to return an enumerable which is not an IEnumerable<object[]>.
This breaks compatibility with the Xunit MemberData attribute, which requires the member to return exactly an IEnumerable<object[]>.

We should figure out how to support the same member data type as Xunit support, so we can use the same member data for both MemberData and CombinatorialMemberData.

I think I might have broken that in the latest prerelease version.

@siewers
Copy link
Contributor Author

siewers commented Feb 17, 2023

Right now I have to do something like this, which is a bit annoying.

public static IEnumerable<string> GetValues 
{
    get 
    { 
        yield "Foo";
        yield "Bar";
        yield "Baz";
    } 
}

public static IEnumerable<object[]> GetValuesXunit 
{ 
    get { return GetValues.Select(x => new object[] { x }); }
}

[Theory]
[MemberData(nameof(GetValuesXunit))]
public void TestWithMemberData(string value)
{
}

public void TestWithCombinatorialData(
    [CombinatorialMemberData(nameof(GetValues))] string value, 
    [CombinatorialValues(1, 2, 3)] int number)
{
}

@siewers
Copy link
Contributor Author

siewers commented Feb 17, 2023

When I think about it, I'm not sure this is even possible to solve. It also seems like a rather odd constraint in xUnit. It should be able to verify any IEnumerable<T> to match the method signature.

@siewers
Copy link
Contributor Author

siewers commented Feb 17, 2023

Perhaps using TheoryData<T> could fix it, but currently CombinatorialMemberData doesn't understand that.

@AArnott
Copy link
Owner

AArnott commented Mar 7, 2023

I don't see how these two would be compatible anyway. CombinatorialMemberDataAttribute takes values for an individual parameter, whereas MemberDataAttribute is applied to the whole method and therefore takes values for all the parameters. They are fundamentally different data and applied to different places.
What's more, the whole point of CombinatorialMemberDataAttribute is to apply to just one parameter so that the set of test cases can be generated by the library, whereas MemberDataAttribute requires you to generate the test cases yourself.

So I'd say this is By Design.

@AArnott AArnott closed this as not planned Won't fix, can't repro, duplicate, stale Mar 7, 2023
@siewers
Copy link
Contributor Author

siewers commented Mar 7, 2023

What do you think about supporting TheoryData<T>?
That should be doable and would probably support both scenarios 🤔

@AArnott
Copy link
Owner

AArnott commented Mar 7, 2023

I don't know anything about TheoryData<T>. If it's something you're proposing, I'd need more of a spec to comment on it. A new issue would be appropriate for that.

@siewers
Copy link
Contributor Author

siewers commented Mar 7, 2023

Yeah, it's also not very well documented, if at all. I haven't found any official documentation about it, but Andrew Lock has a nice blog post about it.

I'm a bit reluctant whether it should be supported, given the feature itself is so poorly documented.
I guess it wouldn't hurt and my best guess is that the xUnit maintainers simply just don't have a lot of documentation at all.

If you think it's worth supporting, I'd be happy to create a PR for it, I just don't want to waste my time if you think the idea is pointless 🙂

@AArnott
Copy link
Owner

AArnott commented Mar 7, 2023

Cool. But I still need to understand how that would be used and apply to this library. Thank you for volunteering to contribute to the library. I'd be interested to read sample code and a brief explanation of its behavior as it would apply to this library (no need to make it actually work yet).

@silkfire
Copy link

silkfire commented Mar 4, 2024

@AArnott

Supporting 1-dimensional TheoryData (i.e. only TheoryData<T> with one type parameter) would be very convenient.

The conversion from object[][] to IEnumerable<T> goes as follows:

return new MyTheoryData().Select(d => (MyType)d.Single());

@siewers
Copy link
Contributor Author

siewers commented Apr 27, 2024

@silkfire

As part of #95 I'm working on supporting a part of this. If the idea is approved, I intend to support TheoryData (or basically IEnumerable<object[]>) on the CombinatorialMemberDataAttribute as well, which will also get a generic counterpart.

@siewers
Copy link
Contributor Author

siewers commented Apr 27, 2024

It's not technically impossible to support IEnumerable<object[]> since that is intended to map to multiple parameters in a test method. TheoryData, however, maps to a single type, which is exactly what will make tests a lot easier to read, write and maintain. With TheoryData the type can be whatever you want it to be, so it should be flexible enough to support most cases.

The idea is to allow this:

public class Tests
{
    public record MyTestCase(int Number, string Text);

    public static readonly TheoryData<MyTestCase> MyTestCases = new(
        new MyTestCase(1, "Foo"),
        new MyTestCase(2, "Bar")
    );

    [Theory, CombinatorialData]
    public void TestMyTestCases([CombinatorialMemberData(nameof(MyTestCases))] MyTestCase testCase, bool flag)
    {
        /*
            This will give you:
            testCase(1, "Foo"), true
            testCase(1, "Foo"), false
            testCase(2, "Bar"), true
            testCase(2, "Bar"), false
        */
    }
}

@siewers
Copy link
Contributor Author

siewers commented Apr 27, 2024

It can also only support TheoryData<T> (with a single generic parameter, since multiple generic parameters act similarly to IEnumerable<object[]> that maps to multiple method arguments.

@AArnott
Copy link
Owner

AArnott commented Apr 27, 2024

Reactivating due to recent activity. I'll review the proposal and get back to you soon.

@AArnott AArnott reopened this Apr 27, 2024
@siewers
Copy link
Contributor Author

siewers commented May 23, 2024

@AArnott have you had time to review the proposal yet?

@AArnott AArnott self-assigned this Oct 1, 2024
@AArnott
Copy link
Owner

AArnott commented Oct 1, 2024

Reviewing this now. Sorry it took so long.
I still don't understand what value you're adding with your proposal. You most recent code snippet that demonstrates what you want to do is great, but I can accomplish the same with no changes to the library after changing very little in your snippet:

public class Tests
{
    public record MyTestCase(int Number, string Text);

    public static readonly IEnumerable<MyTestCase> MyTestCases = [
        new MyTestCase(1, "Foo"),
        new MyTestCase(2, "Bar")
    ];

    [Theory, CombinatorialData]
    public void TestMyTestCases([CombinatorialMemberData(nameof(MyTestCases))] MyTestCase testCase, bool flag)
    {
        /*
            This will give you:
            testCase(1, "Foo"), true
            testCase(1, "Foo"), false
            testCase(2, "Bar"), true
            testCase(2, "Bar"), false
        */
    }
}

So why then is your version so much better?

@silkfire
Copy link

silkfire commented Oct 2, 2024

Will this add support for TheoryData or how is this different from #95?

@siewers
Copy link
Contributor Author

siewers commented Oct 2, 2024

@AArnott The difference is that using TheoryData<T> allows the same data source to be used as theory data in other tests. This can be very convenient for 1-dimensional theories that should also be used for combinatorial tests.
xUnit does not support typed IEnumerables, but only accepts IEnumerable<object[]>.
Additionally, the later versions of xUnit analyzers even warn about the usage of IEnumerable<object[]> and recommend using TheoryData<T> instead.

Does that make sense? :)

@siewers
Copy link
Contributor Author

siewers commented Oct 2, 2024

@silkfire #95 was more about introducing generic combinatorial attributes. It's not directly related, but my work on that added more functionality which could be very useful (I think).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants