Pull Request Test Coverage Report for Build 1992359664

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

• For more information on this, see Tracking coverage changes with pull request builds.
• To avoid this issue with future PRs, see these Recommended CI Configurations.
• For a quick fix, rebase this PR at GitHub. Your next report should be accurate.

Details

• 30 of 31 (96.77%) changed or added relevant lines in 7 files are covered.
• 3 unchanged lines in 1 file lost coverage.
• Overall coverage increased (+1.2%) to 96.002%

Totals
Change from base Build 1982264675:	1.2%
Covered Lines:	11739
Relevant Lines:	12066

💛 - Coveralls

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

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 :-)

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?

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

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?

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.

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

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.

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...

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.

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?

@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?

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).

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??

TreatingNonBrowsableMembersAsMissing would only affect the subject.

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??

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.

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.

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.

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.