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

Contains.Key no longer working for IDictionary #3331

Closed
ItsVeryWindy opened this issue Jul 30, 2019 · 4 comments · Fixed by #3400
Closed

Contains.Key no longer working for IDictionary #3331

ItsVeryWindy opened this issue Jul 30, 2019 · 4 comments · Fixed by #3400

Comments

@ItsVeryWindy
Copy link

I had some tests that were checking for a key in the exception data property that have stopped working since updating the version of nunit.

Narrowing it down I believe it's because it only implements the non generic IDictionary and maybe a change has been made that hasn't taken that into account. This has stopped working since 3.11

It can be replicated fairly easily.
Assert.That(new Exception().Data, Contains.Key("hello"));

Previously this would return

Expected: dictionary containing key "hello"
But was:  <empty>

but now returns

System.ArgumentException : The ListDictionaryInternal value must have a ContainsKey or Contains(TKey) method.
@rprouse
Copy link
Member

rprouse commented Jul 30, 2019

Thanks, it is odd that this was working before. We did make changes to include Exception data in messages, but knowing the nature of that change I don't think it is related.

@mikkelbu
Copy link
Member

mikkelbu commented Aug 1, 2019

The regression was introduced in version 3.11, I guess in #2837 / #2994

The check only matches ContainsKey method or a Contains method in a generic class where the type parameter is called TKey and where the sole parameter of Contains have the type as TKey - see e.g. https://github.com/DaiMichael/nunit/blob/70b5915bd844cf1f08c558452e2427c66c3c68dc/src/NUnitFramework/framework/Constraints/DictionaryContainsKeyConstraint.cs#L227. There are some discussion about this choice in the issues and associated PRs.

So https://referencesource.microsoft.com/#mscorlib/system/collections/listdictionaryinternal.cs,184 does not match the current implementation.

@stevenaw
Copy link
Member

Since this seems to be a regression, I figured we would want to fix it. I've made an attempt at fixing it in the linked PR.

@mikkelbu mikkelbu assigned mikkelbu and stevenaw and unassigned mikkelbu Oct 14, 2019
@mikkelbu
Copy link
Member

@stevenaw I think it makes sense to fix it, and had actually planned to look at it, but I've not had much time the last month or so. So great that you took a look at it.

@rprouse rprouse added this to the 3.13 milestone May 11, 2020
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