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 😉