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

logiclrd · 2022-02-15T05:41:04Z

This PR:

Adds a property ExcludeNonBrowsable to IEquivalencyAssertionOptions, and implements it in SelfReferenceEquivalencyAssertionOptions, CollectionMemberAssertionOptionsDecorator and UsersOfGetClosedGenericInterfaces.
Adds a property IsBrowsable to IMember and implements it in Field and Property.
Uses the new properties in AssertMemberEquality in StructuralEqualityEquivalencyStep to allow for non-browsable members to be skipped when checking equivalence.
Adds automated tests of the new functionality to SelectionRulesSpec.
Accepts API changes into the approved API.
Updates documentation and release notes correspondingly.

IMPORTANT

The code complies with the Coding Guidelines for C#.
The changes are covered by unit tests which follow the Arrange-Act-Assert syntax and the naming conventions such as is used in these tests.
If the PR adds a feature or fixes a bug, please update the release notes with a functional description that explains what the change means to consumers of this library, which are published on the website.
If the PR changes the public API the changes needs to be included by running AcceptApiChanges.ps1 or AcceptApiChanges.sh.
If the PR affects the documentation, please include your changes in this pull request so the documentation will appear on the website.

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

…mbers exclusion feature. Updated releases.md to describe the new feature.

Src/FluentAssertions/Equivalency/Field.cs

Src/FluentAssertions/Equivalency/IEquivalencyAssertionOptions.cs

Src/FluentAssertions/Equivalency/SelfReferenceEquivalencyAssertionOptions.cs

dennisdoomen · 2022-02-15T16:50:06Z

Tests/FluentAssertions.Equivalency.Specs/SelectionRulesSpecs.cs

@@ -1489,5 +1490,89 @@ public void Including_an_interface_property_through_inheritance_should_work()
                .Including(a => a.Value2)
                .RespectingRuntimeTypes());
        }
+
+        [Theory]
+        [MemberData(nameof(TestMembers))]


🤔 I found it very hard to understand what this is doing with the parameter. Because of that, I strongly believe it is much better to drop the parameterized tests and have individual tests for the relevant test cases.

Oh? This is just how Xunit works. Maybe this can be improved with comments and/or better naming??

I think we can continue using parameterized tests, but without using ApplyDifference() to mutate b.

public static object[][] TestMembers => new object[][] { new[] { new ClassWithNonBrowsableMembers { BrowsableField = 1 } }, new[] { new ClassWithNonBrowsableMembers { BrowsableProperty = 1 } }, new[] { new ClassWithNonBrowsableMembers { AdvancedBrowsableField = 1 } }, new[] { new ClassWithNonBrowsableMembers { AdvancedBrowsableProperty = 1 } }, new[] { new ClassWithNonBrowsableMembers { NonBrowsableField = 1 } }, new[] { new ClassWithNonBrowsableMembers { NonBrowsableProperty = 1 } } }; [Theory] [MemberData(nameof(TestMembers))] public void Including_non_browsable_members_should_work(ClassWithNonBrowsableMembers b) { ... } public class ClassWithNonBrowsableMembers { public int BrowsableField; public int BrowsableProperty { get; set; } [EditorBrowsable(EditorBrowsableState.Advanced)] public int AdvancedBrowsableField; [EditorBrowsable(EditorBrowsableState.Advanced)] public int AdvancedBrowsableProperty { get; set; } [EditorBrowsable(EditorBrowsableState.Never)] public int NonBrowsableField; [EditorBrowsable(EditorBrowsableState.Never)] public int NonBrowsableProperty { get; set; } }

I strongly disagree. Parameterized tests are only useful if you have a simple list of values that you use in combination with a Theory attribute. For anything else, they obscure the cause and effect of the test. In this case, having 6 different tests would be much clearer.

Oh dear :-P Which way should I take it?

Interesting 🙃 I prefer the parameterized one to this, as I don't have to jump between method bodies.

Conditionals in tests is a (99.9%) no-go for me.
For parameterized tests I would create separate test case generators to avoid that.

Seems Dennis and I can agree on spelling out the entire test cases.

Okay, conditionals are gone. :-)

Sorry, was trying to understand each test, but still had to dig through the private methods to understand what each test was doing. I really want them to be spelled out and have functional names.

I have pushed a commit that gives the methods in question much more explicit names. Is that what you had in mind? :-)

Unfortunately not. In fact, it makes it worse since you refer to the literal names of the APIs, which is an anti-pattern for me. What I meant is to drop the parameterized tests completely.

Src/FluentAssertions/Equivalency/Field.cs

jnyrup · 2022-02-15T23:55:58Z

Tests/FluentAssertions.Equivalency.Specs/SelectionRulesSpecs.cs

@@ -1489,5 +1490,89 @@ public void Including_an_interface_property_through_inheritance_should_work()
                .Including(a => a.Value2)
                .RespectingRuntimeTypes());
        }
+
+        [Theory]
+        [MemberData(nameof(TestMembers))]


I think we can continue using parameterized tests, but without using ApplyDifference() to mutate b.

public static object[][] TestMembers => new object[][] { new[] { new ClassWithNonBrowsableMembers { BrowsableField = 1 } }, new[] { new ClassWithNonBrowsableMembers { BrowsableProperty = 1 } }, new[] { new ClassWithNonBrowsableMembers { AdvancedBrowsableField = 1 } }, new[] { new ClassWithNonBrowsableMembers { AdvancedBrowsableProperty = 1 } }, new[] { new ClassWithNonBrowsableMembers { NonBrowsableField = 1 } }, new[] { new ClassWithNonBrowsableMembers { NonBrowsableProperty = 1 } } }; [Theory] [MemberData(nameof(TestMembers))] public void Including_non_browsable_members_should_work(ClassWithNonBrowsableMembers b) { ... } public class ClassWithNonBrowsableMembers { public int BrowsableField; public int BrowsableProperty { get; set; } [EditorBrowsable(EditorBrowsableState.Advanced)] public int AdvancedBrowsableField; [EditorBrowsable(EditorBrowsableState.Advanced)] public int AdvancedBrowsableProperty { get; set; } [EditorBrowsable(EditorBrowsableState.Never)] public int NonBrowsableField; [EditorBrowsable(EditorBrowsableState.Never)] public int NonBrowsableProperty { get; set; } }

Tests/Approval.Tests/ApprovedApi/FluentAssertions/net47.verified.txt

Src/FluentAssertions/Equivalency/Steps/StructuralEqualityEquivalencyStep.cs

Swapped the order of operands in StructuralEqualityEquivalencyStep so that it only queries matchingMember.IsBrowsable if it could make a difference. Updated the implementations of IsBrowsable in Field.cs and Property.cs to use lazy evaluation, and to use pattern matching to be more concise and readable.

…n while also avoiding conditionals.

…sted.

dennisdoomen · 2022-02-18T12:43:45Z

Tests/FluentAssertions.Equivalency.Specs/SelectionRulesSpecs.cs

@@ -1493,86 +1493,131 @@ public void Including_an_interface_property_through_inheritance_should_work()

        [Fact]
        public void Including_non_browsable_members_when_testing_NotEquivalent_should_work_when_browsable_field_differs()


Almost ready...

We need to avoid to use the literal name of APIs like NotBeEquivalentTo and BeEquivalentTo. In this case, I would focus the scenarios using BeEquivalentTo because that's what NotBeEquivalentTo internally uses. E.g. Browsable_fields_can_be_included_in_the_assertion

Also, I would explicitly show the value of BrowsableField of the subject.

Also applies on the other scenarios.

What do you mean by

explicitly show the value of BrowsableField of the subject

?? The test should write it to test output??

[Fact] public void When_browsable_field_differs_including_non_browsable_members_should_not_affect_result() { // Arrange var subject = new ClassWithNonBrowsableMembers() { BrowsableField = 2 }; var expectation = new ClassWithNonBrowsableMembers() { BrowsableField = 1 }; // Act Action action = () => subject.Should().BeEquivalentTo(expectation, config => config.IncludingNonBrowsableMembers()); // Assert action.Should().Throw<XunitException>(); }

Ahh, okay, don't use the implicit value of 0. Got it :-)

I've pushed a commit to this effect. It explicitly assigns to the member where a difference is being set up for both the subject and the expectation now.

Ahh, okay, don't use the implicit value of 0. Got it :-)

Exactly, because you don't know that if you look at the test. In other words, if it's important to understand the test case, it's very important to show it ;-)

@dennisdoomen

…implementation. Reworked tests of IncludingNonBrowsableMembers and ExcludingNonBrowsableMembers in SelectionRulesSpecs to all test BeEquivalentTo, and renamed them per input from @dennisdoomen. Updated tests of IncludingNonBrowsableMembers and ExcludingNonBrowsableMembers to not just rely on default values of any of the members being tested.

…citly marked as being always browsable, in addition to testing members that are in this state by default.

Src/FluentAssertions/Equivalency/Property.cs

Tests/FluentAssertions.Equivalency.Specs/SelectionRulesSpecs.cs

…and Property.cs. Corrected a number of tests in SelectionRulesSpec.cs that used the wrong configuration method for the test.

Tests/Approval.Tests/ApprovedApi/FluentAssertions/net47.verified.txt

logiclrd · 2022-02-18T19:01:25Z

The automated PR build failed. It looks like a spurious failure to me. I don't seem to have access to the "Re-run" functionality to make it give it another go.

logiclrd · 2022-02-18T19:02:51Z

..but a merge conflict on releases.md provides an excuse for a new commit :-)

coveralls · 2022-02-18T19:12:01Z

Pull Request Test Coverage Report for Build 1866102890

14 of 26 (53.85%) changed or added relevant lines in 5 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage decreased (-0.1%) to 76.42%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
Src/FluentAssertions/Equivalency/Steps/StructuralEqualityEquivalencyStep.cs	9	10	90.0%
Src/FluentAssertions/Equivalency/Field.cs	0	3	0.0%
Src/FluentAssertions/Equivalency/Property.cs	0	3	0.0%
Src/FluentAssertions/Equivalency/SelfReferenceEquivalencyAssertionOptions.cs	4	9	44.44%

Totals
Change from base Build 1866009228:	-0.1%
Covered Lines:	6455
Relevant Lines:	8361

💛 - Coveralls

dennisdoomen · 2022-02-18T19:31:39Z

Thanks for the great work.

…ency tests (#1807)" This reverts commit 391519b.

…ency tests (fluentassertions#1807)" This reverts commit 391519b.

logiclrd added 2 commits February 14, 2022 23:34

Updated objectgraphs.md to document the new non-browsable "hidden" me…

0dc9852

…mbers exclusion feature. Updated releases.md to describe the new feature.

dennisdoomen requested changes Feb 15, 2022

View reviewed changes

logiclrd changed the title ~~Add the ability to exclude non-browsabe members from equivalency tests~~ Add the ability to exclude non-browsable members from equivalency tests Feb 15, 2022

jnyrup reviewed Feb 16, 2022

View reviewed changes

logiclrd added 2 commits February 16, 2022 08:59

Reworked tests in SelectionRulesSpecs.cs to eliminate parameterizatio…

0609df1

…n while also avoiding conditionals.

logiclrd mentioned this pull request Feb 16, 2022

Restore basic assertions for collections in System.Data #1812

Merged

5 tasks

Renamed tests in SelectionRulesSpecs.cs to spell out what is being te…

d473c3c

…sted.

dennisdoomen requested changes Feb 18, 2022

View reviewed changes

logiclrd force-pushed the JDG_ExcludingNonBrowsable branch 2 times, most recently from fc796b2 to fb9f459 Compare February 18, 2022 16:18

logiclrd force-pushed the JDG_ExcludingNonBrowsable branch from fb9f459 to 3dd47f0 Compare February 18, 2022 17:29

Added test cases to SelectionRulesSpecs.cs for members that are expli…

34cae26

…citly marked as being always browsable, in addition to testing members that are in this state by default.

jnyrup reviewed Feb 18, 2022

View reviewed changes

Src/FluentAssertions/Equivalency/Property.cs Outdated Show resolved Hide resolved

Tests/FluentAssertions.Equivalency.Specs/SelectionRulesSpecs.cs Show resolved Hide resolved

Switched to a lightweight lazy evaluation implementation in Field.cs …

e0d4fb4

…and Property.cs. Corrected a number of tests in SelectionRulesSpec.cs that used the wrong configuration method for the test.

dennisdoomen approved these changes Feb 18, 2022

View reviewed changes

jnyrup reviewed Feb 18, 2022

View reviewed changes

Tests/Approval.Tests/ApprovedApi/FluentAssertions/net47.verified.txt Show resolved Hide resolved

Merge branch 'develop' into JDG_ExcludingNonBrowsable

c9d5974

jnyrup approved these changes Feb 18, 2022

View reviewed changes

jnyrup merged commit 391519b into fluentassertions:develop Feb 18, 2022

jnyrup added the feature label Feb 19, 2022

logiclrd mentioned this pull request Feb 25, 2022

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

Merged

jnyrup added a commit that referenced this pull request Mar 10, 2022

Revert "Add the ability to exclude non-browsable members from equival…

622ba79

…ency tests (#1807)" This reverts commit 391519b.

jnyrup mentioned this pull request Mar 10, 2022

Revert "Add the ability to exclude non-browsable members from equivalency tests" #1842

Merged

jnyrup added a commit to jnyrup/fluentassertions that referenced this pull request Mar 10, 2022

Revert "Add the ability to exclude non-browsable members from equival…

9c2cc12

…ency tests (fluentassertions#1807)" This reverts commit 391519b.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

logiclrd commented Feb 15, 2022 •

edited

dennisdoomen Feb 15, 2022

logiclrd Feb 15, 2022

jnyrup Feb 15, 2022 •

edited

dennisdoomen Feb 16, 2022

logiclrd Feb 16, 2022

jnyrup Feb 16, 2022

logiclrd Feb 16, 2022

dennisdoomen Feb 18, 2022

logiclrd Feb 18, 2022

dennisdoomen Feb 18, 2022

jnyrup Feb 15, 2022 •

edited

dennisdoomen Feb 18, 2022

logiclrd Feb 18, 2022

dennisdoomen Feb 18, 2022

logiclrd Feb 18, 2022

logiclrd Feb 18, 2022 •

edited

dennisdoomen Feb 18, 2022

logiclrd commented Feb 18, 2022 •

edited

logiclrd commented Feb 18, 2022

coveralls commented Feb 18, 2022

dennisdoomen commented Feb 18, 2022

		@@ -1493,86 +1493,131 @@ public void Including_an_interface_property_through_inheritance_should_work()

		[Fact]
		public void Including_non_browsable_members_when_testing_NotEquivalent_should_work_when_browsable_field_differs()

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

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

Conversation

logiclrd commented Feb 15, 2022 • edited

IMPORTANT

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnyrup Feb 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnyrup Feb 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logiclrd Feb 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logiclrd commented Feb 18, 2022 • edited

logiclrd commented Feb 18, 2022

coveralls commented Feb 18, 2022

Pull Request Test Coverage Report for Build 1866102890

💛 - Coveralls

dennisdoomen commented Feb 18, 2022

logiclrd commented Feb 15, 2022 •

edited

jnyrup Feb 15, 2022 •

edited

jnyrup Feb 15, 2022 •

edited

logiclrd Feb 18, 2022 •

edited

logiclrd commented Feb 18, 2022 •

edited