question: If I'm reading this correctly, would this be handled correctly? I think I would expect expectedArgs to contain an array of [1, 2, 3] and that extra would have nothing

private static IEnumerable<Array> Testcases
    {
        get
        {
            yield return new[] { 1, 2, 3 };
        }
    }

public void MethodWhichReceivesArrayAndHasParams(Array expectedArgs, params int[] extra)

Yeah, in this case it looks wrong. (Funny how things become clearer when it gets a name :-)) .
I didn't change this logic though -- , but this is meant to handle signatures like:

public void MethodWhichReceivesIntAndHasParams(ìnt value, params int[] extra).

The signature with Array and params doesn't make sense, as you say, nothing will get into the extra.

Also, public void MethodWhichReceivesIntAndHasArray(ìnt value, Array extra) fails.

I added two tests to the repro for this case, the int case worked as it should, got one value, and the rest to the extra, but the Array + params crashed. It should have been caught in a check earlier imho. Or, we could handle it as a special case - if the earlier parameters contain a collection we simply ignore the last collection.

I believe we have to set up some "rules" on how this can be used, so that we don't have to cover all possible constructions here.

Thanks @OsirisTerje It's good to hear that the logic was just moved and not changed.

Indeed, these have been hard to fully and properly test in the past. I can't speak to the original rules, though the general guideline we've taken in the past has been to keep it simple and to just try to ensure that we maintain consistent behaviour as if the args from the sources were passed to the test method directly via a regular invocation.

Though that of course is anything but simple :)

Ex:

private static IEnumerable<Array> Testcases
    {
        get
        {
            yield return new[] { 1, 2, 3 };
        }
    }

    [TestCaseSource(nameof(Testcases))]
    public void Foo(Array values)
    {
        Assert.That(values.Length, Is.EqualTo(3));
    }

    [TestCaseSource(nameof(Testcases))]
    public void Foo2(Array values, params int[] extra)
    {
        Assert.That(values.Length, Is.EqualTo(3));
    }

  // Should be marshaled the same as:
  Foo(Testcases.First())
  Foo2(Testcases.First())

I haven't tried yet to see how those actually behave

The test public void Foo2(Array values, params int[] extra) will crash. So you mean we should fix this, even if the code is meaningless ?

The code isn't meaningless if the test data would be something with a nested array:
new object[] { new int[] { 1, 2 }, 3, 4 }

question: I'm a little unclear about this one. I notice a comment in the previous implementation:

// If array has the same number of elements as parameters
// and it does not fit exactly into single existing parameter
// we believe that this array contains arguments

Would this short-circuit that check it's checked below this?

Not sure about what you mean here - believe I need some more context.

I read it as when I have an array (which is a prerequisite for this, and covers all collections), and I have more than one parameter we assume that is a set of individual parameters. That has to match exactly to the number of elements in the array. (If it doesn't, we get internal crashes on that too, so also something we should give a better error message on.)
I've debugged through the code, and it seems to me it works.

But I understand you see something here, so please elaborate :-)

PS: We don't really have a good spec of what TestCaseSource can do. The documentation we have show a few of the ways to use it, but everything regarding collections are absent. Perhaps we should do something about that.

I think I had a hard time mapping the logic in your new helper to the original code, at least when I was looking at it last night. I was concerned that a case like this, where argsNeeded is greater than 1 but not equal to the number of arguments could still cause the array to be unpacked even though the number of arguments in the test method differs from the number of elements in the array.

private static IEnumerable<Array> Testcases
{
    get
    {
        yield return new[] { 1, 2, 3 };
    }
}

[TestCaseSource(nameof(Testcases))]
public void Foo(int a, int b)
{
}

I admit the same disclaimer as my other comment. I haven't tried running the code yet to see how these behave.

PS: We don't really have a good spec of what TestCaseSource can do. The documentation we have show a few of the ways to use it, but everything regarding collections are absent. Perhaps we should do something about that.

This is a very good point. I have an issue for myself to add examples for optional, params, etc, but even after that I suspect there would still be gaps

Your example here will also crash. In these cases there has to be an exact match in number of elements and number of parameters. And the crashes are inside our code, so we 1) have to fix this - but then we have to be clear how we should handle overflow and underflow 2) we catch these earlier and provide good error message for it

Did this actually work earlier ?

I made a new repro, moved back to version 3.13, and noted:

For public void Foo(int a, int b)

1. Too few parameters or too many parameters: Fails with message: Not enough arguments provided, provide at least 3 arguments.

For public void Foo(int value, params int[] extra)

2. Fails in 3.13 with Not enough arguments provided, provide at least 2 arguments.

This one works directly in 5129 fix.

About 1: We should fix this to be equal to 3.13. I might have missed something in the latest refactoring here. I'll check this tomorrow, if we agree we should be equal to 3.13.

4.4.0 behaves just as 3.13

The image below show the results for all 3 versions we're talking about now:

Interesting to see that 4.5.0. and 5129 fix overflow/underflow is not even properly identified.

So with the fix, we only need to fix the overflow/underflow, and then at least make it back to 3.13-4.40 level.

One could argue that an overflow, we could ignore the overflow arguments, and for underflow we could assume the parameter overflow values have their default value. But for now, I think fixing the error messages is the way we go.

The overflow/underflow is not really errors either, they are failures in the test setup, so it should perhaps be Inconclusive ?

Thanks for catching that!
And FWIW I would agree that any regressions from previous behaviour should be fixed. Bugs related to new features within 4.5 (such as anything specific to method signatures with params or optional args) should also be fixed but separately as I would suggest they are lower priority. They would also likely have workarounds in that a Test/Value/Fixture source could just be updated to pass those args explicitly where relevant.

ShouldUnpackArrayAsArguments currently treats any array whose runtime type is assignable to the first parameter type as a single argument when the method has exactly one parameter plus a params array. That breaks the common TestCaseSource pattern where an object[] represents the argument list, e.g. for void M(Array first, params int[] rest) with source new object[] { new int[]{1,2}, 3, 4 } the method would not unpack and first would receive the whole object[]. The previous logic always unpacked when a params parameter exists. Consider excluding object[] from the IsAssignableFrom short-circuit (treat object[] as the argument-container by default unless explicitly wrapped) so that object[] continues to represent multiple arguments for multi-parameter methods.