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

Allow Is.AnyOf to be called with arrays or other collections #4355

Closed
manfred-brands opened this issue Apr 19, 2023 · 6 comments · Fixed by #4356
Closed

Allow Is.AnyOf to be called with arrays or other collections #4355

manfred-brands opened this issue Apr 19, 2023 · 6 comments · Fixed by #4356

Comments

@manfred-brands
Copy link
Member

Follow-up to a great docs observation on nunit/docs#750 and nunit/nunit.analyzers#533

The Is.AnyOf only works if the members are specified as individual values, not if passed as an array or list.

Example Gotcha:

[Test]
public void CanBeUsedWithParameters()
{
    Assert.That(42, Is.AnyOf(0, -1, 42, 100)); // Passes
}

[Test]
public void CantBeUsedWithAnArray()
{
    var array = new[] { 0, -1, 42, 100 };
    Assert.That(42, Is.AnyOf(array)); // Fails
}

[Test]
public void CantBeUsedWithAList()
{
    var list = new List<int>() { 0, -1, 42, 100 };
    Assert.That(42, Is.AnyOf(list)); // Fails
}

It was suggested to make an Analyzer to warn about this, but it seems more prudent to fix it in the framework, so that these expected cases work.

@stevenaw
Copy link
Member

stevenaw commented Apr 19, 2023

I'm not positive, but I feel like this may have been by-design to account for an edge case with strings being an ienumerable<char> but I could be mistaken.

I'd have to refresh myself on some of the design discussions in #2704

@manfred-brands
Copy link
Member Author

@stevenaw Yes, that issue was discussed in #2704, with a remark We could add the params now and do IEnumerable on demand later.

I added a nunit test that proofs that the below work.

            Assert.That('A', Is.AnyOf('A', 'B', 'C'));
            Assert.That("Hello", Is.AnyOf("Hi", "Hello", "G'day"));

            string[] greetings = { "Hi", "Hello", "G'day" };
            Assert.That("Hello", Is.AnyOf(greetings));

The one not working is:

            Assert.That('A', Is.AnyOf("ABC"));

But that one doesn't work in the master branch either, so isn't something broken. I would that to be written as:

            Assert.That("ABC", Does.Contain('A'));

This brings me to the point that Is.AnyOf is the reverse of Does.Contain and if we have an array, we should write:

            string[] greetings = { "Hi", "Hello", "G'day" };
            Assert.That(greetings, Does.Contain("Hallo"));

Which actually started the whole Is.AnyOf discussion as the error message was confusing.
It is now:

  Expected: some item equal to "Hallo"
  But was:  < "Hi", "Hello", "G'day" >

So this PR has not broken anything, but added something we might or might not want.

@OsirisTerje
Copy link
Member

OsirisTerje commented Apr 20, 2023

This brings me to the point that Is.AnyOf is the reverse of Does.Contain and if we have an array, we should write:

        string[] greetings = { "Hi", "Hello", "G'day" };
        Assert.That(greetings, Does.Contain("Hallo"));

Good point @manfred-brands . So all this do is to change the a) syntax b) error message c) semantics (who is being checked)

Given the code:

[Test]
    public void TestAnyOf()
    {
        var ls = new List<string> { "a", "b", "c" };
        string x = "e";
        Assert.That(x, Is.AnyOf(ls));
    }

    [Test]
    public void TestDoesContain()
    {
        var ls = new List<string> { "a", "b", "c" };
        string x = "e";
        Assert.That(ls, Does.Contain(x));
    }

We get the results:
image
and
image

That raises the question if the Is.AnyOf should have been implemented with a redirect to the Does.Contain. Right now we have two pieces of code that actually does the same, if I get this right. Are there any differences really here?

The semantics is slightly different though, the Is.AnyOf checks the 'x', whereas the Does.Contain checks the array 'ls'.

@stevenaw
Copy link
Member

Thanks @manfred-brands !
I had a feeling this scenario had been discussed, I just couldn't remember the outcome

@ivan-gurin
Copy link
Contributor

Hey how about

public AnyOfConstraint AnyOf(params object?[]? expected)
?
I want Has.All.AnyOf() :)

@OsirisTerje
Copy link
Member

This issue is closed.

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

Successfully merging a pull request may close this issue.

4 participants