Add support for `ref bool`, `ref bool?`, `in bool`, and `in bool?` when using `NUnit.Framework.ValuesAttribute`

[lsolano]

The following type of tests are not executed because they do not produce any data for [Values] attribute when parameters are in bool, in bool?, ref bool, or ref bool?.

public void SomeParameterizedTest([Values] in bool someFlag)
{
  // Test case code here ...
}

The test is not reported as failure, or ignored but it is silently not executed.

If the described behavior is not a design decision I would like to add support for the four scenarios (already implemented on my fork at commit d6f38fadc1c790695720bddde233c4b0fcdb02f4).

As a side note, it will be very helpful to have some kind of warning about those situations.

[mikkelbu]

I'm not against this change, but can you provide some use case for why a user would use in or ref in a test. I'm a bit worried that the ref could confuse a user.

If the described behavior is not a design decision I would like to add support for the four scenarios (already implemented on my fork at commit d6f38fa).

I don't think this is a design decision, but rather that the NUnit code is older than these concepts (or perhaps that nobody ever considered this case). I think the implementation should be more specific, so we only capture in and ref - using GetElementType() we will also capture other types like a bool[].

As a side note, it will be very helpful to have some kind of warning about those situations.

I don't think this is possible (I'll have to examine this in more detail). The problem is that in these situations we don't create a test as there is no data for the test, so there is no place to associate the error.

[jnm2]

It's also worth mentioning that in bool is a deoptimization. in only makes sense for value types that are a few times larger than a ref (pointer) themselves. bool is one byte; in bool and ref bool are 8 bytes plus possibly an additional allocation on the caller side.

[lsolano]

ref is confusing: agree

@mikkelbu I'm have no real case for ref, as you I think it can be confusing as we the framework is not expecting changes to parameters values. I DO use in a lot, not only with bools but with larger types (such as strings).

De-optimization: I'm not only looking for optimizations but also design expressiveness (read-only parameters)

In my production, and test code, I do use in to signal that I won't touch the values on my parameters. Is a design decision that now I can impose with help from the compiler. The fact that at a lower level in parameters are implemented as references and that for smaller types we can see a detriment in performance (as @jnm2 said) , due to pointers size, is an implementation issue that can be noticed to framework users and let then decide when to apply the in modifier.

Consistency

Also I want to point out a consistency issue with the Values attribute. It already works for other types, when sending hard-coded values. But when using the parameter-less constructor and in bool it just don't, with the impossibility to issue a warning (as explained by @mikkelbu ). Here are some examples.

Vanilla (original test method)

[Test]
public void Equals_With_Semantically_Equals_Names_Returns_True(
    [Values("a.b", "a.B", "A.b", "A.B")] string rawNameA,
    [Values("a.b", "a.B", "A.b", "A.B")] string rawNameB,
    [Values] bool requiredA,
    [Values] bool requiredB)
{
    // test case code here ...
}

Works with in string

[Test]
public void Equals_With_Semantically_Equals_Names_Returns_True(
    [Values("a.b", "a.B", "A.b", "A.B")] in string rawNameA,
    [Values("a.b", "a.B", "A.b", "A.B")] in string rawNameB,
    [Values] bool requiredA,
    [Values] bool requiredB)
{
    // test case code here ...
}

Ignored (with in bools)

[Test]
public void Equals_With_Semantically_Equals_Names_Returns_True(
    [Values("a.b", "a.B", "A.b", "A.B")] in string rawNameA,
    [Values("a.b", "a.B", "A.b", "A.B")] in string rawNameB,
    [Values] in bool requiredA,
    [Values] in bool requiredB)
{
    // test case code here ...
}

Tests count:

(Vanilla)   string and bool: Passed:  1118, Duration: 3 s -
(Works)    in string and bool: Passed:  1118, Duration: 2 s -
(Ignored)  in string and in bool: Passed:  1054, Duration: 2 s -

Note how tests count dropped from 1,118 to 1,054.

Summary Proposal

Nice to have (1st option)

• Support for in bool to be able to express intended design for read-only parameters and avoid accidental value changes.
• Also improving consistency of ValuesAttribute among target types.
• Drop ref support for ANY type as it is very confusing.

Second best option (2nd option)

• Throw an error, from ValuesAttribute, instead of not creating the test cases. That way, framework users, are informed and stopped from using the attribute in an untended scenario.
• Improve attribute's XML documentation to signal that breaking change.

Third option (3rd option)

• Improve actual XML documentation to declare when and how the attribute works as intended and when just don't work because it does not produce test cases for edge cases (such as in bool and ref bool).

[jnm2]

@lsolano In terms of design expressiveness, passing by value (bool) is exactly equivalent to passing by readonly ref (in bool) and is an even stronger guarantee. If you pass by value, the method knows for sure that the value of the parameter cannot change unless the method itself changes it. If you pass by reference, the method could get a different value each time it references the readonly ref even though the method can't assign through that ref.

[jnm2]

I'd be in favor either of making this just work or of producing an error for such methods. Introducing a new error seems unlikely to break anyone in practice because it's not likely that anyone has methods marked as test methods that have in/ref parameters, because of how NUnit behaves if you do that. Even if it did break some people (and I still think this is probably zero), we have treated things like this in the past as bug fixes. I.e. it should have been producing an error all along, and we consider it to be a bug that the current versions are missing this error.

[lsolano]

it should have been producing an error all along, and we consider it to be a bug that the current versions are missing this error.

Me too, the current behavior (silently ignored test cases) is worse than having, or not, the support for reference types (normal or read-only ones).

[stevenaw]

Thanks for noticing this.

I agree the current behaviour of silently skipping tests isn't ideal, and I think any solution here should be generalized.
After a few experiments, I am in favour of fixing this and making it work rather than to treat this as an error. I like API consistency, and it appears that only the first of these will run:

[Test]
public void TestTheValues([Values(true, false)] in bool a)
{
    Assert.That(true); // Passes
}

[Test]
public void TestTheValues([Values] in bool a)
{
    Assert.That(true); // Never runs
}

It may just be my mental model, but I think of [Values] without any args as a shorthand for "I am using a type with a closed set of values, and I don't want to have to specify them all explicitly". I would find it unusual if I could not use them interchangeably.

[lsolano]

@stevenaw It's the same for enumerations.

[Dreamescaper]

@lsolano
Would you be willing to submit PR for this functionality?