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

Assertion for key-value-pair #3470

Closed
Falco20019 opened this issue Feb 14, 2020 · 14 comments · Fixed by #3486
Closed

Assertion for key-value-pair #3470

Falco20019 opened this issue Feb 14, 2020 · 14 comments · Fixed by #3486

Comments

@Falco20019
Copy link
Contributor

Falco20019 commented Feb 14, 2020

I was looking for a good way to check key-value-pairs. I know about DictionaryContainsKeyConstraint and DictionaryContainsValueConstraint, but have not found any option to combine them on the same item. Chaining it with And will also operate on the asserted value and not on the item identified by the key.

public void Merge_NoConflicts()
{
    var mainDict = new Dictionary<string, int>
    {
        { "Test", 1 }
    };
    var otherDict = new Dictionary<string, int>
    {
        { "Test2", 42 }
    };

    var result = mainDict.Merge(otherDict);

    // --- Variant 1 (full checks, ugly to read) ---
    Assert.That(result, Contains.Key("Test"));
    Assert.That(result["Test"], Is.EqualTo(1));
    Assert.That(result, Contains.Key("Test2"));
    Assert.That(result["Test2"], Is.EqualTo(42));

    // --- Variant 2 (better but throws KeyNotFoundException, would like outlike like SomeItemsConstraint) ---
    Assert.That(result["Test"], Is.EqualTo(1));
    Assert.That(result["Test2"], Is.EqualTo(42));

    // --- Variant 3 (works, better than 1 but but uglier to read than 2) ---
    Assert.That(result, Contains.Item(new KeyValuePair<string, int>("Test", 1)));
    Assert.That(result, Contains.Item(new KeyValuePair<string, int>("Test2", 42)));

    // --- Variant 4 (looks correct, but ContainValue is called on result, so no relation to the pair) ---
    Assert.That(result, Contains.Key("Test").And.ContainValue(1));
    Assert.That(result, Contains.Key("Test2").And.ContainValue(42));

    // --- Variant 5 (some API I would prefer but can't find) ---
    Assert.That(result, Contains.Key("Test").WithValue(1));
    Assert.That(result, Contains.Key("Test2").WithValue(42));
}

It would be pretty easy to implement an DictionaryContainsKeyValueConstraint that implements the Matches method using TryGet. It's also easy to add the WithValue method to DictionaryContainsKeyConstraint and have it work with all notations (Contains.Key, Does.ContainKey, ...). But before doing a PR, I wanted to know if there already is a good way of doing it and just not finding it.

@CharliePoole
Copy link
Contributor

CharliePoole commented Feb 14, 2020

I like your last option (which doesn't exist yet) but even better, for consistency, I'd prefer one of these...

Assert.That(result, Contains.Key("Test").EqualTo("1");
Assert.That(result, Contains.Item("Test").EqualTo("1");

The first alternate does not yet exist, while the second exists but means something else. Both require the existing initial constraint to work in two ways, depending on whether it was followed by a further constraint. There are examples of how we already do that in the code for detecting properties and attributes.

Note that for readability, you can already throw .With. into the expression with no other effect.

@Falco20019
Copy link
Contributor Author

Falco20019 commented Feb 14, 2020

I looked a bit more into the code base and successfully played around with an implementation.

Using the Contains.XXX("Test").EqualTo("1") approach, we would enforce the EqualTo to have another meaning as if used with Contains.XXX("Test").And.EqualTo("1") or Contains.XXX("Test").With.EqualTo("1") which would need to look at result again. I think this is especially important for chaining it in these ways

Assert.That(result, Contains.Key("Test").WithValue(1).And.ContainKey("Test2"));
Assert.That(result, Contains.Key("Test").WithValue(1).And.ContainKey("Test2").WithValue(42));

The later part should still target result and not the item.

That's why I tried to use another keyword (WithValue) that doesn't have another meaning to avoid assumptions about the meaning and making it clear, that this is only affecting the key-value-pair. Rest of the constraint untouched.

If you still prefer to use EqualTo in a different meaning, I would prefer the Contains.Item("Test").EqualTo("1") approach for readability as it sounds more natural about checking the item and not the key.

From an implementation perspecitive, I like the Contains.Key(x).WithValue(x) still most, since Contains.Key() already gives you a DictionaryContainsKeyConstraint and therefore puts you logically into an mental IDictionary context, making clear, that WithValue is referencing the item for this key and that all other CollectionConstraint properties relate to the original value.

@CharliePoole
Copy link
Contributor

I get what you are saying about double meanings, but that ship has sailed. 😄

We already have both Has.Property("X") testing the simple presence of a property and Has.Property("X").EqualTo(42). Internally, the infrastructure for this sort of thing is already present in the concept of a SelfResolvingOperator, which can end up producing various constrraints depending on what comes next. In the case of properties, it's either a PropertyConstraint or a PropertyExistsConstraint. However, that approach gets a bit deeper into the internals of the syntax than you may want to go.

For the Contains.XXX part, Contains.Entry pops into my head as something we could use, either now or eventually

@Falco20019
Copy link
Contributor Author

Falco20019 commented Feb 14, 2020

Thanks for the explanation. Then it‘s definitely best to keep it consistent with the properties and use Contains.Key(x).EqualTo(y). Even though it‘s not completely comparable as for the property it IS the value of it. For the key, it’s the value of the pair, not the key itself.

I will give it a look on Monday and prepare a PR :) I‘m usually not afraid of depths in code ;)

@CharliePoole
Copy link
Contributor

You can consider "deeper" to mean "I wish I had written it more transparently in the first place." 😄

@CharliePoole
Copy link
Contributor

@nunit/framework-team Please review what @Falco20019 says he wants to do and let him now if it seems like a reasonable syntax that won't surprise anybody. As y'all know, I kibitz here but don't make decisions these days. 😺

If you feel that modifications to what can come after Contains.Key are too confusing for users, then I suggest a new syntax for dictionaries, which won't conflict with the existing stuff.

Assert.That(myDict, Contains.Entry("key"));
Assert.That(myDict, Contains.Entry("key", value"));
Assert.That(myDict, Contains.Entry("key").SomeConstraint);

If the third form is provided, then the second may not be needed, since you could use Contains.Entry("key").EqualTo(42) where the constraint applies to the value of the entry, if one is found. I think this form is superior to the second form, because you can use other constraints, like GreaterThan or TypeOf in place of EqualTo.

@rprouse
Copy link
Member

rprouse commented Feb 29, 2020

I think it is a reasonable request. Personally, I feel that variant 5 reads best as it is the intention of the test. That is how I would describe the test in English.

// --- Variant 5 (some API I would prefer but can't find) ---
Assert.That(result, Contains.Key("Test").WithValue(1));
Assert.That(result, Contains.Key("Test2").WithValue(42));

I think that Contains.Key("x").EqualTo("y") is a bit confusing as EqualTo could refer to the key or the value.

@Dreamescaper
Copy link
Member

Dreamescaper commented Mar 2, 2020

You could use Create static method for Variant 3, which would look better:

    Assert.That(result, Contains.Item(KeyValuePair.Create("Test", 1)));
    Assert.That(result, Contains.Item(KeyValuePair.Create("Test2", 42)));

Alternative suggestion - allow to test equality of KeyValuePair and ValueTuple with 2 elements.
Something like this could be used then:

Assert.That(result, Does.Contain(("Key", "Value")))```

@rprouse
Copy link
Member

rprouse commented Mar 5, 2020

@Dreamescaper I like the look of the tuple option, but it isn't really discoverable through intellisense like the Contains.Key("Test").WithValue(1)

@mikkelbu mikkelbu added this to the 3.13 milestone May 7, 2020
@Falco20019
Copy link
Contributor Author

Is there an ETA for 3.13? 3.12 was a year ago.

@jnm2
Copy link
Contributor

jnm2 commented May 9, 2020

Not that I know of. @rprouse Is there anything in the milestone that can't wait for 3.14?

@mikkelbu
Copy link
Member

mikkelbu commented May 9, 2020

The only thing I can think of is that it would be nice to get #2574 included. I planned to figure out its status this weekend, and will perhaps have time tomorrow.

@jnm2
Copy link
Contributor

jnm2 commented May 9, 2020

@Falco20019 It's always hard to say for a team of volunteers. Some of us are more affected by the current times than others.

You can use https://www.myget.org/feed/nunit/package/nuget/NUnit in the meantime.

@Falco20019
Copy link
Contributor Author

@jnm2 I totally understand that :) I only saw that some of the fixed issues wait for over a year to be released. And if there is nothing blocking a minor release (that has been tested), I prefer to release more minors instead of wait for a year for each.

Personally, I only asked to know if we should just get a build from the feed or wait for an release.

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.

6 participants