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

Cleaner implementation of the equivalency validator #1026

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 3 additions & 1 deletion FluentAssertions.sln.DotSettings
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpKeepExistingMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpPlaceEmbeddedOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpRenamePlacementToArrangementMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ECSharpUseContinuousIndentInsideBracesMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EAddAccessorOwnerDeclarationBracesMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002ECSharpPlaceAttributeOnSameLineMigration/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/Environment/SettingsMigration/IsMigratorApplied/=JetBrains_002EReSharper_002EPsi_002ECSharp_002ECodeStyle_002ESettingsUpgrade_002EMigrateBlankLinesAroundFieldToBlankLinesAroundProperty/@EntryIndexedValue">True</s:Boolean>
Expand Down Expand Up @@ -106,4 +107,5 @@ public void When_$scenario$_it_should_$behavior$()&#xD;
<s:Boolean x:Key="/Default/PatternsAndTemplates/LiveTemplates/Template/=012E3B0572DEF2448B0B5D9AA88E6210/Applicability/=Live/@EntryIndexedValue">True</s:Boolean>
<s:Boolean x:Key="/Default/PatternsAndTemplates/LiveTemplates/Template/=012E3B0572DEF2448B0B5D9AA88E6210/Scope/=C3001E7C0DA78E4487072B7E050D86C5/@KeyIndexDefined">True</s:Boolean>
<s:String x:Key="/Default/PatternsAndTemplates/LiveTemplates/Template/=012E3B0572DEF2448B0B5D9AA88E6210/Scope/=C3001E7C0DA78E4487072B7E050D86C5/Type/@EntryValue">InCSharpFile</s:String>
<s:String x:Key="/Default/PatternsAndTemplates/LiveTemplates/Template/=012E3B0572DEF2448B0B5D9AA88E6210/Scope/=C3001E7C0DA78E4487072B7E050D86C5/CustomProperties/=minimumLanguageVersion/@EntryIndexedValue">2.0</s:String></wpf:ResourceDictionary>
<s:String x:Key="/Default/PatternsAndTemplates/LiveTemplates/Template/=012E3B0572DEF2448B0B5D9AA88E6210/Scope/=C3001E7C0DA78E4487072B7E050D86C5/CustomProperties/=minimumLanguageVersion/@EntryIndexedValue">2.0</s:String>
<s:Boolean x:Key="/Default/UserDictionary/Words/=Comparands/@EntryIndexedValue">True</s:Boolean></wpf:ResourceDictionary>
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ public virtual bool Handle(IEquivalencyValidationContext context, IEquivalencyVa
if (config.IsRecursive)
{
context.TraceSingle(path => $"Recursing into dictionary item {key} at {path}");
parent.AssertEqualityUsing(context.CreateForDictionaryItem(key, subject[key], expectation[key]));
parent.RecursivelyAssertEquality(context.CreateForDictionaryItem(key, subject[key], expectation[key]));
}
else
{
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,7 @@ private string[] TryToMatch<T>(object subject, T expectation, int expectationInd
{
using (var scope = new AssertionScope())
{
parent.AssertEqualityUsing(context.CreateForCollectionItem(expectationIndex.ToString(), subject, expectation));
parent.RecursivelyAssertEquality(context.CreateForCollectionItem(expectationIndex.ToString(), subject, expectation));

return scope.Discard();
}
Expand All @@ -200,7 +200,7 @@ private bool StrictlyMatchAgainst<T>(object[] subjects, T expectation, int expec
IEquivalencyValidationContext equivalencyValidationContext =
context.CreateForCollectionItem(indexString, subject, expectation);

parent.AssertEqualityUsing(equivalencyValidationContext);
parent.RecursivelyAssertEquality(equivalencyValidationContext);

bool failed = scope.HasFailures();
return !failed;
Expand Down
118 changes: 58 additions & 60 deletions Src/FluentAssertions/Equivalency/EquivalencyValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,10 @@ public void AssertEquality(EquivalencyValidationContext context)
using (var scope = new AssertionScope())
{
scope.AddReportable("configuration", config.ToString());
scope.AddNonReportable("objects", new CyclicReferenceDetector(config.CyclicReferenceHandling));

scope.BecauseOf(context.Because, context.BecauseArgs);

AssertEqualityUsing(context);
RecursivelyAssertEquality(context);

if (context.Tracer != null)
{
Expand All @@ -44,87 +43,86 @@ public void AssertEquality(EquivalencyValidationContext context)
}
}

private bool IsComplexType(object @object)
public void RecursivelyAssertEquality(IEquivalencyValidationContext context)
{
if (@object == null)
return false;
if (ShouldCompareMembersThisDeep(context.SelectedMemberPath))
{
UpdateScopeWithReportableContext(context);

Type type = @object.GetType();
if (!IsCyclicReference(context))
{
RunStepsUntilEquivalencyIsProven(context);
}
}
}

if (!isComplexTypeMap.TryGetValue(type, out bool isComplexType))
private bool ShouldCompareMembersThisDeep(string selectedMemberPath)
{
const char memberSeparator = '.';
var depth = selectedMemberPath.Count(chr => chr == memberSeparator);
bool shouldRecurse = config.AllowInfiniteRecursion || depth < MaxDepth;
dennisdoomen marked this conversation as resolved.
Show resolved Hide resolved

if (!shouldRecurse)
{
isComplexType = !type.OverridesEquals();
isComplexTypeMap[type] = isComplexType;
AssertionScope.Current.FailWith("The maximum recursion depth was reached. ");
}

return isComplexType;
return shouldRecurse;
dennisdoomen marked this conversation as resolved.
Show resolved Hide resolved
}

public void AssertEqualityUsing(IEquivalencyValidationContext context)
private static void UpdateScopeWithReportableContext(IEquivalencyValidationContext context)
{
if (ContinueRecursion(context.SelectedMemberPath))
if (context.SelectedMemberDescription.Length > 0)
{
AssertionScope scope = AssertionScope.Current;
scope.Context = (context.SelectedMemberDescription.Length == 0) ? scope.Context : context.SelectedMemberDescription;
scope.AddNonReportable("subject", context.Subject);
scope.AddNonReportable("expectation", context.Expectation);

var objectTracker = scope.Get<CyclicReferenceDetector>("objects");
AssertionScope.Current.Context = context.SelectedMemberDescription;
}

bool isComplexType = IsComplexType(context.Expectation);
var objectReference = new ObjectReference(context.Expectation, context.SelectedMemberPath, isComplexType);
AssertionScope.Current.TrackComparands(context.Subject, context.Expectation);
}

if (!objectTracker.IsCyclicReference(objectReference))
{
bool wasHandled = false;

foreach (var step in AssertionOptions.EquivalencySteps)
{
if (step.CanHandle(context, config))
{
if (step.Handle(context, this, config))
{
wasHandled = true;
break;
}
}
}

if (!wasHandled)
{
Execute.Assertion.FailWith(
"No IEquivalencyStep was found to handle the context. " +
"This is likely a bug in Fluent Assertions.");
}
}
private bool IsCyclicReference(IEquivalencyValidationContext context)
{
var objectTracker = AssertionScope.Current.Get<CyclicReferenceDetector>("cyclic_reference_detector");
if (objectTracker is null)
{
objectTracker = new CyclicReferenceDetector(config.CyclicReferenceHandling);
AssertionScope.Current.AddNonReportable("cyclic_reference_detector", objectTracker);
}

bool isComplexType = IsComplexType(context.Expectation);

return objectTracker.IsCyclicReference(new ObjectReference(context.Expectation, context.SelectedMemberPath, isComplexType));
}

private bool ContinueRecursion(string memberAccessPath)
private bool IsComplexType(object expectation)
{
if (config.AllowInfiniteRecursion || !HasReachedMaximumRecursionDepth(memberAccessPath))
if (expectation is null)
{
return true;
return false;
}

AssertionScope.Current.FailWith(
"The maximum recursion depth was reached. " +
"The maximum recursion depth limitation prevents stack overflow from " +
"occurring when certain types of cycles exist in the object graph " +
"or the object graph's depth is very high or infinite. " +
"This limitation may be disabled using the config parameter." +
Environment.NewLine + Environment.NewLine +
"The member access chain when max depth was hit was: " +
memberAccessPath);

return false;
Type type = expectation.GetType();

if (!isComplexTypeMap.TryGetValue(type, out bool isComplexType))
{
isComplexType = !type.OverridesEquals();
isComplexTypeMap[type] = isComplexType;
}

return isComplexType;
}

private static bool HasReachedMaximumRecursionDepth(string propertyPath)
private void RunStepsUntilEquivalencyIsProven(IEquivalencyValidationContext context)
{
int depth = propertyPath.Count(chr => chr == '.');
foreach (var step in AssertionOptions.EquivalencySteps)
{
if (step.CanHandle(context, config) && step.Handle(context, this, config))
{
return;
dennisdoomen marked this conversation as resolved.
Show resolved Hide resolved
}
}

return (depth >= MaxDepth);
throw new NotImplementedException($"No {nameof(IEquivalencyStep)} was found to handle the context. ");
dennisdoomen marked this conversation as resolved.
Show resolved Hide resolved
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ private static Type GetIDictionaryInterface(Type expectedType)
{
if (config.IsRecursive)
{
parent.AssertEqualityUsing(context.CreateForDictionaryItem(key, subjectValue, expectation[key]));
parent.RecursivelyAssertEquality(context.CreateForDictionaryItem(key, subjectValue, expectation[key]));
}
else
{
Expand Down
2 changes: 1 addition & 1 deletion Src/FluentAssertions/Equivalency/IEquivalencyValidator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,6 @@ namespace FluentAssertions.Equivalency
{
public interface IEquivalencyValidator
{
void AssertEqualityUsing(IEquivalencyValidationContext context);
void RecursivelyAssertEquality(IEquivalencyValidationContext context);
Copy link
Member

Choose a reason for hiding this comment

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

@dennisdoomen renaming this is a breaking change and should be reverted.

Copy link
Member Author

Choose a reason for hiding this comment

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

How so?

Copy link
Member

@jnyrup jnyrup May 3, 2019

Choose a reason for hiding this comment

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

From the CoreFX Breaking Change Rules

Removing or renaming a member, including a getter or setter from a property or enum members

Though I can't find any code on the internet that implements IEquivalencyValidator except EquivalencyValidator, Every change breaks someone's workflow and last time we thought it didn't matter, it turned out bad.
I think the benefit of renaming a public method versus the risk of introducing a breaking change, does not seem justified.

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ public bool CanHandle(IEquivalencyValidationContext context, IEquivalencyAsserti
subject,
expectation);

parent.AssertEqualityUsing(itemContext);
parent.RecursivelyAssertEquality(itemContext);
}
while (digit.Increment());
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ private static void AssertMemberEquality(IEquivalencyValidationContext context,

if (nestedContext != null)
{
parent.AssertEqualityUsing(nestedContext);
parent.RecursivelyAssertEquality(nestedContext);
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion Src/FluentAssertions/Equivalency/TryConversionStep.cs
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ public bool CanHandle(IEquivalencyValidationContext context, IEquivalencyAsserti

var newContext = context.CreateWithDifferentSubject(convertedSubject, expectationType);

structuralEqualityValidator.AssertEqualityUsing(newContext);
structuralEqualityValidator.RecursivelyAssertEquality(newContext);
return true;
}

Expand Down
23 changes: 16 additions & 7 deletions Src/FluentAssertions/Execution/AssertionScope.cs
Original file line number Diff line number Diff line change
@@ -1,16 +1,16 @@
#region

using System;
using System;
using System.Linq;
using FluentAssertions.Common;

#endregion

namespace FluentAssertions.Execution
{
/// <summary>
/// Represents an implicit or explicit scope within which multiple assertions can be collected.
/// </summary>
/// <remarks>
/// This class is supposed to have a very short life time and is not safe to be used in assertion that cross thread-boundaries such as when
/// using <c>async</c> or <c>await</c>.
/// </remarks>
public class AssertionScope : IAssertionScope
{
#region Private Definitions
Expand Down Expand Up @@ -140,6 +140,12 @@ public AssertionScope WithExpectation(string message, params object[] args)
return this;
}

internal void TrackComparands(object subject, object expectation)
{
contextData.Add(new ContextDataItems.DataItem("subject", subject, reportable: false, requiresFormatting: true));
contextData.Add(new ContextDataItems.DataItem("expectation", expectation, reportable: false, requiresFormatting: true));
}

public Continuation ClearExpectation()
{
expectation = null;
Expand Down Expand Up @@ -212,9 +218,12 @@ public void AddPreFormattedFailure(string formattedFailureMessage)
assertionStrategy.HandleFailure(formattedFailureMessage);
}

/// <summary>
/// Tracks a keyed object in the current scope that is excluded from the failure message in case an assertion fails.
/// </summary>
public void AddNonReportable(string key, object value)
{
contextData.Add(key, value, Reportability.Hidden);
contextData.Add(new ContextDataItems.DataItem(key, value, reportable: false, requiresFormatting: false));
}

/// <summary>
Expand All @@ -223,7 +232,7 @@ public void AddNonReportable(string key, object value)
/// </summary>
public void AddReportable(string key, string value)
{
contextData.Add(key, value, Reportability.Reportable);
contextData.Add(new ContextDataItems.DataItem(key, value, reportable: true, requiresFormatting: false));
}

public string[] Discard()
Expand Down
27 changes: 11 additions & 16 deletions Src/FluentAssertions/Execution/ContextDataItems.cs
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
using System.Collections.Generic;
using System.Linq;

using FluentAssertions.Formatting;

namespace FluentAssertions.Execution
Expand All @@ -22,7 +21,7 @@ public string AsStringOrDefault(string key)
DataItem item = items.SingleOrDefault(i => i.Key == key);
if (item != null)
{
if ((key == "subject") || (key == "expectation"))
if (item.RequiresFormatting)
{
return Formatter.ToString(item.Value);
}
Expand All @@ -41,12 +40,7 @@ public void Add(ContextDataItems contextDataItems)
}
}

public void Add(string key, object value, Reportability reportability)
{
Add(new DataItem(key, value, reportability));
}

private void Add(DataItem item)
public void Add(DataItem item)
{
var existingItem = items.SingleOrDefault(i => i.Key == item.Key);
if (existingItem != null)
Expand All @@ -65,25 +59,26 @@ public T Get<T>(string key)

internal class DataItem
{
private readonly Reportability reportability;

public DataItem(string key, object value, Reportability reportability)
public DataItem(string key, object value, bool reportable, bool requiresFormatting)
{
Key = key;
Value = value;
this.reportability = reportability;
Reportable = reportable;
RequiresFormatting = requiresFormatting;
}

public string Key { get; private set; }
public string Key { get; }

public object Value { get; }

public object Value { get; private set; }
public bool Reportable { get; }

public bool Reportable => reportability == Reportability.Reportable;
public bool RequiresFormatting { get; }

public DataItem Clone()
{
object value = (Value is ICloneable2 cloneable) ? cloneable.Clone() : Value;
return new DataItem(Key, value, reportability);
return new DataItem(Key, value, Reportable, RequiresFormatting);
}
}
}
Expand Down
11 changes: 0 additions & 11 deletions Src/FluentAssertions/Execution/Reportability.cs

This file was deleted.