Improving set comparison

[Vagif Abilov]

Hi,

I made some progress implementing functionality described in a feature request that I submitted earlier (#48). It is about improvements in Table.CompareToSet extension methods. Current implementation only verifies that two sets contain same elements, and sometimes it is important to verify:

- the sets are equal (elements are in the same order)
- the sets are not equal
- the sets are equivalent (current implementation, same elements, order not important)
- the sets are not equaivalent
- the set is a subset of another one
- the set is not a subset of another one (e.g. none of the elements belong to other set)
- the sets have common elements
- the sets don't have common elements

The implementation is close to completion, but I want to figure out the best API for it. My suggestion is to provide compatibility with the existing implementation, so CompareToSet will work exactly as it does now. To achieve it the method can be extended with an enumerator that specifies comparison mode:

    public enum TableComparison
    {
        IsEquivalent,
        IsNotEquivalent,
        IsEqual,
        IsNotEqual,
        IsSubset,
        IsNotSubset,
        HasCommonElements,
        HasNoCommonElements
    }

Then the extension method will be like this:

        public static void CompareToSet<T>(this Table table, IEnumerable<T> set, TableComparison expectedComparisonResult = TableComparison.IsEquivalent)
        {
            new SetComparer<T>(table).CompareToSet(set, expectedComparisonResult);
        }

Alternative would be to implement different methods for different expected results, similar to how it is done in MsTest CollectionAssert (http://msdn.microsoft.com/en-us/library/microsoft.visualstudio.testtools.unittesting.collectionassert_members%28v=vs.80%29.aspx). In this case new extension methods will be introduced, e.g.:

table.VerifyIsEquivalentTo<T>(set);
table.VerifyIsSubsetOf<T>(set);
table.VerifyHasCommonElementsWith<T>(set);
etc.

I am not sure which one is better. Perhaps the second one is more fluent and descriptive. But it does not match CompareToSet. Each of them is easy implement (I am done with most of it already).
What do you think?

Vagif

[Darren Cauthon]

Vagif,

Thanks for your work! I appreciate it very much. I really was going
to get to this, but I'm just a little booked up until next week. :)

I like the fluent interface, too. I think the CompareToSet needs to
stay either way, since it's being used in current versions, but it
doesn't necessarily mean that the fluent interface can't work as
well. I have a couple recommendations:

1.) I'd probably just focus on getting the CompareToSet method
working completely, and issue a pull request just for that feature.
The more fluent interface can be its own feature and come
afterwards.

2.) Now, normally I'm not a fan of enums, for reasons I won't get
into here. However, when I do think they are usable, I like to make
sure they are very descriptive and understandable. It's early, but
let me take a stab...

IsEquivalent = TheTableAndSetMatchWithoutRegardForOrder
IsNotEquivalent = NoneOfTheItemsInTheTableAreInTheSet
IsEqual = TheTableAndSetAreAnExactMatch
IsNotEqual = TheTableAndSetAreNotAnExactMatch
IsSubset = AllItemsInTheTableAreInTheSet
IsNotSubset = ???
HasCommonElements = ???
HasNoCommonElements = ???

It might not match any existing collection assertions, but I think it
would be easier for SpecFlow users to use the feature.


If you want any help, even for testing, just let me know and I'll take
a look. I'm also interested in how the logic and tests split; there's
probably some room for cleanup there.

Thanks, I'm excited for the feature!


Darren

> results, similar to how it is done in MsTest CollectionAssert (http://msdn.microsoft.com/en-us/library/microsoft.visualstudio.testto...).

[Vagif Abilov]

Hi Darren,

Thanks for quick response. I am myself not fan of enums, this is why I suggested fluent methods, but enums seem to be a reasonable way to keep existing implementation backward compatible.
I will start working on the implementation using your naming suggestions, I believe since the whole Assist namespace is done mostly by you, we should keep the naming spirit. Your names are also more descriptive, with mine there is a risk to confuse where's the table and where's the set. One this I'd check however: are you sure you want enum values to contain article "the". I know it will make English correct but a/the articles are usually skipped from such names. Also, if we keep "the" for "table", shouldn't we have it also for "set"?

So here are two versions of names: your original (I added missing definitions), and the one without definite articles.

IsEquivalent = TheTableAndSetMatchWithoutRegardForOrder // default, corresponds to current behavior
IsNotEquivalent = TheTableAndSetAreNotMatch // negation of the default

IsEqual = TheTableAndSetAreAnExactMatch
IsNotEqual = TheTableAndSetAreNotAnExactMatch
IsSubset = AllItemsInTheTableAreInTheSet

IsNotSubset = NotAllItemsInTheTableAreInTheSet
HasCommonElements = TheTableAndSetHaveCommonItems
HasNoCommonElements = NoneOfTheItemsInTheTableAreInTheSet

TableAndSetMatchWithoutRegardForOrder // default, corresponds to current behavior
TableAndSetAreNotMatch // negation of the default
TableAndSetAreAnExactMatch
TableAndSetAreNotAnExactMatch
AllItemsInTableAreInSet
NotAllItemsInTableAreInSet
TableAndSetHaveCommonItems
TableAndSetHaveNoCommonItems

Let me know what you think

Vagif

[HL]

I've also been looking in this area (see my other post about intance variables), I've had some success with duplicating the table extension methods to also take a ScenarioContext. 

 

So, if a table is:

| UserID                   | UserName                  |

| @generatedUserID | @generatedUserName |

 

Values starting with "@" are looked up in the ScenarioContext passed into the extension method, if they can't be found the value is used as is.

 

Obviously your earlier steps have to populate the ScenarioContext with the corresponding key,value pairs and any value has to be convetable to a string.

If you've got any idea how you could specify the value should be saved in the context in an earlier step  in the Feature file directly I'd be very interested

 

Is this of any interest to anyone else?

[Darren Cauthon]

Vagif,

Unfortunately, you're getting my responses either really early in the
morning or late in the evening, so I may not be thinking clearly...

I read through these descriptions, and I think there might have been a
case of duplication and some cleanup. Plus, I think it might be a
good idea to limit the scope of the updates, just to make it easier to
grasp and test. Having more options is great, but it's not like we
have to provide every possible way to cross the set and the table. :)
If we have just two or three options available, we'd probably be in a
better position to make sure everything is on the right track.

So below is my list and a little bit of refactoring of the enum
values.

If you would like (and because I think it would be fun...) I'd love to
be able to help with this process using some good ol' social coding
with GitHub. Maybe while we're in there we can get a couple refactors
in there. I have exactly *three* statements in that code that I have
on my post-1.6 hit list.

As for the "the" article, yeah... I think that's probably a question
for the Gáspár or Jonas. I like using "the" in this particular
situation. I don't think it belongs everywhere, but when code starts
to get more complex I try to make my code really, really expressive.
And since we're talking about very subtle differences in how the
tables are compared, I'd like those enum values to read almost as if
they were sentences. :)


Important:

IsEquivalent = TheTableAndSetMatchWithoutRegardForOrder
IsNotEquivalent = NoneOfTheItemsInTheTableAreInTheSet
IsEqual = TheTableAndSetAreAnExactMatch

IsSubset = AllItemsInTheTableCanBeFoundWithinTheSet

Next:
IsNotEqual = TheTableAndSetAreNotAnExactMatch

IsNotSubset = NotAllItemsInTheTableAreInTheSet
HasCommonElements = TheTableAndSetHaveCommonItems

Duplicated?
HasNoCommonElements = NoneOfTheItemsInTheTableAreInTheSet



Darren

[Darren Cauthon]

My only fear with this is exposing the ScenarioContext to the text
version. I feel like ScenarioContext is really meant to be a way to
shuttle objects and values between our methods (since we're not
writing on top of the World object like in Cucumber). I don't know if
those types of step-definition-level details should be visible in the
specs themselves.


Darren

[Vagif Abilov]

Hi Darren,

There may be too many choices, agree. I just tried to go through all possible cases. I believe during the implementation phase with concrete example we'll see what's really needed, but in general I agree with your choice of most important ones.
I don't think IsNotEquivalent is the same as NoneOfTheItemsInTheTableAreInTheSet. IsEquivalent is your current implementation that throws an exception if two collections have some elements that are not equal. Therefore IsNotEquivalent asserts that there are such elements but does not require that theren't any common elements. NoneOfTheItemsInTheTableAreInTheSet asserts that the table and the set have empty intersection.

When it comes to naming as I said you can make a final choice, my only wish is it is consistent across all enumerator values. I see below that "the"  is sometimes applied to "set", sometimes not. Enghlish is not my native language, so I may not get some nuances.

While reviewing the proposition, it struck me that one possible implementation could be do replace enumerator with a lambda expression, something like this: CompareToSet(IEnumerable<T> set, Predicate<Table, IEnumerable<T>> comparator), and provide some standard implementations for typical scenarios. I need to think more about it.

Perhaps the most efficient way to continue is for me to do some coding and submit it to github. I'll drop you a mail when I have something to show.

Vagif

[Darren Cauthon]

Vagif,

Ok, I think I see what you're saying why there isn't duplicates. I
think the one I marked as duplicate might not be in the "priority"
list, but I guess we'll figure it out when we get deeper in the code.

When it comes to "the," I don't think putting an article is always
necessary when grouping two subjects. Or, at least, it's sometimes
left out when people speak it. It's the difference between:

The cat and the dog jumped over the log.
The cat and dog jumped over the log.

The first sentence looks better, but I don't think it's how people
always speak. And, more importantly, dropping the second "the"
doesn't drop any meaning to someone reading the sentence.

In all of the cases I listed, the times I don't put "the" in front of
"step" is when I say "The table and" before. Anytime I don't have
another subject (with its own article) preceding, I will put "the" in
front of "set."

We'd probably be on better English footing adding a second "the," but
then we're talking about the difference between:

TheTableAndSetMatchWithoutRegardForOrder
TheTableAndTheSetMatchWithoutRegardForOrder

Now we're dealing with long variable names, where the meaning of
either can't be grasped by glancing at the variable name. Names like
this are dangerous, so I'd like to cut out as much "optional" wording
that I can. The second "the" is the only word I see that we can drop
without becoming cryptic.

That's why I'd prefer we stick with the former. But hey, I should not
be trusted alone with English, I live in Kansas where we have our own
special dialect. :)

I'll be waiting for that Github branch. Thanks!


Darren

On Apr 8, 12:47 am, Vagif Abilov <vagif.abi...@gmail.com> wrote:
> Hi Darren,
>

> There may be too many choices, agree. I just tried to go through all
> possible cases. I believe during the implementation phase with concrete
> example we'll see what's really needed, but in general I agree with your
> choice of most important ones.
> I don't think IsNotEquivalent is the same as
> NoneOfTheItemsInTheTableAreInTheSet. IsEquivalent is your current
> implementation that throws an exception if two collections have some
> elements that are not equal. Therefore IsNotEquivalent asserts that there
> are such elements but does not require that theren't any common elements.
> NoneOfTheItemsInTheTableAreInTheSet asserts that the table and the set have
> empty intersection.
>
> When it comes to naming as I said you can make a final choice, my only wish
> is it is consistent across all enumerator values. I see below that "the"  is
> sometimes applied to "set", sometimes not. Enghlish is not my native
> language, so I may not get some nuances.
>
> While reviewing the proposition, it struck me that one possible
> implementation could be do replace enumerator with a lambda expression,
> something like this: CompareToSet(IEnumerable<T> set, Predicate<Table,
> IEnumerable<T>> comparator), and provide some standard implementations for
> typical scenarios. I need to think more about it.
>
> Perhaps the most efficient way to continue is for me to do some coding and
> submit it to github. I'll drop you a mail when I have something to show.
>
> Vagif
>

[Vagif Abilov]

Darren,

Now I see the reason for dropping the second article: in case of conjunction it is shared between diffrerent words. Then it makes sense and become logical.

I hope to send you something tonight.

Vagif

[Vagif Abilov]

Hi Darren,

I've implemented support for different set comparison criteria. To illustrate how it can be used, I wrote a blog post:

http://bloggingabout.net/blogs/vagif/archive/2011/04/08/enhancing-specflow-set-comparison-methods.aspx

As you can see, I introduced enumerator with four values corresponding to main comparison criteria:

    public enum TableComparison
    {
        TheTableAndSetMatchWithoutRegardForOrder,
        TheTableAndSetAreAnExactMatch,
        AllItemsInTheTableCanBeFoundWithinTheSet,
        TheTableAndSetHaveCommonItems
    }

The last one differs from what you suggested, in fact it inverted it. I first implemented negated form of it, the one that asserts that collections have no common elements, but it didn't really fit others that assert for presence of something. After I inverted it and turned into TheTableAndSetHaveCommonItems, their implementation was more unified.

As you can also see, in order to express negative expectations, I added a second optional parameter to CompareToSet: a boolean value (default is "true") that can be set to specify whether match is expected or not. If the value is set to "false", then the method asserts for negative result of comparison.

To play with the extended method, I created a sample project available here: https://github.com/object/SpecFlowCompareSet. It basically demonstrates what I've described in my blog post. In addition it has two "Error" features that I used to validate that failed assertions generate correct exceptions and message. Ideally they should be rewritten and placed in unit test project for SpecFlow.Assist, but I wanted first you to review the concept.

I had to do some refactoring in the code, because some of internals had to be changed quite a bit. But I believe the main structure of the file (I only updated one file - SetComparisonExtensionMethods.cs) is the same, and the implementation is straightforward. I had to introduce control block with switch statements to choose execution path for selected comparison mode.

Please let me know what you think about it and how you would like to proceed from here.

Best regards
Vagif

[Vagif Abilov]

And I forgot to mention that my forked SpecFlow is here: https://github.com/object/SpecFlow

Vagif

[Vagif Abilov]

... and when it comes to tests for this feature, I can add unit tests to RuntimeTests\AssistTests that validate this functionality, but I would like your opinion regarding how these tests should be structured (added to existing files, put in a new file, should be grouped by TableComparison value etc.)

Vagif

[Darren Cauthon]

Hi Vagif,

Sorry for not getting back to you until now -- it has been a long,
long weekend.

I think the easiest way to test this is to write a few basic stories
in SpecFlow that describe the functionality, and then get into unit
tests with a separate file for each type of testing.

It's not what I did back then, but I know better now. :)

I looked at the code and have a couple suggestions, but I'll save them
for in code.

Thanks again for doing this!


Darren

On Apr 10, 2:30 pm, Vagif Abilov <vagif.abi...@gmail.com> wrote:
> ... and when it comes to tests for this feature, I can add unit tests to
> RuntimeTests\AssistTests that validate this functionality, but I would like
> your opinion regarding how these tests should be structured (added to
> existing files, put in a new file, should be grouped by TableComparison
> value etc.)
>
> Vagif
>

> On Sat, Apr 9, 2011 at 12:03 AM, Vagif Abilov <vagif.abi...@gmail.com>wrote:
>
>
>
>
>
>
>
> > And I forgot to mention that my forked SpecFlow is here:
> >https://github.com/object/SpecFlow
>
> > Vagif
>

> > On Fri, Apr 8, 2011 at 11:59 PM, Vagif Abilov <vagif.abi...@gmail.com>wrote:
>
> >> Hi Darren,
>
> >> I've implemented support for different set comparison criteria. To
> >> illustrate how it can be used, I wrote a blog post:
>

> >>http://bloggingabout.net/blogs/vagif/archive/2011/04/08/enhancing-spe...

> >> On Fri, Apr 8, 2011 at 3:52 PM, Vagif Abilov <vagif.abi...@gmail.com>wrote:
>
> >>> Darren,
>
> >>> Now I see the reason for dropping the second article: in case of
> >>> conjunction it is shared between diffrerent words. Then it makes sense and
> >>> become logical.
>
> >>> I hope to send you something tonight.
>
> >>> Vagif
>

> ...
>
> read more »

[Gáspár Nagy]

Hi Guys!

Sorry for jumping into the middle of the discussion, but I was
thinking on an alternative solution. Before going into the
implementation of the different tricky patterns, like subset,
intersect, etc. maybe we should slice the problem up. We already have
a good framework for calculating set operations (union, intersect,
etc.): LINQ

Here is my solution:
We need a class that represents an "abstract" instance. Abstract means
it can be a row in the table or a real instance. Let's call this
TableRowOrInstance<TInstance> for the sake of simplicity. Imagine that
this has a proper equals method (see my dummy implementation below),
and you have extension methods that can "convert" a table or a set of
instances to this form:

public static IEnumerable<TableRowOrInstance<TInstance>>
AsAbstractInstanceSet<TInstance>(this Table table)
{
return table.Rows.Select(r => new
TableRowOrInstance<TInstance>(r));
}

public static IEnumerable<TableRowOrInstance<TInstance>>
AsAbstractInstanceSet<TInstance>(this IEnumerable<TInstance>
instances)
{
return instances.Select(i => new
TableRowOrInstance<TInstance>(i));
}

With this a subset assertion could be formulated this way:

table.AsAbstractInstanceSet<SetComparisonTestObject>().Except(items.AsAbstractInstanceSet()).ShouldBeEmpty();

(note that the Except here is the LINQ Except method, there you can do
anything else, like Intersect() or SequentialEqual())


What do you think? I'm not 100% sure (especially with the naming), but
maybe this gives you ideas.


Here is the dummy code for the TableRowOrInstance<TInstance>, sorry
that it is a bit long:

public class TableRowOrInstance<TInstance>
{
public TableRow TableRow { get; private set; }
public TInstance Instacne { get; private set; }

public bool IsRow { get { return TableRow != null; } }

public bool IsInstance { get { return !IsRow; } }

public TableRowOrInstance(TableRow tableRow)
{
TableRow = tableRow;
}

public TableRowOrInstance(TInstance instacne)
{
Instacne = instacne;
}

public override int GetHashCode()
{
return 42; //unfortunately it is impossible to calculate a
proper hashcode
}

public override bool Equals(object obj)
{
TableRowOrInstance<TInstance> other = obj as
TableRowOrInstance<TInstance>;
if (other == null)
{
if (obj is TableRow)
other = new
TableRowOrInstance<TInstance>((TableRow)obj);
else if (obj is TInstance)
other = new
TableRowOrInstance<TInstance>((TInstance)obj);
else
return false;
}

if (IsInstance && other.IsInstance)
return Equals(Instacne, other.Instacne);

if (IsRow && other.IsRow)
return Equals(TableRow, other.TableRow); //TODO: we
need an equals on the TableRow

if (IsRow)
{
Debug.Assert(other.IsInstance);
//TODO: invoke Assist.CompareInstance but without the
exception
return CompareToSetTmp(TableRow, other.Instacne);
}

Debug.Assert(IsInstance);
Debug.Assert(other.IsRow);

return CompareToSetTmp(other.TableRow, Instacne);
}

// this is a temporary hack now
private static bool CompareToSetTmp(TableRow tableRow,
TInstance instance)
{
try
{
var table = new Table(tableRow.Select(r =>
r.Key).ToArray());
table.AddRow(tableRow.Select(r => r.Value).ToArray());
table.CompareToSet(new[] { instance });
return true;
}
catch (Exception)
{
return false;

> ...
>
> read more »

[Josh Carroll]

I obviously haven't been active on this group for a while, but wanted to at least mention a few things.

Gaspar is right. You should work with the framework since it is a paradigm that is already understood.

System.Collections.Generic.ISet<T>  offers pretty much all of the set operations you are looking for.

Unfortunately, all of the Linq set operators pretty much require you to implement IEquatable, providing an implementation of both Equals(), and GetHashcode()

Without GetHashcode(), they will not work as expected. I know only because I have recently been using them extensively for custom types in Unit testing.

Anyway how this helps avoid some problems.

-- Josh

On Apr 11, 2011 5:15 AM, "Gáspár Nagy" <gaspa...@gmail.com> wrote:

[Vagif Abilov]

Hi Gaspar, Josh and Darren,

I agree that we should try to reuse existing framework paradigms as much as we can, especially if alternative implementation is based on introduction of new types (like the enumerator which is used in my current implementation).
I will look into the Gaspar's sketch and try to play with implementation (no problem if any of you come with it first, I enjoy it anyway :-))

Vagif

[Darren Cauthon]

Actually, I'm going to have to respectfully disagree on this, mainly
for a couple reasons:

1.) I feel like creating another abstraction or concept isn’t
necessary, since we have a working solution in place right now that’s
*relatively* simple.

2.) I know it’s mostly in my head and I haven’t shared it with many
others, but right now the table instance and comparisons are on the
verge of another type of refactor that will a.) make things much
simpler, and b.) make it possible to extend this thing (even custom
uses), and c.) remove the 2-3 little hacks I put in to handle some
weird custom issues with dates, decimals, or something else like that,
and d.) allow better reporting of differences.

I want to apply the same refactor to Vagif’s work. What he has there
*will* work, but the stacking with the case statements are begging to
be refactored out into separate, small pieces.

SpecFlow is an open-source project, anybody can write whatever they
want, but before we do any major restructuring can I put my ideas
into specs and code and show it? I think/hope everybody will be
pleasantly surprised.

(I’m sorry – I should have done it already, but with all of the other
updates I thought it would be better to let 1.6 hit its release before
I did anything major.)


Darren

On Apr 11, 6:19 am, Vagif Abilov <vagif.abi...@gmail.com> wrote:
> Hi Gaspar, Josh and Darren,
>
> I agree that we should try to reuse existing framework paradigms as much as
> we can, especially if alternative implementation is based on introduction of
> new types (like the enumerator which is used in my current implementation).
> I will look into the Gaspar's sketch and try to play with implementation (no
> problem if any of you come with it first, I enjoy it anyway :-))
>
> Vagif
>

> On Mon, Apr 11, 2011 at 12:57 PM, Josh Carroll <jwarren.carr...@gmail.com>wrote:
>
>
>
>
>
>
>
> > I obviously haven't been active on this group for a while, but wanted to at
> > least mention a few things.
>
> > Gaspar is right. You should work with the framework since it is a paradigm
> > that is already understood.
>
> > System.Collections.Generic.ISet<T>  offers pretty much all of the set
> > operations you are looking for.
>
> > Unfortunately, all of the Linq set operators pretty much require you to
> > implement IEquatable, providing an implementation of both Equals(), and
> > GetHashcode()
>
> > Without GetHashcode(), they will not work as expected. I know only because
> > I have recently been using them extensively for custom types in Unit
> > testing.
>
> > Anyway how this helps avoid some problems.
>
> > -- Josh

> > On Apr 11, 2011 5:15 AM, "Gáspár Nagy" <gaspar.n...@gmail.com> wrote:

[Vagif Abilov]

Now feel being in between now :-) Actually I am sitting now playing with ISet implementations trying to figure out what can be done. It's premature for me to compare two approaches because I haven't fully grasped how the latest proposition might look. But my first impression:

Change that I applied to Darren's code was really simple. Since this was my first attempt to contribute to SpecFlow, I deliberately restricted myself to small changes. You can see it was a small effort, and it conforms the spirit of incremental development: implement new functionality using least effort and then refactor.

Having said that, I do rate approach that would use standard framework capabilities higher providing that it won't result in considerable increase of complexity. And for the time being I fail to grasp how ISet-flavoured implementation would look. So if I could choose, I would probably keep (with some changes) suggested implementation, so it could be used and reviewed by others, and since it's not part of release and don't need backward compatibility in the future, work on ISet-based approach in refactoring stage that may or may not be adopted depending on how elegant it becomes.

But I am puzzled to play with Gaspar/Josh suggestion.

Vagif

[Vagif Abilov]

Played little with IQueryable, and indeed LINQ covers pretty much what's needed. Assuming we have IQueryable a and b most commonly used cases look as follows:

a_should_match_b

Assert.IsTrue(a.Count() == b.Count() && a.Except(b).Count() == 0);


a_should_not_match_b
 Assert.IsFalse(a.Count() == b.Count() && a.Except(b).Count() == 0);

a_should_exactly_match_b
Assert.That(a.SequenceEqual(b), Is.True);



a_should_not_exactly_match_b
Assert.That(a.SequenceEqual(b), Is.False);

a_should_contain_all_in_b
Assert.That(a.Except(b), Is.Empty);


a_should_not_contain_all_b
Assert.That(a.Except(b), Is.Not.Empty);

a_should_have_common_items_with_b
Assert.That(a.Except(b).Count(), Is.LessThan(a.Count()));


a_should_not_have_common_items_with_b
Assert.That(a.Except(b).Count(), Is.EqualTo(a.Count()));

With TableRow/Instance equality in place, methods like AssertThatNoExtraRowsExist becomes redundant because this will be handled by LINQ.

So it is quite tempting, and it opens for any custom comparison, not just predefined ones.

Vagif