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.

Sign up for GitHub

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

Jump to bottom

Order sensitive overload for compare to set #778

Merged
merged 6 commits into from
May 18, 2017
Merged

Order sensitive overload for compare to set #778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8f48b6a
Add tests
gasparnagy Jan 31, 2017
7b6e9ad
Refactor TableDifferenceResults to be able to represent order sensiti…
gasparnagy Jan 31, 2017
67e7846
make throw tests pass
gasparnagy Jan 31, 2017
2052252
refactor message tests to be able to test order sensitive comparison
gasparnagy Jan 31, 2017
2796780
fix diff messages for order sensitive
gasparnagy Jan 31, 2017
3fcdf09
Replace Tuple with TableDifferenceItem
gasparnagy Jan 31, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
50 changes: 40 additions & 10 deletions TechTalk.SpecFlow/Assist/SetComparer.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ public class SetComparer<T>
{
private const int MatchNotFound = -1;
private readonly Table table;
private List<T> actualItems;
private List<TableDifferenceItem<T>> extraOrNonMatchingActualItems;
Copy link
Contributor

Choose a reason for hiding this comment

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

I saw this pull request earlier, and I swear I saw you using a Tuple? I thought that was interesting, as I'm finding tuples more and more useful for internal behaviors in my own code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. It was this commit: 2796780
I was also wanted to see how Tuples could be better used, but altogether I did not really like the result. It was very hard to communicate with the code what data I am combining together (in my case the item with the expected position). So I refactored it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, my tuples regularly get refactored to types as well. It's nice to have them as an option, though, because in the middle of working I don't always want to load my brain with more types. I'd rather establish the flow of the code and then refactor it afterwards.

I think the tuples make more sense in other languages that make unpacking them simpler.

private readonly ITableDiffExceptionBuilder<T> tableDiffExceptionBuilder;

public SetComparer(Table table)
Expand All @@ -25,19 +25,24 @@ private SafetyTableDiffExceptionBuilder<T> BuildTheTableDiffExceptionBuilder()
return makeSureTheFormattingWorkAboveDoesNotResultInABadException;
}

public void CompareToSet(IEnumerable<T> set)
public void CompareToSet(IEnumerable<T> set, bool sequentialEquality)
Copy link
Contributor

Choose a reason for hiding this comment

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

I am a little concerned about exposing a boolean as a trailing argument for any method. Not just in this case, but all cases... it's a red flag. Because you know how it goes, you make this change with a boolean, then another change comes in... what do you do? Tack on more flags?

Another option might be to take in an options class, for those concerned with types, with an override that takes a dynamic object. So the syntax could be as simple as checker.CompareToSet(set, new { CheckTheSequence = true })

Allowing an optional arguments parameter will also allow us to add on existing behavior. Considering how complicated set comparisons might be, I wouldn't be surprised if people want more options for it later.

I'd at least make the boolean optional, defaulting it to the existing behavior. Just so it won't break other people's code when they upgrade.

Copy link
Contributor

Choose a reason for hiding this comment

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

By making it optional, I can also just call checker.CompareToSet(set) and I won't have to worry about breaking any test builds.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. Actually on the "real" public API, the SetComparisonExtensionMethods.CompareToSet extension method (https://github.com/techtalk/SpecFlow/blob/3fcdf09b4f28160d4f6851b4b7e4f98b24933df5/TechTalk.SpecFlow/Assist/SetComparisonExtensionMethods.cs#L7) I have used an optional parameter. I did not think that someone would use this method directly, but you are right, we should also make this backwards compatible to be on the safe side.
Regarding the option class approach: from the trainings I do, my experience is that people are not aware of the object initializer syntax, so as long as we only have one option, I would rather choose the optional parameter, or alternatively a different API method, like CompareToSetOrderSensitive... (maybe with a better name)

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I like that idea! Using a a separate method, I mean... I think that would be preferable to the trailing bit parameter.

I think it might be a cultural thing, in terms of .Net versus other stacks. I've spent a lot of time in Ruby, and having the trailing "options" parameter is much more common and accepted. Nobody thinks twice about it, as the language syntax around creating a hash/dictionary is much simpler and intuitive, and nobody thinks twice about the magic-string-ness of the object... but in C# developers have a conniption fit if they don't have a type defined with parameters with XML documentation. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darrencauthon and would would be your preferred name?

Copy link
Contributor

Choose a reason for hiding this comment

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

@gasparnagy Let me think on that today... perhaps my inability to think of a good name will change my mind. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@darrencauthon in Cucumber JVM the two method is called diff and unorderedDiff BTW. So it seems that also decided to go for two separate methods... https://cucumber.github.io/api/cucumber/jvm/javadoc/cucumber/api/DataTable.html

{
AssertThatAllColumnsInTheTableMatchToPropertiesOnTheType();

if (ThereAreNoResultsAndNoExpectedResults(set))
return;

var getListOfExpectedItemsThatCouldNotBeFound =
sequentialEquality ?
new Func<IEnumerable<T>, IEnumerable<int>>(GetListOfExpectedItemsThatCouldNotBeFoundOrderSensitive) :
new Func<IEnumerable<T>, IEnumerable<int>>(GetListOfExpectedItemsThatCouldNotBeFoundOrderInsensitive);

if (ThereAreResultsWhenThereShouldBeNone(set))
ThrowAnExpectedNoResultsError(set, GetListOfExpectedItemsThatCouldNotBeFound(set));
ThrowAnExpectedNoResultsError(set, getListOfExpectedItemsThatCouldNotBeFound(set));

AssertThatTheItemsMatchTheExpectedResults(set);
AssertThatTheItemsMatchTheExpectedResults(set, getListOfExpectedItemsThatCouldNotBeFound);

AssertThatNoExtraRowsExist(set, GetListOfExpectedItemsThatCouldNotBeFound(set));
AssertThatNoExtraRowsExist(set, getListOfExpectedItemsThatCouldNotBeFound(set));
}

private void AssertThatNoExtraRowsExist(IEnumerable<T> set, IEnumerable<int> listOfMissingItems)
Expand All @@ -62,17 +67,17 @@ private bool ThereAreNoResultsAndNoExpectedResults(IEnumerable<T> set)
return !set.Any() && !table.Rows.Any();
}

private void AssertThatTheItemsMatchTheExpectedResults(IEnumerable<T> set)
private void AssertThatTheItemsMatchTheExpectedResults(IEnumerable<T> set, Func<IEnumerable<T>, IEnumerable<int>> getListOfExpectedItemsThatCouldNotBeFound)
{
var listOfMissingItems = GetListOfExpectedItemsThatCouldNotBeFound(set);
var listOfMissingItems = getListOfExpectedItemsThatCouldNotBeFound(set);

if (ExpectedItemsCouldNotBeFound(listOfMissingItems))
ThrowAnErrorDetailingWhichItemsAreMissing(listOfMissingItems);
}

private IEnumerable<int> GetListOfExpectedItemsThatCouldNotBeFound(IEnumerable<T> set)
private IEnumerable<int> GetListOfExpectedItemsThatCouldNotBeFoundOrderInsensitive(IEnumerable<T> set)
{
actualItems = GetTheActualItems(set);
var actualItems = GetTheActualItems(set);

var listOfMissingItems = new List<int>();

Expand All @@ -89,12 +94,37 @@ private IEnumerable<int> GetListOfExpectedItemsThatCouldNotBeFound(IEnumerable<T
else
RemoveFromActualItemsSoItWillNotBeCheckedAgain(actualItems, matchIndex);
}

extraOrNonMatchingActualItems = actualItems.Select(i => new TableDifferenceItem<T>(i)).ToList();
return listOfMissingItems;
}

private IEnumerable<int> GetListOfExpectedItemsThatCouldNotBeFoundOrderSensitive(IEnumerable<T> set)
{
var actualItems = GetTheActualItems(set);

var listOfMissingItems = new List<int>();

var pivotTable = new PivotTable(table);

for (var index = 0; index < Math.Min(actualItems.Count, table.Rows.Count()); index++)
{
var instanceTable = pivotTable.GetInstanceTable(index);
if (!ThisItemIsAMatch(instanceTable, actualItems[index]))
listOfMissingItems.Add(index + 1);
}

extraOrNonMatchingActualItems =
listOfMissingItems.Select(index => new TableDifferenceItem<T>(actualItems[index-1], index)).Concat(
actualItems.Skip(table.RowCount).Select(i => new TableDifferenceItem<T>(i)))
.ToList();

return listOfMissingItems;
}

private void ThrowAnErrorDetailingWhichItemsAreMissing(IEnumerable<int> listOfMissingItems)
{
var message = tableDiffExceptionBuilder.GetTheTableDiffExceptionMessage(new TableDifferenceResults<T>(table, listOfMissingItems, actualItems));
var message = tableDiffExceptionBuilder.GetTheTableDiffExceptionMessage(new TableDifferenceResults<T>(table, listOfMissingItems, extraOrNonMatchingActualItems));
throw new ComparisonException("\r\n" + message);
}

Expand Down
4 changes: 2 additions & 2 deletions TechTalk.SpecFlow/Assist/SetComparisonExtensionMethods.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,10 @@ namespace TechTalk.SpecFlow.Assist
{
public static class SetComparisonExtensionMethods
{
public static void CompareToSet<T>(this Table table, IEnumerable<T> set)
public static void CompareToSet<T>(this Table table, IEnumerable<T> set, bool sequentialEquality = false)
{
var checker = new SetComparer<T>(table);
checker.CompareToSet(set);
checker.CompareToSet(set, sequentialEquality);
}
}
}
19 changes: 14 additions & 5 deletions TechTalk.SpecFlow/Assist/TableDiffExceptionBuilder.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,20 +24,29 @@ public string GetTheTableDiffExceptionMessage(TableDifferenceResults<T> tableDif
if (tableDifferenceResults.IndexesOfTableRowsThatWereNotMatched.Contains(index))
prefix = "- ";
realData.AppendLine(prefix + line);

var missingIndexedItem = tableDifferenceResults.ItemsInTheDataThatWereNotFoundInTheTable.FirstOrDefault(i => i.Index == index);
if (missingIndexedItem != null)
AppendExtraLineText(tableDifferenceResults, missingIndexedItem, realData);
index++;
}

foreach (var item in tableDifferenceResults.ItemsInTheDataThatWereNotFoundInTheTable)
foreach (var item in tableDifferenceResults.ItemsInTheDataThatWereNotFoundInTheTable.Where(i => !i.IsIndexSpecific))
{
var line = "+ |";
foreach (var header in tableDifferenceResults.Table.Header)
line += $" {GetTheValue(item, header)} |";
realData.AppendLine(line);
AppendExtraLineText(tableDifferenceResults, item, realData);
}

return realData.ToString();
}

private void AppendExtraLineText(TableDifferenceResults<T> tableDifferenceResults, TableDifferenceItem<T> item, StringBuilder realData)
{
var line = "+ |";
foreach (var header in tableDifferenceResults.Table.Header)
line += $" {GetTheValue(item.Item, header)} |";
realData.AppendLine(line);
}

private static object GetTheValue(T item, string header)
{
var propertyValue = item.GetPropertyValue(header);
Expand Down
22 changes: 22 additions & 0 deletions TechTalk.SpecFlow/Assist/TableDifferenceItem.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
using System;
using System.Collections.Generic;
using System.Linq;
using System.Text;
using System.Threading.Tasks;

namespace TechTalk.SpecFlow.Assist
{
public class TableDifferenceItem<TT>
{
public TT Item { get; private set; }
public int? Index { get; private set; }

public bool IsIndexSpecific => Index != null;

public TableDifferenceItem(TT item, int? index = null)
{
Item = item;
Index = index;
}
}
}
7 changes: 4 additions & 3 deletions TechTalk.SpecFlow/Assist/TableDifferenceResults.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
using System.Collections.Generic;
using System;
using System.Collections.Generic;

namespace TechTalk.SpecFlow.Assist
{
public class TableDifferenceResults<TT>
{
public TableDifferenceResults(Table table, IEnumerable<int> indexesOfTableRowsThatWereNotMatched, IEnumerable<TT> itemsInTheDataThatWereNotFoundInTheTable)
public TableDifferenceResults(Table table, IEnumerable<int> indexesOfTableRowsThatWereNotMatched, IEnumerable<TableDifferenceItem<TT>> itemsInTheDataThatWereNotFoundInTheTable)
{
this.Table = table;
this.IndexesOfTableRowsThatWereNotMatched = indexesOfTableRowsThatWereNotMatched;
Expand All @@ -15,6 +16,6 @@ public TableDifferenceResults(Table table, IEnumerable<int> indexesOfTableRowsTh

public IEnumerable<int> IndexesOfTableRowsThatWereNotMatched { get; }

public IEnumerable<TT> ItemsInTheDataThatWereNotFoundInTheTable { get; }
public IEnumerable<TableDifferenceItem<TT>> ItemsInTheDataThatWereNotFoundInTheTable { get; }
}
}
1 change: 1 addition & 0 deletions TechTalk.SpecFlow/TechTalk.SpecFlow.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@
<Compile Include="Assist\FormattingTableDiffExceptionBuilder.cs" />
<Compile Include="Assist\SafetyTableDiffExceptionBuilder.cs" />
<Compile Include="Assist\SetComparer.cs" />
<Compile Include="Assist\TableDifferenceItem.cs" />
<Compile Include="Assist\TableDifferenceResults.cs" />
<Compile Include="Assist\TableDiffExceptionBuilder.cs" />
<Compile Include="Assist\TEHelpers.cs" />
Expand Down
102 changes: 73 additions & 29 deletions .../TechTalk.SpecFlow.RuntimeTests/AssistTests/SetComparisonExtensionMethods_MessageTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,8 +5,7 @@

namespace TechTalk.SpecFlow.RuntimeTests.AssistTests
{
[TestFixture]
public class SetComparisonExtensionMethods_MessageTests
public abstract class SetComparisonExtensionMethods_MessageTests
{
[Test]
public void Returns_the_names_of_any_fields_that_do_not_exist()
Expand Down Expand Up @@ -98,31 +97,6 @@ public void Returns_2_as_the_missing_item_when_the_second_item_does_not_match()
".AgnosticLineBreak());
}

[Test]
public void Returns_both_1_and_two_as_the_missing_items_when_both_cannot_be_found()
{
var table = new Table("StringProperty");
table.AddRow("orange");
table.AddRow("apple");

var items = new[]
{
new SetComparisonTestObject {StringProperty = "rotten orange"},
new SetComparisonTestObject {StringProperty = "rotten apple"}
};

var exception = GetTheExceptionThrowByComparingThese(table, items);

exception.Message.AgnosticLineBreak().Should().Be(
@"
| StringProperty |
- | orange |
- | apple |
+ | rotten orange |
+ | rotten apple |
".AgnosticLineBreak());
}

[Test]
public void Returns_a_descriptive_error_when_three_results_exist_when_two_expected()
{
Expand Down Expand Up @@ -170,17 +144,87 @@ public void Returns_a_descriptive_error_when_three_results_exist_when_one_expect
".AgnosticLineBreak());
}

private static ComparisonException GetTheExceptionThrowByComparingThese(Table table, SetComparisonTestObject[] items)
protected ComparisonException GetTheExceptionThrowByComparingThese(Table table, SetComparisonTestObject[] items)
{
try
{
table.CompareToSet(items);
CallComparison(table, items);
}
catch (ComparisonException ex)
{
return ex;
}
return null;
}

protected abstract void CallComparison(Table table, SetComparisonTestObject[] items);
}

[TestFixture]
public class SetComparisonExtensionMethods_OrderInsensitive_MessageTests : SetComparisonExtensionMethods_MessageTests
{
[Test]
public void Returns_both_1_and_two_as_the_missing_items_when_both_cannot_be_found()
{
var table = new Table("StringProperty");
table.AddRow("orange");
table.AddRow("apple");

var items = new[]
{
new SetComparisonTestObject {StringProperty = "rotten orange"},
new SetComparisonTestObject {StringProperty = "rotten apple"}
};

var exception = GetTheExceptionThrowByComparingThese(table, items);

exception.Message.AgnosticLineBreak().Should().Be(
@"
| StringProperty |
- | orange |
- | apple |
+ | rotten orange |
+ | rotten apple |
".AgnosticLineBreak());
}

protected override void CallComparison(Table table, SetComparisonTestObject[] items)
{
table.CompareToSet(items);
}
}

[TestFixture]
public class SetComparisonExtensionMethods_OrderSensitive_MessageTests : SetComparisonExtensionMethods_MessageTests
{
[Test]
public void Returns_both_1_and_two_as_the_missing_items_when_both_cannot_be_found()
{
var table = new Table("StringProperty");
table.AddRow("orange");
table.AddRow("apple");

var items = new[]
{
new SetComparisonTestObject {StringProperty = "rotten orange"},
new SetComparisonTestObject {StringProperty = "rotten apple"}
};

var exception = GetTheExceptionThrowByComparingThese(table, items);

exception.Message.AgnosticLineBreak().Should().Be(
@"
| StringProperty |
- | orange |
+ | rotten orange |
- | apple |
+ | rotten apple |
".AgnosticLineBreak());
}

protected override void CallComparison(Table table, SetComparisonTestObject[] items)
{
table.CompareToSet(items, sequentialEquality: true);
}
}
}
Loading