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

Add the ability to exclude non-browsable members from equivalency tests

Merged
merged 4 commits into from Apr 16, 2022

Conversation

logiclrd
Copy link
Contributor

@logiclrd logiclrd commented Feb 25, 2022

In #1807, changes were made to allow non-browsable members to be excluded from equivalency tests. But, these changes ended up being reverted because they affected processing of both subject & expectation, but include/exclude functions are supposed to apply only to the expectation. In addition, the original changes only worked properly when the subject & expectation were of the same data type. In my actual usage, the subject and expectation are different data types and only one of them marks the member as non-browsable. This wasn't factored in in the original implementation, for which the unit tests exclusively test the case where the subject and expectation are the same data type. The changes in #1807 got reverted since they were in an incomplete/uncertain state and were blocking a release.

This PR makes another attempt at adding this feature. In addition to the tests added in #1807, the PR adds unit tests that comprehensively cover the behaviour when the subject and expectation are different types and only one of them marks a member as non-browsable.

This PR then also provides an implementation that allows excluding non-browsable members from the expectation using an Excluding method, and from the subject using a separate Ignore method.

@logiclrd logiclrd changed the title Fixed excluding non-browsable members when the subject and expectation are different types Fix excluding non-browsable members when the subject and expectation are different types Feb 25, 2022
@coveralls
Copy link

coveralls commented Feb 25, 2022

Pull Request Test Coverage Report for Build 2172527928

  • 30 of 31 (96.77%) changed or added relevant lines in 7 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.01%) to 95.176%

Changes Missing Coverage Covered Lines Changed/Added Lines %
Src/FluentAssertions/Equivalency/Selection/ExcludeNonBrowsableMembersRule.cs 1 2 50.0%
Totals Coverage Status
Change from base Build 2171653108: 0.01%
Covered Lines: 8195
Relevant Lines: 8494

💛 - Coveralls

@dennisdoomen dennisdoomen requested a review from jnyrup Feb 25, 2022
@jnyrup
Copy link
Member

jnyrup commented Feb 26, 2022

Thanks for dogfooding the feature 👍

My first concern was that we normally just "let the the expectation drive the comparison", e.g. when using an anonymous object with a subset of members.

But it seems to me that we have approached this issue from the wrong angle.

This example

var subject = new { Browsable = 1 };
var expected = new NonBrowable { Browsable = 1, NonBrowsable = 2 };
subject.Should().BeEquivalentTo(expected, opt => opt.ExcludingNonBrowsableMembers());

private class NonBrowable
{
    public int Browsable { get; set; }

    [EditorBrowsable(EditorBrowsableState.Never)]
    public int NonBrowsable { get; set; }
}

fails with

Expectation has property subject.NonBrowsable that the other object does not have.

With configuration:
- Use declared types and members
- Compare enums by value
- Compare tuples by their properties
- Compare anonymous types by their properties
- Compare records by their members
- Exclude non-browsable members
- Match member by name (or throw)
- Be strict about the order of items in byte arrays
- Without automatic conversion.

In short, it fails on the missing NonBrowsable on subject before it sees that it can be ignored due to ExcludingNonBrowsableMembers().

The example above works if we instead implement this feature using an IMemberSelectionRule.
See jnyrup@476069d

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 27, 2022

That's a clever approach :-) I hadn't thought of this possibility at all, but it makes a lot of sense.

I'll try this alternate implementation against our test suites, and I anticipate cherry-picking your commit into this branch :-)

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 27, 2022

Hmm, as written it doesn't actually do everything that's needed, because it only excludes members if they're marked non-browsable in the expectation. If only the subject marks them non-browsable, then this isn't noticed because the filter only applies to members of the expectation.

With this implementation, tests When_field_is_non_browsable_only_in_subject_excluding_non_browsable_members_should_make_it_succeed and When_property_is_non_browsable_only_in_subject_excluding_non_browsable_members_should_make_it_succeed fail.

Is there a way to resolve this? Or is it fundamentally impossible to have a selection rule guide itself based on aspects of the subject?

@logiclrd
Copy link
Contributor Author

logiclrd commented Feb 27, 2022

I've done a more direct fix. We can backtrack if there is a more elegant solution.

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 6, 2022

I agree with with @dennisdoomen that only the members to select on the expectation should be affected.

Be aware that BeEquivalentTo is not meant to be a symmetrical assertion.

Okay, this makes sense, but, how, then, do I achieve my required use case, which is specifically that non-browsable members of the subject should be skipped?

@dennisdoomen
Copy link
Member

dennisdoomen commented Mar 6, 2022

Okay, this makes sense, but, how, then, do I achieve my required use case, which is specifically that non-browsable members of the subject should be skipped?

If the expectation doesn't contain that property or it is excluded, it should not even look for a property on the subject.

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 6, 2022

The expectation does contain the property. It is marked non-browsable on the subject only.

@dennisdoomen
Copy link
Member

dennisdoomen commented Mar 6, 2022

The expectation does contain the property. It is marked non-browsable on the subject only.

Ah, I didn't get that scenario. Maybe by having an additional option called TreatingNonBrowsableMembersAsMissing so that the default ThrowingOnMissingMembers will do what you had in mind.

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 6, 2022

Mm, so then ... TreatingNonBrowsableMembersAsMissing would make the logic pretend that the member straight up didn't exist on the subject, and if you wanted that situation to be a soft "ignore" then you simply don't call ThrowingOnMissingMembers? That could work, though, it does make it impossible to both ignore non-browsable members and throw a hard error if members are actually missing...

@dennisdoomen
Copy link
Member

dennisdoomen commented Mar 6, 2022

Mm, so then ... TreatingNonBrowsableMembersAsMissing would make the logic pretend that the member straight up didn't exist on the subject, and if you wanted that situation to be a soft "ignore" then you simply don't call ThrowingOnMissingMembers?

No, ThrowingOnMissingMembers is the default. But you can override that using ExcludingMissingMembers.

That could work, though, it does make it impossible to both ignore non-browsable members and throw a hard error if members are actually missing...

Sorry, I don't think I get that scenario. If you exclude a member from the expectation, than it's completely ignored.

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 10, 2022

How about we keep the functionality the way it's currently implemented, but rename it from .ExcludingNonBrowsableMembers, which by convention should only look at the expectation, to .DisregardingAllNonBrowsableMembers, which makes it clear that it isn't a part of the Including/Excluding world that only implicates the expectation?

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 15, 2022

@jnyrup @dennisdoomen How about my suggestion of simply changing the name so that this feature isn't implicitly expected to follow the same rules as other Including/Excluding methods?

@dennisdoomen
Copy link
Member

dennisdoomen commented Mar 16, 2022

It's kind of a niche feature (which nobody asked for in over 10 years), so either we do it correctly (using two options like TreatingNonBrowsableMembersAsMissing and ExcludingNonBrowsableMembers) or we don't. FYI, we've reverted the previous PR to keep develop consistent until we've agreed on a proper solution (which I just did).

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 16, 2022

That's fair. But, with TreatingNonBrowsableMembersAsMissing and ExcludingNonBrowsableMembers (on the expectation only), how do you exclude members from consideration that are non-browsable on the subject only??

@dennisdoomen
Copy link
Member

dennisdoomen commented Mar 16, 2022

TreatingNonBrowsableMembersAsMissing would only affect the subject.

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 16, 2022

Ah, okay, and the default logic doesn't care if the expectation has extra members? So then, with what you describe, those two methods are basically doing exactly the same thing, just one on the subject and one on the expectation.

The wording TreatingNonBrowsableMembersAsMissing doesn't feel "clear" to me -- at first glance, it seems to suggest that non-browsable members will be an error condition, and it isn't obvious that it's referring to the subject. What about IgnoringSubjectNonBrowsableMembers??

@dennisdoomen
Copy link
Member

dennisdoomen commented Mar 16, 2022

Ah, okay, and the default logic doesn't care if the expectation has extra members?

It does. The expectation defines the members that are expected.

What about IgnoringSubjectNonBrowsableMembers??

I chose TreatingNonBrowsableMembersAsMissing because it felt like a good alignment to ThrowingOnMissingMembers

But maybe IgnoringSubjectsNonBrowsableMembers (with the extra s) or IgnoringNonBrowsableMembersOnSubject would be better. I'm fine with both.

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 16, 2022

In previous discussion, the Including and Excluding methods were described as sort of "editors" that are used to build up the desired state in a stepwise manner. For instance, you ought to be able to do something like:

.ExcludingProperties()
.Including(e => e.SpecificProperty)

But, looking at the implementation, it doesn't seem to work that way. The various Excluding and Including methods aren't actually editing a list and building it up as they go, they're just setting flags that get processed later on down the line. Only the .Excluding and .Including members defined on the EquivalencyAssertionOptions subclass work by building up a sequence of changes to the state.

Furthermore, it appears that if you explicitly include any member, then the default inclusion of public properties & fields is disabled. So, it isn't possible to do something like:

.ExcludingNonBrowsableMembers()
.Including(e => e.ExceptForThisSpecificNonBrowsableMember)

The current logic sees the .Including and disables the default inclusion of members, and this can't be overridden even by explicitly trying to include them:

.IncludingProperties()
.ExcludingNonBrowsableMembers()
.Including(e => e.ExceptForThisSpecificNonBrowsableProperty)

...because the .IncludingProperties isn't actually a selection rule in the sequence, it's just a flag.

I think if we want this to work consistently and in an easy-to-understand way, then this needs to be reworked so that AllPropertiesSelectionRule and AllFieldsSelectionRule can be explicitly specified in the sequence of selections.

I'm inclined to instead make an IMemberSelectionRule that excludes non-browsable members, and return it explicitly prior to any custom selection rules. This is a smaller change to the code. But, it does mean that the logic continues to have special cases rather than being truly a generic sequence of edits.

@logiclrd
Copy link
Contributor Author

logiclrd commented Mar 16, 2022

I have rebased the changes here, collapsing the commits that were part of #1807 into a single commit, and the edits made in this PR into a single commit. The character of the changes changed sufficiently that I didn't see any value in preserving the edit history that went down a different path. With this rebase, this PR can be merged and will reintroduce the changes reverted in #1842 as a base onto which the new approach separating the subject from the expectation is applied.

@dennisdoomen
Copy link
Member

dennisdoomen commented Mar 16, 2022

I'm inclined to instead make an IMemberSelectionRule that excludes non-browsable members, and return it explicitly prior to any custom selection rules. This is a smaller change to the code. But, it does mean that the logic continues to have special cases rather than being truly a generic sequence of edits

I guess in the current state of the code base, there's no other way to make this work.

Copy link
Member

@dennisdoomen dennisdoomen left a comment

We're almost there ;-)

@logiclrd logiclrd changed the title Fix excluding non-browsable members when the subject and expectation are different types Add the ability to exclude non-browsable members from equivalency tests (take 2) Mar 18, 2022
@dennisdoomen dennisdoomen requested a review from jnyrup Mar 20, 2022
logiclrd added 4 commits Apr 15, 2022
…. Implemented it in SelfReferenceEquivalencyAssertionOptions.cs and CollectionMemberAssertionOptionsDecorator.cs and UsersOfGetClosedGenericInterfaces.cs.

Added property IsBrowsable to IMember.cs and implemented it in Field.cs and Property.cs.
Added benchmark CheckIfMemberIsBrowsable.
Adjusted the implementation of AssertMemberEquality in StructuralEqualityEquivalencyStep.cs to combine these fields to allow for non-browsable members to be skipped when checking equivalence.
Added automated tests of the new functionality to SelectionRulesSpec.cs.
Accepted API changes into the approved API.
Updated objectgraphs.md to document the new non-browsable "hidden" members exclusion feature.
Updated releases.md to describe the new feature.
…rate properties IgnoreNonBrowsableOnSubject and ExcludeNonBrowsableOnExpectation.

Updated implementations of the interface accordingly.
Subject: Added new selection rule ExcludeNonBrowsableMembersRule.cs and updated SelfReferenceEquivalencyAssertionOptions.SelectionRules to inject an instance of it after AllPropertiesSelectionRule and AllFieldsSelectionRule but before explicit member selection rules.
Expectation: Updated AssertMemberEquality in StructuralEqualityEquivalencyStep.cs to factor in whether the selected member is browsable.
Updated the ToString implementation in SelfReferenceEquivalencyAssertionOptions.cs to consider ignoreNonBrowsableOnSubject and excludeNonBrowsableOnExpectation independently.
Added tests exercising all combinations of non-browsable fields and properties in the subject and expectation.
Reran AcceptApiChanges.ps1.
- <see cref> in XML docs.
- Bug fix: MustMatchByNameRule.cs raises an error if it's supposed to be pretending that a member in the subject doesn't exist because of .IgnoringNonBrowsableMembersOnSubject.
- ExcludeNonBrowsableMembersRule.cs makes the list of selected members concrete before returning instead of returning the LINQ query.
- Removed the IncludingNonBrowsableMembers method from SelfReferenceEquivalencyAssertionOptions.cs.
- Removed redundant tests from SelectionRulesSpecs.cs.
- Removed redundant word from releases.md.
Typo fix in SelectionRulesSpecs.cs.
Reran AcceptApiChanges.ps1.
@logiclrd
Copy link
Contributor Author

logiclrd commented Apr 15, 2022

(rebased to resolve release.md conflict)

@jnyrup jnyrup changed the title Add the ability to exclude non-browsable members from equivalency tests (take 2) Add the ability to exclude non-browsable members from equivalency tests Apr 16, 2022
jnyrup
jnyrup approved these changes Apr 16, 2022
@jnyrup jnyrup added the feature label Apr 16, 2022
@jnyrup jnyrup merged commit e87aa94 into fluentassertions:develop Apr 16, 2022
1 check passed
@dennisdoomen
Copy link
Member

dennisdoomen commented Apr 16, 2022

Wow. That was a big and long-running PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants