Fail with a descriptive error when path-based rules are used on value-semantic types#3187
Open
dennisdoomen wants to merge 4 commits intomainfrom
Open
Fail with a descriptive error when path-based rules are used on value-semantic types#3187dennisdoomen wants to merge 4 commits intomainfrom
dennisdoomen wants to merge 4 commits intomainfrom
Conversation
…-semantic types When Excluding() or Including() targets a member of a type that overrides Equals(), the rule is silently ignored because BeEquivalentTo() compares by value and never traverses members. This commit detects that conflict at comparison time and fails with a descriptive assertion error that tells the user to call ComparingByMembers<T>() or remove the selection rule. Fixes #2571 Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
Test Results 30 files - 7 30 suites - 7 5m 8s ⏱️ - 1m 41s For more details on these failures, see this check. Results for commit ad9541e. ± Comparison against base commit 27bd2f9. This pull request removes 14 and adds 15 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
Pull Request Test Coverage Report for Build 23692926007Details
💛 - Coveralls |
There was a problem hiding this comment.
Pull request overview
This PR addresses surprising behavior in BeEquivalentTo() where path-based selection rules (Excluding() / Including()) can be silently ignored when a type is auto-compared by value semantics (EqualityStrategy.Equals). It introduces conflict detection to fail fast with a descriptive assertion message.
Changes:
- Detect and report conflicts between auto-detected value-semantics comparison and path-based selection rules during equivalency comparison.
- Add
SelectsMembersOf(INode)to path-based selection rules to encapsulate overlap detection logic. - Add/extend selection rule specs and document the behavior change in the release notes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/_pages/releases.md | Adds a release note describing the new fail-fast behavior for conflicting path rules + value semantics. |
| Tests/FluentAssertions.Equivalency.Specs/SelectionRulesSpecs.Including.cs | Adds a test asserting Including() fails with a descriptive message on value-semantic types. |
| Tests/FluentAssertions.Equivalency.Specs/SelectionRulesSpecs.Excluding.cs | Adds tests asserting Excluding() fails (and explicit overrides do/don’t fail). |
| Src/FluentAssertions/Equivalency/Steps/ValueTypeEquivalencyStep.cs | Adds conflict detection and emits a dedicated assertion failure instead of silently ignoring path rules. |
| Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs | Implements overlap detection (SelectsMembersOf) and normalizes current node paths. |
| Src/FluentAssertions/Equivalency/Selection/IncludeMemberByPathSelectionRule.cs | Refactors to expose the rule MemberPath via the base class for conflict detection. |
| Src/FluentAssertions/Equivalency/Selection/ExcludeMemberByPathSelectionRule.cs | Refactors to expose the rule MemberPath via the base class for conflict detection. |
Comment on lines
+38
to
+40
| return rulePath.StartsWith(currentPath + ".", StringComparison.Ordinal) | ||
| || rulePath.StartsWith(currentPath + "[", StringComparison.Ordinal); | ||
| } |
There was a problem hiding this comment.
SelectsMembersOf uses StartsWith(currentPath + "."/"[") against currentNode.Expectation.PathAndName. This won’t match when the node path contains specific indices (e.g. Children[0]) but the rule path comes from NestedExclusionOptionBuilder/MemberPath.AsParentCollectionOf and uses [] wildcards (e.g. Children[]Text). That can prevent conflicts from being detected for value-semantic types inside collections (non-root). Consider matching via MemberPath segment logic (or normalizing both paths to ignore specific indices and handle the [] separator convention) instead of raw string prefix checks.
Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs
Show resolved
Hide resolved
Src/FluentAssertions/Equivalency/Selection/SelectMemberByPathSelectionRule.cs
Show resolved
Hide resolved
Comment on lines
+70
to
+76
| AssertionChain.GetOrCreate() | ||
| .For(context) | ||
| .FailWith( | ||
| $"{expectationType.Name} is compared by value (because it overrides Equals), " + | ||
| $"so the {conflictingRule} selection rule does not apply. " + | ||
| $"Either call ComparingByMembers<{expectationType.Name}>() to force member-wise comparison, " + | ||
| $"or remove the selection rule."); |
There was a problem hiding this comment.
The conflict failure message hardcodes expectationType.Name and doesn’t include {reason} / the caller-provided because text. Also, Type.Name will render generics as Foo1, making the suggested ComparingByMembers<Foo1>() invalid C#. Consider including {reason} and formatting the type in a compilable way (e.g., recommending the ComparingByMembers(Type) overload for generics).
Suggested change
| AssertionChain.GetOrCreate() | |
| .For(context) | |
| .FailWith( | |
| $"{expectationType.Name} is compared by value (because it overrides Equals), " + | |
| $"so the {conflictingRule} selection rule does not apply. " + | |
| $"Either call ComparingByMembers<{expectationType.Name}>() to force member-wise comparison, " + | |
| $"or remove the selection rule."); | |
| var reason = context.Reason; | |
| bool isGeneric = expectationType.IsGenericType; | |
| string suggestion = isGeneric | |
| ? "Either call the ComparingByMembers(Type) overload to force member-wise comparison, " | |
| : $"Either call ComparingByMembers<{expectationType.Name}>() to force member-wise comparison, "; | |
| string message = | |
| $"{expectationType} is compared by value (because it overrides Equals), " + | |
| $"so the {conflictingRule} selection rule does not apply. " + | |
| suggestion + | |
| "or remove the selection rule." + | |
| reason.FormattedMessage; | |
| AssertionChain.GetOrCreate() | |
| .For(context) | |
| .FailWith(message, reason.Arguments); |
Comment on lines
+1263
to
+1267
| [Fact] | ||
| public void Excluding_a_member_by_path_on_a_type_with_value_semantics_should_fail_with_a_descriptive_error() | ||
| { | ||
| // Arrange | ||
| var actual = new ClassWithValueSemanticsOnSingleProperty { Key = "same", NestedProperty = "x" }; |
There was a problem hiding this comment.
The PR description mentions coverage for “Collection of value-semantic types — exclusion correctly detected per item”, but there’s no test here exercising a collection member path built via .For(...).Exclude(...) (which uses [] wildcards in MemberPath). Adding that test would both align with the stated behavior and guard against regressions in SelectsMembersOf for indexed paths.
Extend the value-semantics conflict detection to cover non-path selection rules (Excluding(m => predicate), Including(m => predicate), Excluding<T>()). - Path-based rules are still detected cheaply via SelectsMembersOf (handles deep paths like o.Child.Text without reflection) - Non-path rules are detected by running them against the actual member list: exclusion rules are applied to the full member set; inclusion rules are applied from an empty seed and compared to the full member set - Infrastructure rules (AllPropertiesSelectionRule, AllFieldsSelectionRule, ExcludeNonBrowsableMembersRule) are skipped as they are not user-specified Co-authored-by: Copilot <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #2571
When
Excluding()orIncluding()targets a member of a type that overridesEquals(), the rule is silently ignored becauseBeEquivalentTo()compares the whole object by value and never traverses its members. This is surprising and hard to debug.Changes
This PR detects the conflict at comparison time and fails with a descriptive assertion error:
All user-specified selection rule types are detected:
Excluding(o => o.Prop)o.Child.Text)Including(o => o.Prop)Excluding(m => m.Name == "Etag")Including(m => m.Name == "Id")Excluding<Guid>()Design decisions
EqualityStrategy.Equals). Explicitly callingComparingByValue<T>()(ForceEquals) is intentional and does not trigger the error.AllPropertiesSelectionRule,AllFieldsSelectionRule,ExcludeNonBrowsableMembersRule) are skipped since they are not user-specified selections.SelectMemberByPathSelectionRule.SelectsMembersOf(INode)(no reflection needed). Non-path rules are evaluated by running them against the member list of the current node's type.Tests
Seven new tests covering:
o.Child.Text)m.Name == "...")m.Name == "...")ComparingByValue<T>()— no errorComparingByMembers<T>()— exclusion works normally