Suppress successfully matched rows. #203

marcusrehm · 2014-01-09T15:18:27Z

If the parameter "show wrong rows only" is passed as a fourth row in Compared Stored Queries, only the rows with unmatched values will be displayed.

This change does not affect existing tests, If the parameter is not passed, the fixture asumes it as false.

benilovj · 2014-01-09T15:42:29Z

@marcusrehm, thanks for the pull request. Here's some initial feedback:

The indentation in CompareStoredQueries is messed up. I've just reformatted it, so could I ask you to rebase against the current master and reapply your changes.
This would definitely need a documentation update. Please see the website subfolder for the documentation.

benilovj · 2014-01-09T15:43:07Z

dbfit-java/core/src/main/java/dbfit/DatabaseTest.java

+        }
+        else {
+            return new dbfit.fixture.CompareStoredQueries(environment,symbol1, symbol2, false);
+        }


this could be simplified to

return new dbfit.fixture.CompareStoredQueries(environment,symbol1, symbol2, "showwrongrowsonly".equals(normalname));

benilovj · 2014-01-09T15:44:30Z

I'd also like to at least have a testcase in the common FitNesse test suite that exercises this feature.

marcusrehm · 2014-01-10T17:33:07Z

Hi Jake,

I did the rebase and update the website folder with documentation, but is
not clear to me where are the common Fitnesse test suite. Could you guys
help with that?

2014/1/9 Jake Benilov notifications@github.com

I'd also like to at least have a testcase in the common FitNesse test
suite that exercises this feature.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/203#issuecomment-31944226
.

benilovj · 2014-01-10T17:37:40Z

website/_includes/manual/compare-stored-queries.md

+ For large amounts of rows, find rows with wrong matches can be difficult, in these cases should be better hide successfully matched rows. To accomplish it you can use `show wrong rows only`.
+
+ |compare stored queries|fromtable|fromdual|show wrong rows only|
+ |name                  |n?                |


~~missing a trailing | here~~

oops, apologies, what I meant was "could you please align this", thanks

benilovj · 2014-01-10T17:43:09Z

I did the rebase and update the website folder with documentation,

thanks, I've left some comments on the documentation.

but is not clear to me where are the common Fitnesse test suite. Could you guys help with that?

sorry, what I meant was the core tests, that are included in the test suites for several database adapters.

javornikolov · 2014-01-10T17:59:10Z

dbfit-java/core/src/main/java/dbfit/fixture/CompareStoredQueries.java

        this.symbol1 = symbol1;
        this.symbol2 = symbol2;
+        this.showWrongRowsOnly=showWrongRowsOnly;


Missing white space around = sign here.

Before reformatting and rebase, the file was this way.
Sorry, I didn't notice before.

javornikolov · 2014-01-11T11:27:44Z

I found a problem when trying to test that. I'm running tests like these: https://gist.github.com/javornikolov/8369736

I'm comparing same queries on same test page (First Test and Second Test). The output of First Test is influenced by First Test: if we have both tests, then the second one doesn't show all mismatches. If I comment/remove 1st - the the output of 2nd seems OK.

Seems the issue exists even without show worng rows only clause.

benilovj · 2014-01-11T11:33:18Z

@javornikolov that's worrying.

benilovj · 2014-01-11T11:40:45Z

@marcusrehm I've had some thoughts about my earlier comment.

I think having a magic string to enable successful row suppression might cause problems for users. For example, if the user mistypes the string:

|compare stored queries|fromtable|fromdual|hid matching rows|

(missing e for hide), then the code silently doesn't hide the matching rows.

I think it'll be better to instead do the following (similar to how it's done for execute procedure expect exceptions:

|compare stored queries hide matching rows|fromtable|fromdual|

that way, if the user mistypes, then they get an exception for fitnesse. What do you think?

javornikolov · 2014-01-11T12:31:06Z

I found a problem when trying to test that. I'm running tests like these: https://gist.github.com/javornikolov/8369736

I'm comparing same queries on same test page (First Test and Second Test). The output of First Test is influenced by First Test: if we have both tests, then the second one doesn't show all mismatches. If I comment/remove 1st - the the output of 2nd seems OK.

Seems the issue exists even without show wrong rows only clause.

This is because of the static DataTable symbols and their mutable state (processed flag).

marcusrehm · 2014-01-11T21:38:29Z

@benilovj , I thought about that too, but as it is a minor change I thought it best not to create a new method just to it, but as you said, it's better gets an exception than gets no warning and the behavior is not expected.

So I'm gonna change code to create a new method. Any suggestions about the name? Could be "Compare Stored Queries And Hide Matched Rows"?

benilovj · 2014-01-11T21:51:40Z

Any suggestions about the name?

Compare Stored Queries Hide Matching Rows

marcusrehm · 2014-01-11T22:18:51Z

And what if we let as it is now and rise a exception if the user types it wrong? Could be an option too.
What you guys think about it?

benilovj · 2014-01-11T22:25:27Z

And what if we let as it is now and rise a exception if the user types it wrong?

A new keyword is more consistent with how this has been done in other places (see execute statement) and the implementation is slightly nicer, so I'd prefer that option.

javornikolov · 2014-01-12T18:14:37Z

I'll reformat whitespace in DatabaseTest and I'll rebase & squash this PR onto upstream/master (to pick recent changes of CompareStoredQueries related to #205). I'll send my branch ref when I'm ready with these changes.

javornikolov · 2014-01-12T19:23:00Z

I'll reformat whitespace in DatabaseTest and I'll rebase & squash this PR onto upstream/master (to pick recent changes of CompareStoredQueries related to #205). I'll send my branch ref when I'm ready with these changes.

Here it is: suppress-succesfully-matched-rows-cleanup 384cd38

but is not clear to me where are the common Fitnesse test suite. Could you guys help with that?

sorry, what I meant was the core tests, that are included in the test suites for several database adapters.

The existing CompareStoredQueries tests have been moved to the common suite: CoreTests/CommonSuite/StoreAndCompareQueries. So now it's more clear where to add the tests for hide matching rows.

javornikolov · 2014-01-12T20:20:45Z

And what if we let as it is now and rise a exception if the user types it wrong?

A new keyword is more consistent with how this has been done in other places (see execute statement) and the implementation is slightly nicer, so I'd prefer that option.

This made me think of some more general scenarios when multiple options are a available. E.g. - if we compare queries we may want: ordered, hide matching rows, timeout=x, stop comparing on first mismatch, ignore missing right rows ...

But for a start I agree it's good to be consistent with the other similar fixtures.

marcusrehm · 2014-01-13T16:18:05Z

@benilovj , @javornikolov I did the rebase and created the CoreTests\CommonSuite\StoreAndCompareQueriesHideMatchingRows test too.

benilovj · 2014-01-13T16:35:48Z

...Fit/AcceptanceTests/CoreTests/CommonSuite/StoreAndCompareQueriesHideMatchingRows/content.txt

+
+!|Store Query|select * from TESTTBLA|q2|
+
+# Should appear no rows since both queries match all rows.


Now, this raises an interesting point. In the "success" case, no rows are returned from Compare Stored Queries Hide Matching Rows. In some cases, this will mean that there are no green cells in the test at all. Perhaps it's worth returning a green "summary" row if all rows are green. @javornikolov thoughts?

Yes, It's very strange when no rows are displayed and every thing is fine.

Can we make the test summary that appears at the top of resulting page
stays fixed with the breadcrumb?

2014/1/13 Jake Benilov notifications@github.com

In
FitNesseRoot/DbFit/AcceptanceTests/CoreTests/CommonSuite/StoreAndCompareQueriesHideMatchingRows/content.txt:

+|N |TWON |
+|1 |2 |
+|2 |4 |
+|3 |6 |
+
+!|Insert|TESTTBLA|
+|N |TWON |
+|1 |2 |
+|2 |4 |
+|3 |6 |
+
+!|Store Query|select * from TESTTBL|q1|
+
+!|Store Query|select * from TESTTBLA|q2|
+
+# Should appear no rows since both queries match all rows.

Now, this raises an interesting point. In the "success" case, no rows are
returned from Compare Stored Queries Hide Matching Rows. In some cases,
this will mean that there are no green cells in the test at all. Perhaps
it's worth returning a green "summary" row if all rows are green.
@javornikolov https://github.com/javornikolov thoughts?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/203/files#r8830783
.

A summary would be useful. Even without this hiding of matching rows: recently I had a case with too wide table where exception is in right-most column, outside of my screen (needed scrolling to see it).

I just committed a summary for the CompareStoredQueriesAndHideMatchingRows, now when no erros are found and no rows are displayed the result table will have a green line with the summary information containing totals of right, wrongs and exceptions, just like the one that appears at the top of the page.

javornikolov · 2014-01-13T20:19:26Z

@benilovj , @javornikolov I did the rebase

Hmm. My idea here was to just replace with the cleanup branch instead of merging it and fighting with some conflicts. The hope was to get rid of the superfluous noise related to reformatting, etc.

@marcusrehm, can you try again - this time pick from suppress-succesfully-matched-rows-cleanup-2 branch. First save your pending work, of course.

git branch a-backup-2
git fetch upstream suppress-succesfully-matched-rows-cleanup-2
git fetch upstream master
git reset --hard '36deb1d09a5e0eab3d9b5e4abfa7f6ecf33cf994'
git rebase upstream/suppress-succesfully-matched-rows-cleanup-2
git rebase upstream/master

And then you can continue working on top of that.

javornikolov · 2014-01-17T10:30:49Z

Adjusting and commiting it. But this one I think will do no harm as the condition to pass by here is all rows not added as they are all right, so it is not replacing anything.

OK. But it still relies on having specific number of rows in the rendered table which is a bit brittle. (Some day we may want to have an additional row for some reason).

How about pushing that summary as first row on the table - seems more stable approach. (And if we want to re-use that for other kind of tests /not hiding rows/ -> first row is easier to see). @marcusrehm , what do you think?

marcusrehm · 2014-01-17T12:12:47Z

Good ideia @javornikolov!
Actually it would be good if we have it both in the top and bottom of table. With large number of rows, scrolling up and down just to see the summary won't be any good.

I can add the summary on top of the table and let also at bottom too. What do you think?

javornikolov · 2014-01-17T13:16:45Z

Actually, in the context of current feature (hiding matching rows): I think a single summary header (or footer) would be enough. We can think of further enhancements of larger test tables when we target the other kinds of fixtures (in separate pull request(s)).

javornikolov · 2014-01-18T01:08:43Z

dbfit-java/core/src/main/java/dbfit/DatabaseTest.java

@@ -155,6 +155,10 @@ public Fixture compareStoredQueries(String symbol1, String symbol2) {
        return new dbfit.fixture.CompareStoredQueries(environment, symbol1, symbol2);
    }

+    public Fixture compareStoredQueriesHideMatchingRows(String symbol1, String symbol2) {
+        return new dbfit.fixture.CompareStoredQueriesHideMatchingRows(environment, symbol1, symbol2);    	


There is some trailing space here.

.last() is safer for attaching summary as last row

javornikolov · 2014-01-18T11:12:56Z

I added 2 commits on https://github.com/dbfit/dbfit/tree/supress-matching-rows with whitespace reformatting and changed summary row attachment to a bit safer version.

These 2 be merged and I think this PR is good enough to merge to master. @benilovj, @marcusrehm?

benilovj · 2014-01-18T13:16:42Z

@javornikolov do you propose that we merge this PR and then put your commits on top on master? Alternatively if you want to open a PR on https://github.com/dbfit/dbfit/tree/supress-matching-rows then we can merge that one and close this one.

javornikolov · 2014-01-18T13:46:14Z

It's best to have that as single merge I think. I opened separate PR. (I can't find ability in GH for changing, adding source/target branch for a PR...).

marcusrehm · 2014-01-18T23:18:14Z

Hi guys, sorry for the late reply, but my little daughther (20 days) didn't
give a break today.

Yes, I agree with you. I think it is good to merge to master.

The other improvements we talk about can be in future PR.
Em 18/01/2014 09:12, "Yavor Nikolov" notifications@github.com escreveu:

I added 2 commits on
https://github.com/dbfit/dbfit/tree/supress-matching-rows with whitespace
reformatting and changed summary row attachment to a bit safer version.

These 2 be merged and I think this PR is good enough to merge to master.
@benilovj https://github.com/benilovj, @marcusrehmhttps://github.com/marcusrehm
?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/203#issuecomment-32679620
.

javornikolov · 2014-01-18T23:40:30Z

@marcusrehm, if you're here you may merge (no rebase this time) from my branch: just do git pull upstream supress-matching-rows and push it to github.

marcusrehm · 2014-01-19T15:10:32Z

Done @javornikolov! Yours commits are now on my master branch.

benilovj · 2014-01-20T23:32:16Z

dbfit-java/core/src/main/java/dbfit/fixture/CompareStoredQueriesHideMatchingRows.java

+
+    public void doTable(Parse table) {
+        super.doTable(table);
+        if (counts.wrong == 0 && counts.exceptions == 0) {


what's the thinking behind only showing this when everything's OK? Is there a particular reason why we don't want to show the summary when matching fails?

As we are hiding successfully matched rows, in the case of all rows were matched there is nothing to display, so displaying just the table header would lead the user to wrong interpretation of results as we discussed earlier, so a summary was added to advise that no errors were found.

When matching fails the behavior is exactly the same as ComparedStoredQueries, so the summary is not needed.

benilovj · 2014-01-20T23:37:42Z

Apart from the comment about the conditional summary logic, this looks OK.

I'm not crazy about the fact that we're writing new code without unit tests. The FitNesse-level test only really shows that covers the happy path doesn't crash, and doesn't test any of the logic around mismatches, and doesn't pick up when the fixture doesn't display properly. The number of times that something regressed during the development of this change showed how easy it is to break things when there isn't a test harness to catch you.

In the spirit of moving matters forward, I will merge this change when the last comment is resolved, however for the next part of work I kinda feel unit tests are going to be the pre-requisite to getting it into the library.

marcusrehm · 2014-01-21T02:25:00Z

however for the next part of work I kinda feel unit tests are going to be the pre-requisite to getting it into the library.

I agree with you. In fact I found that there is no unit tests for fixtures while I was looking for a example to right it to ComparedStoredQueriesHideMatchingRows, maybe we can put it as techdebt on issue list. What do you think?

benilovj · 2014-01-21T08:30:24Z

maybe we can put it as techdebt on issue list. What do you think?

It's a sisyphean task - the core is pretty hard to test at the moment. I think a better strategy is to:

write unit tests (or better still, test-driven) code whenever new code is written
write unit-level tests around code being refactored.

benilovj · 2014-01-21T08:31:01Z

@marcusrehm @javornikolov I would probably like to remove the conditional logic for showing the summary row - any objections?

javornikolov · 2014-01-21T09:03:27Z

@marcusrehm @javornikolov I would probably like to remove the conditional logic for showing the summary row - any objections?

I agree on that. In case of mismatches - it would still help identifying the number of matched ones.

javornikolov · 2014-01-21T09:14:29Z

maybe we can put it as techdebt on issue list. What do you think?
It's a sisyphean task - the core is pretty hard to test at the moment. I think a better strategy is to:
write unit tests (or better still, test-driven) code whenever new code is written
write unit-level tests around code being refactored.

One of the goals around the on-going PR #213 was to help for easier testing. An idea for a test with not only happy paths: CompareStoredQueriesMatcherTest.testMismatchWithRightWrongSurplusAndMissing.

marcusrehm · 2014-01-21T10:51:39Z

@benilovj @javornikolov

I agree with you. I will remove the condition to show summary and will commit it.

…w summary information only when all rows matched. Now it will be shown every time test execute.

marcusrehm · 2014-01-21T13:42:21Z

@benilovj @javornikolov . Done!

Should I merge with upstream/master?

Suppress successfully matched rows.

benilovj · 2014-01-21T13:45:41Z

merged, thanks @marcusrehm and @javornikolov

benilovj reviewed Jan 9, 2014
View reviewed changes

benilovj reviewed Jan 10, 2014
View reviewed changes

javornikolov reviewed Jan 10, 2014
View reviewed changes

javornikolov mentioned this pull request Jan 11, 2014

Side effects on tests with Stored Queries are messing up subsequent tests with them #205

Closed

benilovj reviewed Jan 13, 2014
View reviewed changes

javornikolov reviewed Jan 18, 2014
View reviewed changes

javornikolov added 2 commits January 18, 2014 12:42

attach summary to .last() instead of .more.more

4e9943e

.last() is safer for attaching summary as last row

whitespace reformat

9b34e93

javornikolov mentioned this pull request Jan 18, 2014

Supress successfully matched rows #212

Merged

benilovj reviewed Jan 20, 2014
View reviewed changes

- CompareStoredQueriesAndHideMatchingQueries: remove condition to sho…

0803694

…w summary information only when all rows matched. Now it will be shown every time test execute.

benilovj added a commit that referenced this pull request Jan 21, 2014

Merge pull request #203 from marcusrehm/master

2e38c6f

Suppress successfully matched rows.

benilovj merged commit 2e38c6f into dbfit:master Jan 21, 2014


		!\|Store Query\|select * from TESTTBLA\|q2\|

		# Should appear no rows since both queries match all rows.

Suppress successfully matched rows. #203

Suppress successfully matched rows. #203

Conversation

marcusrehm commented Jan 9, 2014

benilovj commented Jan 9, 2014

Choose a reason for hiding this comment

benilovj commented Jan 9, 2014

marcusrehm commented Jan 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benilovj commented Jan 10, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javornikolov commented Jan 11, 2014

benilovj commented Jan 11, 2014

benilovj commented Jan 11, 2014

javornikolov commented Jan 11, 2014

marcusrehm commented Jan 11, 2014

benilovj commented Jan 11, 2014

marcusrehm commented Jan 11, 2014

benilovj commented Jan 11, 2014

javornikolov commented Jan 12, 2014

javornikolov commented Jan 12, 2014

javornikolov commented Jan 12, 2014

marcusrehm commented Jan 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

javornikolov commented Jan 13, 2014

javornikolov commented Jan 17, 2014

marcusrehm commented Jan 17, 2014

javornikolov commented Jan 17, 2014

Choose a reason for hiding this comment

javornikolov commented Jan 18, 2014

benilovj commented Jan 18, 2014

javornikolov commented Jan 18, 2014

marcusrehm commented Jan 18, 2014

javornikolov commented Jan 18, 2014

marcusrehm commented Jan 19, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benilovj commented Jan 20, 2014

marcusrehm commented Jan 21, 2014

benilovj commented Jan 21, 2014

benilovj commented Jan 21, 2014

javornikolov commented Jan 21, 2014

javornikolov commented Jan 21, 2014

marcusrehm commented Jan 21, 2014

marcusrehm commented Jan 21, 2014

benilovj commented Jan 21, 2014