Skip to content

Conversation

@dennisdoomen
Copy link
Member

@dennisdoomen dennisdoomen commented Jun 9, 2021

Because of some accidental design choices, in previous releases, when the expectation is a dictionary, it was picked up by the GenericEnumerableEquivalencyStep. This resulted in an IDictionary<TKey, TValue> being treated as a collection of KeyValuePair<TKey, TValue>. In 6.0, some of the semantics of the equivalency steps changed in such a way that a dictionary is now picked up by the GenericEquivalencyStep. This is expected, but had the side-effect that when a dictionary is expected, it really expects a dictionary.

This PR changes GenericEnumerableEquivalencyStep so it tries to convert a collection of key-value pairs back to a dictionary and then does the equivalency check.

Fixes #1598

@dennisdoomen dennisdoomen force-pushed the Fix/CollectionOfKeyValuePair branch 7 times, most recently from 698510a to 8e247d4 Compare June 16, 2021 19:01
@dennisdoomen dennisdoomen force-pushed the Fix/CollectionOfKeyValuePair branch 8 times, most recently from 1d1efe4 to 6aeb59e Compare June 26, 2021 14:30
@dennisdoomen dennisdoomen force-pushed the Fix/CollectionOfKeyValuePair branch 2 times, most recently from 0fcad04 to 801c7fc Compare June 26, 2021 20:20
@dennisdoomen dennisdoomen marked this pull request as ready for review June 26, 2021 20:20
@dennisdoomen dennisdoomen requested a review from jnyrup June 26, 2021 20:21
@dennisdoomen dennisdoomen changed the title A collection of key-value pairs should be equivalent to a dictionary with the same pairs A collection of similarly typed key-value pairs should be equivalent to a dictionary Jun 26, 2021
@dennisdoomen dennisdoomen force-pushed the Fix/CollectionOfKeyValuePair branch from 801c7fc to 3dd80f7 Compare June 26, 2021 20:41
Copy link
Member

@jnyrup jnyrup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give a high level description of what's changed.
I have a bit of a hard time separating the restructuring from the change in logic.

Have you tried my suggestion from
#1598 (comment) ?

// Assert
act.Should().Throw<XunitException>()
.WithMessage("*expectation*keys*Int32*compatible types*IDictionary`2[System.String,System.String]*");
.WithMessage("Expected the expectation (of type Dictionary<int, string>) to be a dictionary or collection of key-value pairs that is keyed to type System.Int32*");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

expectation and actual seems flipped in the act.

Comment on lines 27 to 28
bool isDictionary = DictionaryInterfaceInfo.TryGetFrom(expectationType, "expectation", out var expectedDictionary);
if (comparands.Expectation != null && isDictionary)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider only calling DictionaryInterfaceInfo.TryGetFrom if comparands.Expectation != null

return false;
}

private static bool AssertIsCompatiblyTypedDictionary(Type expectedType, object subject)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a now outdated comment on line 118 mentioning AssertIsCompatiblyTypedDictionary

if (expectedDictionary.TryConvertFrom(comparands.Subject, out var convertedSubject))
{
comparands.Subject = convertedSubject;
DictionaryInterfaceInfo.TryGetFrom(comparands.Subject.GetType(), "subject", out actualDictionary);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

double check: is the return value of TryGetFrom unimportant?

@dennisdoomen
Copy link
Member Author

Can you give a high level description of what's changed.

Sorry. Forgot about that one. Will do.

Have you tried my suggestion from #1598 (comment) ?

Not really. The old implementation was kind of an accidental side-effect. Since the expectation is a dictionary, I decided to implement this where it belongs. Now it's completely symmetric.

I have a bit of a hard time separating the restructuring from the change in logic.

I understand, but I don't think I could have avoided that. The idea of extracting some of the logic happened while I was trying to implement the algorithm.

@dennisdoomen dennisdoomen force-pushed the Fix/CollectionOfKeyValuePair branch 2 times, most recently from 36fcacb to 6c9c2c8 Compare July 11, 2021 19:31
@dennisdoomen dennisdoomen force-pushed the Fix/CollectionOfKeyValuePair branch from 6c9c2c8 to 51a4bd0 Compare July 13, 2021 18:47
@dennisdoomen dennisdoomen force-pushed the Fix/CollectionOfKeyValuePair branch from 51a4bd0 to 7cf777d Compare July 20, 2021 17:25
@dennisdoomen dennisdoomen requested a review from jnyrup July 20, 2021 18:25
@dennisdoomen dennisdoomen merged commit 838f4cd into fluentassertions:release-6.0 Jul 21, 2021
@dennisdoomen dennisdoomen deleted the Fix/CollectionOfKeyValuePair branch July 21, 2021 15:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

A collection of key value pairs is no longer equivalent to a dictionary

2 participants