`.ContainKey().WithValue()` and `.Or`/`.And` interact incorrectly

[ssrmm]

Combining Or/And with the DictionaryContainsKeyValuePairConstraint produces incorrect results, see examples below.

The error messages in the failure cases imply that the expression is parsed in a weird/unintended way.

[Test]
public void BugReport()
{
    var dictionary = new Dictionary<string, int>
    {
        { "a", 123 },
        { "b", 456 }
    };

    Assert.Multiple(() =>
    {
        Assert.That(dictionary, Does.ContainKey("a").WithValue(456).Or.ContainKey("b").WithValue(123), "Passes but should fail");
        Assert.That(dictionary, Does.ContainKey("a").WithValue(456).And.ContainKey("b").WithValue(456), "Passes but should fail");

        // Expected: dictionary containing key "c" or dictionary containing entry ["c", 456]
        // But was:  < ["a", 123], ["b", 456] >
        Assert.That(dictionary, Does.ContainKey("a").WithValue(123).Or.ContainKey("c").WithValue(456), "Fails but should pass");

        // Expected: dictionary containing key "c" and dictionary containing entry ["c", 456]
        // But was:  < ["a", 123], ["b", 456] >
        Assert.That(dictionary, Does.ContainKey("a").WithValue(123).And.ContainKey("c").WithValue(456), "Fails but for the wrong reason");
    });
}

[OsirisTerje]

Repro code here: https://github.com/nunit/nunit.issues/tree/main/Issue4670

[OsirisTerje]

@Falco20019 @rprouse @mikkelbu
This PR #3486 introduced the WithValue which seems to trigger a bug when using Or/And constraint expressions, It might be because the or/and only looks at the immediate left and right constraints, and assumes they have been fully resolved - which they doesn't seem to be. Or, it might be by design, ref docs on the or/and stating they resolve left->right. I am not exactly sure how that is to be interpreted.
The constraint stack also seems to contain two constraints on each side, both a DictionaryContainsKeyConstraint and a DictionaryContainsKeyValuePairConstraint. If the WithValue had been a modifier, it should work, but now it seems to chain in a new constraint. I am not sure if we have other places where this is being done.

When the Or/And resolve the object looks like:

indicating it doesn't see two DictionaryContainsKeyValuePairConstraint, but one of each.

[OsirisTerje]

Just to highlight the peculiarity of this situation:

In the provided statement

 Assert.That(dictionary, Does.ContainKey("a").WithValue(123).Or.ContainKey("c").WithValue(456))

yields an Or constraint with 'c' as the Left and ['c', 456] as the Right. It failed, as expected, but it should have passed.

Note that the 'c' is incorrect.

However

Assert.That(dictionary, Does.ContainKey("a").WithValue(123).Or.ContainKey("b").WithValue(789));

This test passes, even though the value on the right side is false, but the key 'b' does exist."

[Falco20019]

I can confirm that the issue is, that we have two constraints not linked properly. Technically, it should work to use the And (which would compare contains key AND contains key with value) on building. I will prepare tests and correct it. It would be nicer to replace the key check and only do the combined one.

[OsirisTerje]

It would be nicer to replace the key check and only do the combined one.

Do you mean having only one constraint ?
If so, I believe that would be better, and using the WithValue as a modifier to that constraint. We could still have the two Does.ContainKey and Does.ContainValue, but let both modify a single constraint. We should not change the public interface here, so the two existing ones should be marked as obsolete and redirect to a new one.

[Falco20019]

Having only one would have also made it easier. But I didn't want to break public API and therefore added it as new one. Also, it would have made the match-check more complex. The PR is currently replacing the existing constraint added by ContainsKey with the combined one that matches the entry (checking for key + value pair).

In my opinion, Does.ContainKey and Does.ContainValue are still valid and have a different meaning then checking for a key-value-pair. So I would not mark them obsolete as they fullfil different use-cases. I'm also not sure if having one constraint that allows checking for key-only, value-only or key-value-pair would increase maintainability.

[OsirisTerje]

Fair point.

PS: I meant obsolete on the "older" constraints, not the keywords :-)

[OsirisTerje]

@ssrmm Please verify that this work: https://www.myget.org/feed/nunit/package/nuget/NUnit/4.2.0-alpha.0.10

[ssrmm]

Looks good. Thank you.