DictionaryContainsKeyConstraint behaviour is inconstant with Dictionary.ContainsKey when the dictionary uses a custom Comparer

[EamonHetherton]

If you have a dictionary that was created with a custom comparer then the DictionaryContainsKeyConstraint does not work as expected.

var dictionary = new Dictionary<string, string>(StringComparer.OrdinalIgnoreCase) { { "Hello", "World" }, { "Hola", "Mundo" } };
Assert.That(dictionary, Does.ContainKey("hello"));

[rprouse]

Until this is fixed, you can use Using as a workaround,

Assert.That(dictionary, Does.ContainKey("hello").Using((IComparer)StringComparer.OrdinalIgnoreCase));

[rprouse]

Failing unit tests for this issue are at #2838, currently marked explicit.

[DaiMichael]

I can take a look at this, I have a local branch and have the unit tests which were nicely added by Eamon.

[ChrisMaddock]

Sounds great, thanks @DaiMichael!

From a quick look at the code on my phone however, it looks like we may be in ‘breaking change’ territory here.

DictionaryContainsKeyConstraint currently uses NUnitEqualityComparer, rather than the default comparer for a type as a dictionary would. NUnitEqualityComparer has various additions that allow certain types which wouldn’t compar equal with normal .NET Equals() to ‘be equal’.

Given the nature of the constraint, my opinion right now is that we should make the change so this constraint always uses the comparer of the dictionary - despite the fact that may break some existing users tests. It’s probably worth waiting for others on @nunit/framework-team to chime in however, on what they think the best way to take this is!

Like I say, I’ve only looked on my phone, so this may all be wrong!

[DaiMichael]

@ChrisMaddock

So having a bit more of a look, at first stab I would look to check to see if the passed in Dictionary has a EqualityComparer<TKey>.Default. If this is the case we can use the current 'NUnitEqualityComparer' else we use the one defined in the Dictionary, which in theory should be the desired one.

I haven't see if this would break any tests yet but it should only change behaviour when the use case requires it?! Famous last words obviously. I'll make the change locally and see what comes out in the wash...

[ChrisMaddock]

@DaiMichael I agree that would avoid broken tests - but I worry it's complicating the 'expected result' a lot to avoid the breaking change. I'm personally currently erring on the side of going for the breaking change - but keen to here what others think. 🙂

[CharliePoole]

It would be cool to see a test the change will break. Possibly two of it can break on both directions.

[jnm2]

I'd rather make the breaking change. It seems wrong to have to read Dictionary<,>.Comparer though. What if my type implements IDictionary and delegates to an inner dictionary instance?

Can we instead recognize the IDictionary/IDictionary<,>/IReadOnlyDictionary<,>/ILookup<,> /KeyedCollection<,> and call the Contains(TKey) or ContainsKey(object) method?
Given the constraint name, what if we duck-type and use the public bool-returning instance ContainsKey method that takes a single parameter? Then if no such public method exists, check for the same method on all implemented interfaces? Something like this?

[DaiMichael]

I'll look to proceed with the, non-easy way 😉

[DaiMichael]

@CharliePoole @jnm2 @ChrisMaddock

I've done the change, shamelessly taken the code @jnm2 posted up for looking for the ContainsKey(). This works and I will add some tests for this if we continue to use that code as a way to solve the issue.

The new code fixes the 2 new tests, yay! 🎉
It breaks 3 tests 😞.

Code on my branch
https://github.com/DaiMichael/nunit/blob/issue-2837/src/NUnitFramework/framework/Constraints/DictionaryContainsKeyConstraint.cs

Tests
https://github.com/DaiMichael/nunit/blob/issue-2837/src/NUnitFramework/tests/Constraints/DictionaryContainsKeyConstraintTests.cs

Failures

1. FailsWhenNotUsedAgainstADictionary -> This is a list of KeyValuePair and it uses KeyValuePairsComparer from NUnitEqualityComparer.
2. IgnoreCaseIsHonored is using DictionaryContainsKeyConstraint.IgnoreCase which is totally ignored by the ContainsKey().
3. UsingIsHonored is using DictionaryContainsKeyConstraint.Using which adds the Func<> to NUnitEqualityComparer and now call ContainsKey().

So I guess it comes down to whether we want to break the current use cases? We could default to using the old method if we can't find ContainsKey() but no one seemed keen.

I'm all 👂's on what people think is the best way to proceed?

[jnm2]

Is FailsWhenNotUsedAgainstADictionary is failing because the message changed or because it no longer throws an exception?

IgnoreCase is counter-intuitive to me. What's the use case for using a comparer that conflicts with the dictionary to check for a key's existence? A more semantically appropriate approach would be to use a collection assertion on dictionary.Keys since the fact that it's a dictionary is a red herring.

Probably the best way to proceed is to either deprecate the .IgnoreCase and .Using options, or else internally keep a second code path. Rather than keeping the existing code though, it's tempting to me to generate a collection assertion like Has.Property("Keys").With.Some.EqualTo(expected).IgnoreCase etc.

@rprouse What do you think so far? Is my straw-man ContainsKey a good thing, and should we keep existing behavior if .IgnoreCase is touched?

@DaiMichael If we do this, I'd like to see/contribute tests showing the new ContainsKey code working against:

• a class that inherits from object and implements IReadOnlyDictionary<,>
• ditto for IDictionary<,>
• ditto for IDictionary
• ditto for ISet<> (negative test, since sets don't have keys)
• ditto for ILookup<,>
• a class that inherits from KeyedCollection<,>
• a class that inherits from object, implements no interfaces, and declares a public ContainsKey method
• a class that inherits from Dictionary<,>, implements no additional interfaces, and declares a new public ContainsKey method which acts different from the base.
• a class that inherits from object, implements no interfaces, and declares a public Contains(TKey) method

[DaiMichael]

@jnm2 FailsWhenNotUsedAgainstADictionary fails as the ArgumentException message has changed, this would be easy to change for the input type (i.e. IDictionary, ISet, etc...).

I agree in that I would add a lot more test cases as you mention but I wanted to gauge what the end game was before writing a lot of tests, only for them to be discarded if we changed tact.

[jnm2]

I see, thanks!
That's the strategy I’d expect. (Btw ‘changed tack’ is one of those phrases that you gotta watch out for! 😃)

[ChrisMaddock]

I agree with all your comments, @jnm2. To me, DictionaryContainsKeyConstraint is testing a very specific thing, so other constraints should be used for testing other things!

Rather than keeping the existing code though, it's tempting to me to generate a collection assertion like Has.Property("Keys").With.Some.EqualTo(expected).IgnoreCase

I especially like this idea! But do think we should deprecate if we go for that, as that's could create some unexpected behaviour for anyone writing new tests! The purpose of this change, in my eyes, would be to avoid breaking compilation for existing tests.

[jnm2]

@ChrisMaddock I was thinking we'd still return a DictionaryContainsKeyConstraint which would delegate internally to Has.Property("Keys").With.Some.EqualTo(expected).IgnoreCase for compatibility, and that the resulting hat the pass/fail behavior would be identical to what we do today. But if I'm wrong, never mind. :D

[DaiMichael]

@jnm2 ISet<> doesn't support ContainsKey, it does support Contains but then every collection base class does ala List does and it will just test the equality between items. This doesn't seem relevant in the case of DictionaryContainsKey tests?

I don't think we should test in this case and it isn't currently working as Duck Type can't find a ContainsKey with a TKey generic, so it is a different use case.

[DaiMichael]

@jnm2 @ChrisMaddock

Okay, I've checked the tests in but this isn't complete as I have a few on Ignore.

As per above comment ISet<> doesn't seem valid test case as it isn't really a "keyed" collection. The 'Contains' would check if the item is in the set, not that it is a key. I have not done this.

IDictionary' fails as it doesn't contain ContainsorContainsKey`, this is because we are looking for a method with a generic TKey parameter, I could adjust the lookup to pull either back and ignore the generic constraint we have on it.

The 2 tests which just implement the Contains and ContainsKey don't work as the CollectionConstraint' expects there to be an IEnumerable` interface, so failure out side of this change and perhaps valid. We could do this as test but expect to throw an exception. Currently not done as not valid?

Finally... 😓 I still have the two test from first check in IgnoreCase flag and Using as I wasn't sure what to do here yet.

So questions are

1. Do I modify the duck type to get Contains or ContainsKey and ignore if TKey is generic and just get the method.
2. What to do with IgnoreCase flag and Using

https://github.com/DaiMichael/nunit/tree/issue-2837

[jnm2]

@DaiMichael

ISet<> doesn't support ContainsKey

Sorry, I didn't explain what I meant! We're on the same page. That's why I proposed adding a negative test: that way we show that we are intentionally not supporting it.
Does that help answer your question 1?

With question 2, I don't have a strong leaning on this one. We either keep the original implementation to fall back to, or we hard-obsolete those methods which the team has not wanted to do when hard-obsoletion was suggested in the past. @ChrisMaddock, @rprouse?

[jnm2]

I hadn't meant that you should explicitly inherit from object, just that you shouldn't inherit from a class that already implemented an interface. But it's cool.

[DaiMichael]

@jnm2 It's been a lonnnnggg week in work... I was just being dumb on the negative tests and actually the plain objects with Contains and ConatinsKey are good negative tests as well...

I'll revisit... ditto on the ISet<>...

I'll take look at the mark Obsolete and make backwards compatible in line with previous changes.

[ChrisMaddock]

I'll take look at the mark Obsolete and make backwards compatible in line with previous changes.

I'd agree this is preferable 🙂

[DaiMichael]

@ChrisMaddock @jnm2

I've finished up on the additional unit tests but not commited yet.

Marking Using and IgnoreCase as [Deprecated] is a bit sticky. They are all implemented in the CollectionItemsEqualConstraint class.

I can't think of a way of allowing backwards compatibilty and marking it [Deprecated] without :

1. Make the methods in CollectionItemsEqualConstraint virtual and override them (call base method but add point 2).
2. Create a _obsoleteFlag flag that then uses the old method when set via the overridden methods.

This will obviously work but isn't particularly pretty 😞

Any better thoughts.

[jnm2]

@DaiMichael We'd do the obsoletion by declaring shadowing methods on the DictionaryContainsKeyConstraint class via the new keyword. It's highly unlikely that DictionaryContainsKeyConstraint is being used polymorphically through CollectionItemsEqualConstraint.

[DaiMichael]

@jnm2 - I had started that approach. I perhaps ask you guys too many questions... It's just I don't want to waste anyone's time by making pull requests that are not want is wanted.

So we now have a bunch more tests as requested and I have deprecated the other calls used in the tests with backwards compatability for the deprecated calls.

I'll do a pull request now... 😉

[jnm2]

We've already discussed how inheriting from CollectionConstraint is a leaky abstraction. @DaiMichael brought to our attention that CollectionConstraint.ApplyTo throws ArgumentException for things that aren't IEnumerable:

nunit/src/NUnitFramework/framework/Constraints/CollectionConstraint.cs

Lines 71 to 75 in 46b62b6

	public override ConstraintResult ApplyTo<TActual>(TActual actual)
	{
	IEnumerable enumerable = ConstraintUtils.RequireActual<IEnumerable>(actual, nameof(actual));
	return new ConstraintResult(this, actual, Matches(enumerable));
	}

This is a problem if we want to enable DictionaryContainsKeyConstraint to work on dictionaries that don't implement interfaces.

Should we override ApplyTo to get rid of this check, or should we require the IEnumerable interface as a marker before we duck-type and use the ContainsKey method?

[DaiMichael]

To play devils advocate 👿

Is a plain object (as per test) a Dictionary just because it implements either ContainsKey and or Contains method. I'm not saying these aren't valid traits to support but in the context of DictionaryContainsKeyConstraint ?

[jnm2]

The syntax people will be reading is Contains.Key, so I'd expect that to work on KeyedCollection or ILookup which are not dictionaries. Once you go there, why not let the type decide if you can ask whether it contains a key?

[DaiMichael]

Very fair point 👍, as I'm thinking from the lower DictionaryContainsKeyConstraint rather than the real use case of Contains.Key. Which comes then back to my

I'm not saying these aren't valid traits to support

So where we support this in the framework is mute to a degree.

[DaiMichael]

Should we override ApplyTo to get rid of this check, or should we require the IEnumerable interface as a marker before we duck-type and use the ContainsKey method?

I have implemented this and checked in. I have overriden the base CollectionConstraint in DictionaryContainsKeyConstraint as I think the abstract version is fair considering the name of the class. This will also maintain backwards compatibility for 6 other classes that inhert this abstract class.

.... I can always revert if decided last minute ...

[jnm2]

I was confused when you said overridden, since shadowing and overriding are different things, but by looking at the code I see we're on the same page. =)

[jnm2]

@ChrisMaddock This should be reopened until https://github.com/DaiMichael/nunit/tree/issue-2837 is rebased in a new pull request, right?

[ChrisMaddock]

It should, sorry!

[DaiMichael]

@ChrisMaddock - Soooo.. I think I have rebased, all seems to run, do you want me to do another PR?

Also - With regards to the "failing" tests. I don't have an issue locally but I'm a tad confused as in nunit.framework we reference standard but in nunit.framework.test we are referencing netcore ? I don't understand that....

[ChrisMaddock]

Yes please @DaiMichael!

Also - With regards to the "failing" tests. I don't have an issue locally

That's strange! The issue I thought we had before, the CI build will catch - so we should be able to see if the problems still there.

nunit.framework we reference standard but in nunit.framework.test we are referencing netcore ? I don't understand that....

I'm very much not the best person to explain this...but .NET Standard is the "API" that these builds target - but the tests must be ran on an actual platform. The platforms we run those tests on is .NET Core.

[DaiMichael]

@ChrisMaddock - okay ignore all of the last message... I am working on this... post haste but confusion/ misunderstanding of github when I started helping (sic) out means that I have a polluted branch (on this issue) and I have some of another change (in an earlier version) in there. I've spent the day been rather fustrated but I have detangled the mess but I need to remove the bad commit to this branch now. I will then be able to rebase (properly) and I suspect I will see the failing unit tests. Hopefully an exciting day doing this tomorrow. Luckly I just had a hip op (no, I'm not that old ;)) and I'm limited (i.e. I can't) on my cycling!

I'm very much not the best person to explain this...but .NET Standard is the "API" that these builds target - but the tests must be ran on an actual platform. The platforms we run those tests on is .NET Core.

I understand how it all works, I think the branch issue from above was making a mess, I've only just been been dipping my toe into Standard/Core/Framework recently so I'm also a bit noob around it and easily confused.

[ChrisMaddock]

Gah - I know that feeling well...I'm a mercurial-man, so regularly end up in a mess with git as soon as something non-trivial happens! Hope you can get things sorted out - let us know if we can help at all! 🙂

[jnm2]

Since I'm the one that caused all those changes underneath you, I'll gladly rebase your changes and fix merge conflicts if you'd like.

[DaiMichael]

@jnm2

Since I'm the one that caused all those changes underneath you, I'll gladly rebase your changes and fix merge conflicts if you'd like.

I'm 99.9% sure this is my fault. I have a very old check in from the #2786 issue, I must have committed a very early version to this branch, now I've detangled my branches and my master.... I should be able to remove the bad commit.

If I'm still struggling by eod tomorrow, I might blame you 😋 and ask for help, but fingers crossed.

@ChrisMaddock - amen to mecurial...

[DaiMichael]

New PR #2967