CombinatorialMemberData not being newed up for each test

[pellet]

I used the beta version of xunit.combinatorial to use the CombinatorialMemberData feature but unfortunately the data instances were being shared between tests and causing my mocks call verification to fail due to the calls being summed up over all the tests in the theory. I managed to get around the issue temporarily by doing something like:

[Theory, CombinatorialData]
public void CombinatorialMemberDataFromProperties(
    [CombinatorialMemberData(nameof(MockCombinations))] Func<IMock> createMock,
    bool boolParam
)
{
    var mock = createMock();
    Assert.NotNull(mock);
}
public static IEnumerable<Func<IMock?>> MockCombinations =>
  new [] 
  {
    () => new MockA(),
    () => null as IMock
  }

Any help will be appreciated, btw thanks for the great job in creating this library... well done 👍

[AArnott]

I'm not sure what you mean. We don't cache the values obtained from the member providing the data. If you're getting the same values to multiple tests, that's unexpected though because the test discovery process gets the data from xunit.combinatorial for each test individually. Further, just to test I defined two tests that pull on the same member, which generates fresh random value each time. We can see in Test Explorer that indeed the values the test ran with were all unique.

[pellet]

I've created some tests to demonstrate the issue I experienced, from my understanding all the tests should pass but one test does not. I hope this at least clears up expected behaviour :) Mutable object combinations

[AArnott]

Ok, thank you. That's very helpful. The unique thing about your case is that you have one test and multiple parameters. We generate data for each parameter individually, and then combine them in various ways to produce test cases. This is why your test cases share arguments.

"Fixing" this is non-trivial. Your property produces n values, from which a given test case only takes one value as an argument. Intuitively we could get that collection once and assign each value out to a test case. Because we're doing combinatorial testing, it turns out that with more than one parameter on the test method, each value will be assigned to multiple test cases. What you're asking for is that each value be assigned to no more than one test case. One naive fix would be to call your property getter once per test case, which would produce a lot of values that we never use. Ideally we would call your property only enough to get one fresh value per test case, but implementing that would be even more complex.

This is likely an issue with PairwiseData as well as the CombinatorialData attributes, since they both pull for data exactly once per parameter and then generate test cases from that data.

[pellet]

Hi @AArnott, if the CombinatorialMemberData property was made non-static would that require it to be called/created every time a test/combination is ran? It would be good to have combinatorial behaving the same way as xunit where it basically deallocates then reallocates the entire fixture after each test run to make it intuitive that there will be no shared state between tests(otherwise the setup/teardown delegates can be used for advanced use cases). The performance should take a backseat to the behaviour but if the performance is unweildly maybe a LazyCombinatorialMemberData could be used which reallocates the property on each combination but needs to run a lambda to create the value required by the test, therefore not creating values/objects which aren't used in the particular combination ran at the time. Hope these ideas help :) Happy to clarify :)

[AArnott]

No, a non-static field wouldn't make this automatically work. When you say it should work like xunit does, xunit has no parallel to what we're doing, except its MemberData attribute, which does what we do: calls the property once and then creates each test case. But the difference there is that it's the invoked member's responsibility and opportunity to create unique values because it generates a whole set of arguments for every test case. xunit.combinatorial doesn't put that responsibility on you. You just create values and it combines it in various ways.

Don't get me wrong, I'm not arguing the value-prop, as I agree with you. I'm just saying it'll be complex. If you'd like to take a crack at fixing it, and you have tests in place to verify it, and you can consistently fix both PairwiseData and CombinatorialData, I'd very likely merge your PR.