Bug: Empty params inline data

[weifenluo]

The following test will success when targeting net461, whereas failed targeting net5.0 with error message System.InvalidOperationException : The test method expected 1 parameter value, but 0 parameter values were provided.:

public class UnitTest1
{
    [Theory]
    // This does not work either
    // [InlineData]
    [InlineData(new object[] {})]
    public void Test1(params string[] values)
    {
        Assert.Empty(values);
    }
}

Repo: https://github.com/weifenluo/XUnitEmptyParamsBug

[bartelink]

What are you expecting to happen? What happens if you provide an empty array as arguments to the InlineData attribute as one normally would?

[weifenluo]

What are you expecting to happen? What happens if you provide an empty array as arguments to the InlineData attribute as one normally would?

I'm expecting the test successfully passed both targeting net461 and net5.0.

[bartelink]

Right, what I was nudging at is that I suspect that it will work fine if you actually pass (an empty array of) arguments in the InlineData like one normally would

Whether it's xUnit or .NET that's issuing the exception, I think its useful that it rejects that case. But even then - InlineData with no arguments does not really make a lot of sense.

[weifenluo]

Do you mean change the code to the following:

  public class UnitTest1
  {
      [Theory]
      [InlineData(new object[] {})]
      public void Test1(params string[] values)
      {
          Assert.Empty(values);
      }
  }

Same result:

It appears when targeting net5.0, the provided arguments are ignored.

[weifenluo]

Plus: when test method has parameter with params keyword, InlineData with no arguments should be considered as empty array - IMHO it makes a lot of sense because that's how C# does.

[bartelink]

I agree that it is inconsistent behavior, and its good that you have provided a repro. As for whether its possible or important to fix, that's what I'm more pushing back on as a courtesy to the maintainers - I'd like to see their precious time they donate to us be invested in V3 timeframe stuff for .NET Core only.

I'm not sure what the root cause is - whether its down to Reflection impl changes in .NET, the C# compiler or something else.

Its possible that something can be done in InlineDataAttribute or in the Theory impl, but I personally would see this as a low priority issue as:

1. it fails unambigiously on the .NET that is going to be uses IRL
2. passing no arguments to a Theory is a pretty moot case - why would one ever do that?

[weifenluo]

1. When the test method has parameter with params keyword, [InlineData] is actually a simpler version of [InlineData(new object[] {}], we're passing an empty array, not passing no arguments.
2. Testing against an empty array of data is very basic when dealing with collection of data. I don't agree it is a pretty moot case.

I really appreciate works that maintainers has devoted to this project. However, if I'm care taker of the project, I would be curious to find out the reason for the inconsistent behavior, at least.

[bartelink]

Testing against an empty array of data is very basic when dealing with collection of data. I don't agree it is a pretty moot case.

Hm, I missed the point that there does not seem to be a syntax that works to pass an empty array - I was more quibbling about whether [InlineData] should be expected to map to an empty string array - I'd prefer for that construct to be outright rejeted.

So yes, no harm to log it on that basis (but the most helpful thing of all would be a PR - its not only on the mainainer ;P)

[bradwilson]

The problem appears to be with theory data pre-enumeration. I'm not sure whether it's in the core framework or the VSTest adapter.

As a workaround, you can follow the documentation instructions for adding a configuration file and set preEnumerateTheories to false:

{
  "$schema": "https://xunit.net/schema/current/xunit.runner.schema.json",
  "preEnumerateTheories": false
}

[weifenluo]

It appears there is a downside of setting preEnumerateTheories to false: the theory test will be displayed as one single item in Test Explorer window, instead of breaking down into separate inline data items.

I end up with putting #if NET461 for the empty InlineData which causes the problem targeting net5.0 only.

[bartelink]

Rather than disabling the test case, the workaround I'd suggest is to use MemberData to yield return each of the InlineData values you have; this should allow you to pass the empty array?

[bradwilson]

Rather than disabling the test case, the workaround I'd suggest is to use MemberData to yield return each of the InlineData values you have; this should allow you to pass the empty array?

No, that won't help. The bug isn't in data discovery, it's in serialization.

It appears there is a downside of setting preEnumerateTheories to false: the theory test will be displayed as one single item in Test Explorer window, instead of breaking down into separate inline data items.

Using MemberData instead of InlineData would allow you to selectively disable pre-enumeration just for that one test (since MemberData has a property named DisableDiscoveryEnumeration. That would then limit the impact to Test Explorer to just this one test.

xunit/src/xunit.core/MemberDataAttributeBase.cs

Lines 30 to 35 in b88fee5

	/// <summary>
	/// Returns <c>true</c> if the data attribute wants to skip enumerating data during discovery.
	/// This will cause the theory to yield a single test case for all data, and the data discovery
	/// will be during test execution instead of discovery.
	/// </summary>
	public bool DisableDiscoveryEnumeration { get; set; }

[weifenluo]

If pre-enumeration is disabled, I would rather prefer to write traditional [Fact] test, which is strongly typed and intuitive:

[Fact]
public void TestMethod()
{
    VerifyMethod(...);  // Equals to one InlineData/ClassData/MemberData
    VerifyMethod(...);  // Equals to another InlineData/ClassData/MemberData
}

private void VerifyMethod(...)
{
    ...
}

IMO the biggest advantage of InlineData, is that it will be displayed separately in Test Explorer window, handy for debug and with more reported test cases. Personally I don't like ClassData/MemberData, I feel it makes the test code unnecessarily complicated and less readable - test code should be kept as simple as possible. I would rather write multiple [Fact] tests:

[Fact]
public void TestMethod_Condition1()
{
    VerifyMethod(...);
}

[Fact]
public void TestMethod_Condition2()
{
    VerifyMethod(...);
}

private void VerifyMethod(...)
{
    ...
}

[bartelink]

I agree in general, but it can work given the right conditions

[cdibbs]

Similar story with me: I came back to a project in a newly-installed copy of VS2022 only to find several, params-array InlineData tests had broken. I'm not sure when they broke, but I don't have much time to devote to this so, in case it helps anyone, I worked around it using [InlineData(default)] and myargs ??= Array.Empty<string>(); at the top of each test.

[dahlbyk]

The bug isn't in data discovery, it's in serialization.

Here's what I see for [InlineData][InlineData("hi")]:

[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.3+1b45f5407b (64-bit Desktop .NET 4.0.30319.42000)
[xUnit.net 00:00:00.05] XUnitEmptyParamsBug: Deserializing 2 test case(s):
  Xunit.Sdk.XunitTestCase, xunit.execution.{Platform}:VGVzdE1ldGhvZD...
  Xunit.Sdk.XunitTestCase, xunit.execution.{Platform}:VGVzdE1ldGhvZD...
[xUnit.net 00:00:00.00] xUnit.net VSTest Adapter v2.4.3+1b45f5407b (64-bit .NET 5.0.17)
[xUnit.net 00:00:00.19] XUnitEmptyParamsBug: Deserializing 2 test case(s):
  :F:XUnitEmptyParamsBug.UnitTest1:Test1:1:0:fcabad1bf8134136a357421b754df74f
  Xunit.Sdk.XunitTestCase, xunit.execution.{Platform}:VGVzdE1ldGhvZD...

So serializing the empty InlineData on .NET Core+ runs through here:

xunit/src/xunit.execution/Sdk/Frameworks/XunitTestFrameworkDiscoverer.cs

Lines 166 to 167 in 93616f7

	if (className != null && methodName != null && (xunitTestCase.TestMethodArguments == null || xunitTestCase.TestMethodArguments.Length == 0))
	return $":F:{className.Replace(":", "::")}:{methodName.Replace(":", "::")}:{(int)xunitTestCase.DefaultMethodDisplay}:{(int)xunitTestCase.DefaultMethodDisplayOptions}:{testCase.TestMethod.TestClass.TestCollection.UniqueID.ToString("N")}";

It seems like the bug may be inconsistency in populating XunitTestCase.TestMethodArguments?

[bradwilson]

This bug was likely introduced when support for parameter defaults was added. When the code was written, "no parameters" meant "this is a Fact, not a Theory" but this is not strictly speaking true any more.

[bradwilson]

(Another way to say this is: originally, [InlineData] was a broken declaration, because there was no such thing as a parameter-less Theory.)

[dahlbyk]

I'm wondering if dropping xunitTestCase.TestMethodArguments.Length == 0 might be sufficient. But at a loss as to why Framework and Core+ would have different behavior, and not sure where to write a failing test for Core:

xunit/test/test.xunit.execution/Sdk/Frameworks/TheoryDiscovererTests.cs

Line 1 in 93616f7

#if NETFRAMEWORK

[bradwilson]

That's because AcceptanceTestV2 on the v2 branch only works on .NET Framework, because it depends on loading things into a separate app domain so they don't get locked.

https://github.com/xunit/xunit/blob/93616f73fc2c5a1dca11963a41d52ce9e51f1bb3/test/test.utility/AcceptanceTestV2.cs