MatchRows command implementation

[Eugene Degtiarenko]

Hello all,

I've been using Concordion for a half of year now and like it very much.

Typically my tests asserts that some events have been raised in the system. It is very useful to assert events fields using tables, but unfortunately system may receive them in different order. 

Let me propose a command that will match rows in table ignoring differences in order.

Implementation

Spec with html examples in html or pdf

Spec with table  examples in html or pdf

Matching algorithm is:

• Populate matching candidates list with actual data
• For each expected row try to find best matching one.
  • If matching row is found then remove it from matching candidates and evaluate expected row against it.
  • If matching row is not found then report expected one missing.
• For each remaining row in matching candidates list report surpulus.

Best matching algorithm is:

• Find row in matching candidates list that has maximum number of fields equal to expected row (matching field count).
• If matching field count is equal to fields count then matching candidate equal to expected row, return it.
• If matching field count then zero but less then fields count then return it as partially matching.
• If there are more then one row with same matching field count then matching is ambiguous, return nothing.

Please let me know what You think.

Regards,

Ievgen Degtiarenko

[Nigel Charman]

Hi Ievgen

Thanks for the post and pull requests.

As per issue 21, this is an oft-requested enhancement to Concordion. I've bcc'd the requesters on issue 21 for their opinions.

For some use cases, I do think it will be important to differentiate the key columns that we want to match from the value columns that we want to compare.

For instance, in the last example of your spec:

Expected:
| John | Smith | 17 |

Actual:
| John | Smith | 18 |
| Jane | Smith | 17 |

Result in spec:
| John | Smith | 17 |
| John | Smith | 18 |
| Jane | Smith | 17 |

If we were to define the first 2 columns as keys, we could get a better result using your best matching algorithm against just the first 2 columns:

| John | Smith | 17 18 |
| Jane | Smith | 17 |

If no columns are marked as keys, we could default to all columns being used to determine a match.

I'm keen for more opinions on this.

cheers

--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-de...@googlegroups.com.
To post to this group, send email to concord...@googlegroups.com.
Visit this group at http://groups.google.com/group/concordion-dev.
To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/d025c7b1-d303-4fdd-83ce-f4e77bc9282a%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Tim Wright]

Thinking about how to match the columns, rather than have something in the table tag itself, how about the <th> tags? Then we can specify a match priority as well - and the match attributes are close to the column attributes. Something like this might work:

<table c:verifyRows="#data" c:verificationMethod="bestMatch">

  <tr>

    <th c:assertEquals="#data.id" c:columnMatchType="match" c:matchPriority="1">ID of Data</th>

    <th c:assertEquals="#data.position" c:columnMatchType="no_match"></th>

</th>

...

</table>

It would even let us match on a column that is not being asserted. Although I can't think of a use case for that!

Tim

To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/55D9AF7D.8020808%40gmail.com.

For more options, visit https://groups.google.com/d/optout.

--

Tim

021 251 5593

http://www.linkedin.com/in/drtimwright 

[Eugene Degtiarenko]

Hello all,

Thanks for your ideas, guys. Will try to add match priority (I believe this is more flexible then just key\value) during this weekend.

Also, could you please write me if you have any red flags during code review. What approach is better: new method in Command interface or ElementUpdating.instance().isUpdatesRestricted() is fine?

понедельник, 24 августа 2015 г., 2:01:24 UTC-4 пользователь Tim Wright написал:

[Nigel Charman]

Hi

To be honest, I don't like either approach of the new method in Command interface or the isUpdatesRestricted() guard clause. The former because it adds noise and could break existing extensions, and the latter because it stores static state so won't work with the parallel run extension. I guess changing it to ThreadLocal could solve the latter, but it's still messy.

I do like the verificationStrategy work you've done in PR #120 and think that this is the way forward. For the case I was suggesting, we could add an exact-match strategy. We should also look to support user-supplied strategies.

How about introducing a c:match command, which would simplify Tim's suggestion to:

<table c:verifyRows="#data" c:verificationStrategy="best-match">

  <tr>

    <th c:match="#data.id" c:matchPriority="1">ID of Data</th>

    <th c:assertEquals="#data.position"></th>

</th>

...

</table>

This is assuming that we always want to match for equality. Any thoughts on whether additional match types would be needed?

I think the strategy implementation would be able to execute all the c:match commands first to determine which row to verify, and only execute the assertions once. This would obviate the need to introduce the new method in Command or add the isUpdatesRestricted() method.

I would also be tempted to make the default match priority as being left to right, so that in most cases we wouldn't need to define the additional attribute.

What do you think?

Nigel.

[Eugene Degtiarenko]

Hi,

• I get the point, I'll update replace static state with thread local (did not expect someone is running specs in parallel, but this definitely make sense) for existing pr with strategies.
  
• I'll close other one with new command, it seems less promising.
  
• Verify strategies already could support user-supplied strategies. By default it tries to load class from the org.concordion.internal.command.strategies. package and if nothing is found tries to load something as if strategy name is fully qualified class name. And of course default behavior will be absolutely same with existing verifyRows. 
  
• I'll open one more branch with new match commands. Also, what do you think if I name that verifyField/verifyFieldFalse/verifyFieldTrue and verifyPriority to be concise with the verifyRows? verifyField/verifyFieldFalse/verifyFieldTrue will be similar to assert commands, but will be designed specially for the table to enable background check.

суббота, 29 августа 2015 г., 2:57:35 UTC-4 пользователь Nigel Charman написал:

[Nigel Charman]

Hi

I'd still like to explore other options rather than thread local, for example:

• executing the match algorithm first with the new match commands not updating the elements. After a match is found, run the asserts on only that row so that the asserts are only run once per row, or
  
• copying the table elements, and running the match algorithm against the table copy, then running the asserts against the original table.
  

For the new command names, how about verifyKeyEquals, verifyKeyTrue, verifyKeyFalse and verifyKeyPriority? I'm undecided whether it is better to use "verify" or "match" as a prefix. Using "verify" ties it in with verifyRows, but could imply we're verifying the field itself whereas "match" implies that we're using it for matching.

Nigel.

To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/5287e9da-7ebf-43be-90dc-f856e8124205%40googlegroups.com.

[Tim Wright]

Hey,

I'm a fan of the "matchKeyEquals" rather than "verify...". It 'seems' more generic and might let us reuse the keywords elsewhere.

I see this working in a two pass process over the table:

Pass 1: re-order the table/data

Pass 2: execute the verification using existing "verifyRows" schematics.

That should let existing extensions for verifyRows work - which is always a nice thing.

Thinking about if we want to re-order the table or data, I'm leaning toward re-ordering the table - not the data. That way if something goes wrong, the user can see the order that the data was returned from the fixture. They can see the table order by looking at the raw specification HTML.

Tim

To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/55E20A38.7050508%40gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Eugene Degtiarenko]

Hello all,

Please review my next PR with discussed changes:

https://github.com/concordion/concordion/pull/121

Ievgen

суббота, 29 августа 2015 г., 17:40:52 UTC-4 пользователь Tim Wright написал:

[Nigel Charman]

Thanks. At first look, the PR is looking good. I'll need to do a more detailed review, which will have to wait until the weekend - there's quite a lot there :)

[Tim Wright]

Hey,

I just had a chance to look at this - the number of tests you've written is great! It looks like there are two API classes you've modified - so I'll just look at those because once we've released an API, it's hard to change later. In general the code looks good - and I like how you've cleaned up some of the existing code to use the foreach construct instead of a standard loop construct. Overall seems a well constructed request!

Here are my picky thoughts:

CommandCallList.java:

This currently implements Iterator - which means we can use it in a nice for loop construct. My preference would to have a method on it (like getCommands()) that returns a Collection - rather than implementing Iterator directly. This might give us a bit more flexibility in future. Basically I think that CommandCallList *has* a collection of CommandCalls but *is* not a collection of command calls.

Element.java:

It looks like there is some code that looks up the concordion namespace - hard coded as the 2007 namespace. There's a better way to do this - the CommandCall class has these set properly - and should be usable by all commands. Look at the "setParameters" and "getParameters" methods in that class. Then line 61 of VerifyRowsStrategy can just do:

String strategy = commandCall.getParameter("verificationStrategy)"

instead of this:

String strategy = commandCall.getElement().getConcordionAttributeValue("verificationStrategy", "verification-strategy");

It's probably worth mentioning that that method on the CommandCall didn't exist when you started this work - it was a recent addition so doesn't surprise me at all that you didn't notice it!

VerifyRowsStrategy.java

I'd prefer for the constructor to take a List of Objects for the actual rows rather than an Iterator of objects. In this case, I'd go with List rather than Collection because the order of the rows might matter. It'd also be good if all the fields were private and there were getters provided - that's just a bit cleaner and gives us more flexibility in future. It'd also be nice if there was a null args constructor and setters for each of the properties. That would make the code in VerifyRowsCommand when we construct the strategy class easier - just call newInstance on the class (no need to lookup constructor) and then cast to a VerifyRowsStrategy and call all the setters.

Tim

To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/55ED5B66.6010203%40gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Nigel Charman]

Thanks for the review Tim. I agree with your comments, especially about it being "well constructed" :)

I've just reviewed too. I made a few notes on minor issues on the PR. My additional comments:

Multi-pass evaluation
Since the new strategies are potentially evaluating the commands on each row multiple times, this could cause issues if the commands are not idempotent or are slow.

I suggest we document this in the specifications, rather than trying to address it, which would be intrusive. In the specifications for BestMatch and KeyValue, I think we should explicitly say that the commands are being evaluated on a copy of the rows, potentially multiple times.

Naming
I'd suggest:
concordion:matchStrategy rather than concordion:verificationStrategy

KeyMatch rather than KeyValue

Strategy class names
I think it would be better to append "MatchStrategy" to the names of the strategy classes, eg DefaultMatchStrategy.java rather than Default.java

Specifications
For the 4 new specifications for the strategies, I'd rather that assertEquals was executed on the Result column than assertTrue on the Scenario column. This makes it more visible what is being asserted, and outputs the actual result on failure.

I also think the specs would be more readable if each scenario only contained the table rows, and the table header were described outside of the scenarios. Let me know if you'd like me to do this.

Target Release
The code on master is targeted for a Concordion 2.0 release, including Tim's changes to run each example as a separate JUnit test. We are aiming to get 2.0 released by the end of this year, with a SNAPSHOT version due to be released in the next few days. Are you OK for your changes to go out with this release?

The alternative is a 1.5.2 release, branching from the 1.5.1 tag - this would not be able to use the changes that Tim describes in Element.java below.

If you're happy working either from source, or from a snapshot repository, I'd suggest we target 2.0. Otherwise, if you'd like it in the Maven Central, we'll create a 1.5.2.


Discussion is encouraged if anyone feels differently about these comments :)

cheers
Nigel

[Eugene Degtiarenko]

Forwarding message to Tim and concordion-dev

Hi Eugene

I just noticed that this was only sent to my email address. Would you resend to the list please.

We'll need to discuss your PS further, since this is by design. Are you able to give a concrete example of this?

cheers

On 12/09/15 00:00, Eugene Degtiarenko wrote:

Hello all,

Thanks for the reviewing. I will be able to fix that today in the evening, hope that can be merged soon.

As for the VerifyRowsStrategy.java i would like to avoid using getters and setters as this makes fields not final so that you are able to call verify() on uninitialized properly object. I would totally agree that we need getters and setters on entites/dtos/dataclasses but I would like to use constructor initialization for service or component classes.

Specifications

I went with assertEquals initially but I faced html formatting issue. Is there way to call assertEquals(#expected, #actual) in html and is it going to print difference in html? Anyway, I'll try to improve that as well.

Target Release

Generally, I would like to release earlier so that people may start to use new feature, but final decision should be made by project owners :).

P.S.

Before releasing I would like to update one more thing. Recently I have noticed that I am not able to use <span assertEquals="someMethod('arg')">hello</span> in my spec even if it is valid OGNL expression. After converting previous to <span set="#key">arg</span> <span assertEquals="someMethod(#key)">hello</span> everything worked. I would like to rise separate PR targeting same release for that during the weekend.

Cheers,

Євген

[Eugene Degtiarenko]

Hello All,

As for the OGNL expressions i will start new topic on google groups from home. That is separate topic related to SimpleEvaluator class.

Regards

[Nigel Charman]

On 12/09/15 00:00, Eugene Degtiarenko wrote:

Specifications

I went with assertEquals initially but I faced html formatting issue. Is there way to call assertEquals(#expected, #actual) in html and is it going to print difference in html? Anyway, I'll try to improve that as well.

The verifyRows/TableBodySupport spec has an example of asserting on html.

[Eugene Degtiarenko]

Hello All,

I just pushed updates to the pull request

Target release
Probably it should be 2.0 (or 1.6?) as I am using newer api like getParameters from CommandCall and introducing new command looks like major change (Please correct me if I am wrong)

Assert equals and html formatting
I have updated tests to do assertEquals over html without cleaning formatting in background. As the result I had to copy generated formatting to expected of the tests, unfortunately sometimes it is hard to read.

Naming
I am still thinking about verificationStrategy name as the strategy classes not only determine matching between expected and actual rows but also do verification via verify() method. Anyway as for the user perspective of view it is indeed looks more like matching so renaming is done.

CommandCallList implements iterable
Tim, you are absolutely right that composition gives more flexibility over inheritance and this class indeed *has* a List of command calls, but it behaves as list and have a List in name even if it is not java.util.List. Current implementation of get and size method delegates to underlying list, same for the iterator method. If you still prefer not to implement iterable I may introduce subcommands() method to create iterator.

Please let me know If I miss something.

Regards,
Євген

[Tim Wright]

Hi Eugene,

There are actually two things with the Iterable for CommandCallList. The first is using a Collection instead of an Iterator. The second is composition v inheritance.

On the Collection side, Collection gives us access to all the Collection methods as well as Iterable methods - like "stream" in Java8 - whereas the Iterable interface just gives us the nice for each loop. So my preference is a Collection.

On the composition v inheritance, if we go with Collection then composition is easier - as there are lots and lots of methods in a collection.

There are longer term support implications of this as well - because CommandCallList is in the api class, we'll have to keep the composition v inheritance decision for some time - and these methods might change over time in different java versions. By having a method that returns a Collection, we can change what type of collection we return.

Those are my thoughts anyway :) Happy to keep debating...

Tim

--
You received this message because you are subscribed to the Google Groups "concordion-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to concordion-de...@googlegroups.com.
To post to this group, send email to concord...@googlegroups.com.
Visit this group at http://groups.google.com/group/concordion-dev.

To view this discussion on the web, visit https://groups.google.com/d/msgid/concordion-dev/CAOYmAgxwJk44uaY9a5sHhsoxfDJyLxV-gLqSnjkqoqYMVQyphw%40mail.gmail.com.

For more options, visit https://groups.google.com/d/optout.

[Eugene Degtiarenko]

Hi Tim,

With collection interface you fully convinced me. I'll commit it in a few moments. It is going to be unmodifiableList inside. What is the best name for this? components/subcomands/asCollection? I would like to avoid commands or list as it is already in type name.

Regards

[Tim Wright]

Hmm. SubCommands and components implies something different than just the CommandCallList as a collection, so I am leaning toward asCollection. 

Returning an unmodifiable list makes sense - or any clone of the list that ensures that a user of that method can't modify the internal list is good.

Tim

[Nigel Charman]

Thanks, comments inline:

On 12/09/15 12:48, Eugene Degtiarenko wrote:

Target release
Probably it should be 2.0 (or 1.6?) as I am using newer api like getParameters from CommandCall and introducing new command looks like major change (Please correct me if I am wrong)

Yes, my preference would still be for the 2.0 release, otherwise we'll have to start cherry picking some functionality for a 1.6 release. This will give us more impetus to get the 2.0 release out!

Assert equals and html formatting
I have updated tests to do assertEquals over html without cleaning formatting in background. As the result I had to copy generated formatting to expected of the tests, unfortunately sometimes it is hard to read.

I've just made a PR against your repo with an example of just including the table rows in the examples, and moving the main table definition to the top of the spec. I think this makes it more readable. See what you think.

Naming
I am still thinking about verificationStrategy name as the strategy classes not only determine matching between expected and actual rows but also do verification via verify() method. Anyway as for the user perspective of view it is indeed looks more like matching so renaming is done.

I see your point. Unless there's anything other than matching that we're going to vary, I think "matchStrategy" is fine.

CommandCall getParameterWithVariants
I'm not sure we need to add a new getParameterWithVariants() method. The getParameter() method is new for 2.0, so we could just change its signature to take the varargs of the variants. Alternatively, we could keep it with a single argument, and add some logic to check for the hyphenated version of any parameter names that are camel-cased. Tim - keen for your thoughts on this too.

Nigel.

[Nigel Charman]

Even after this change, I still think the specification takes too much time to grok - especially for casual Concordion users.

I had a crazy idea about pimping the spec up to allow you to view the rendered version of the fragments. I've created a version with a "View rendered" button at the top of the example:



Clicking on this shows the rendered view:



I'm keen for feedback on this. If it's something that users are keen to see carried forward to other specifications, it would be worth pulling the common code into an extension.

For now, the code for this experiment is at https://github.com/nigelcharman/concordion/commit/4f6afbd1863c6e178fd937994cfd9644d9e656fe.

cheers

On 13/09/15 19:23, Nigel Charman wrote:

[Eugene Degtiarenko]

Hi Tim,

I really like idea to show tables instead of html. I tried to start with this option (https://www.dropbox.com/s/10vm7imyvabqayu/MatchRowsWithMultipleColumnsTableView.pdf?dl=0) but I was unable to add my js to the result page without getting it escaped or added to the common concordion js.

If it is going to be tables output instead of html then I would like to show entire table html like in TableBodySupport.hyml. I realize it is going to be much bigger and also will include "copy-paste" header, but I believe it will be easier for understanding, where you do not need to scroll up and down to get complete table.

Thanks

[Nigel Charman]

Hi Eugene

I don't think we should show tables output instead of html, since the failures that are shown on the tables could be confused for failing tests.

I see it more as having a "Specification mode" and "Tutorial mode". By default we show the Specification mode, but the user is able to switch it to Tutorial mode. In Specification mode, all of the examples should be green. In Tutorial mode, some of the examples will be red, since the examples are of failing scenarios.

I've sent you a PR with my changes that you can use.

I also found the problem with the JS being escaped, and have created https://github.com/concordion/concordion/issues/124 for it. In my case, I could work around it by changing ">" to "!=". Another workaround may be to create the JavaScript in a file and add an extension to include the custom Javascript, similar to http://concordion.org/ExtensionsAPI.html#cssExample.

cheers

On 14/09/15 03:44, Eugene Degtiarenko wrote: