[PATCH] test: make test_expect_equal_file() arguments flexible

[PATCH] test: make test_expect_equal_file() arguments flexible

[Dmitry Kurochkin]

Before the change, test_expect_equal_file() function treated the first
argument as "actual output file" and the second argument as "expected
output file".  When the test fails, the files are copied for later
inspection.  The first files was copied to "$testname.output" and the
second file to "$testname.expected".  The argument order for
test_expect_equal_file() is often wrong which results in confusing
diff output and incorrectly named files.

The patch solves the issue by changing test_expect_equal_file() to
treat arguments just as two files, without any special properties
(like "actual" and "expected").  The file names for copying is now
based on the given file name: "$testname.$file1" and
"$testname.$file2".  E.g. if test_expect_equal_file() is called with
"OUTPUT" and "EXPECTED", the copied files can be named
"emacs.1.OUTPUT" and "emacs.1.EXPECTED".

The down side of this approach is that diff argument order depends on
test_expect_equal_file() argument order.  So sometimes we get diff
from expected to actual results, and sometimes the other way around.
But the files are always named correctly.
---
 test/README      |   10 +++++-----
 test/test-lib.sh |   12 ++++++------
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/test/README b/test/README
index 43656a3..e0364e8 100644
--- a/test/README
+++ b/test/README
@@ -176,12 +176,12 @@ library for your script to use.
    will generate a failure and print the difference of the two
    strings.
 
- test_expect_equal_file <output> <expected>
+ test_expect_equal_file <file1> <file2>
 
-   Identical to test_exepect_equal, except that <output> and
-   <expected> are files instead of strings.  This is a much more
-   robust method to compare formatted textual information, since it
-   also notices whitespace and closing newline differences.
+   Identical to test_exepect_equal, except that <file1> and <file2>
+   are files instead of strings.  This is a much more robust method to
+   compare formatted textual information, since it also notices
+   whitespace and closing newline differences.
 
  test_debug <script>
 
diff --git a/test/test-lib.sh b/test/test-lib.sh
index 8158328..581b8be 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -488,17 +488,17 @@ test_expect_equal_file ()
 	test "$#" = 2 ||
 	error "bug in the test script: not 2 or 3 parameters to test_expect_equal"
 
-	output="$1"
-	expected="$2"
+	file1="$1"
+	file2="$2"
 	if ! test_skip "$test_subtest_name"
 	then
-		if diff -q "$expected" "$output" >/dev/null ; then
+		if diff -q "$file1" "$file2" >/dev/null ; then
 			test_ok_ "$test_subtest_name"
 		else
 			testname=$this_test.$test_count
-			cp "$output" $testname.output
-			cp "$expected" $testname.expected
-			test_failure_ "$test_subtest_name" "$(diff -u $testname.expected $testname.output)"
+			cp "$file1" "$testname.$file1"
+			cp "$file2" "$testname.$file2"
+			test_failure_ "$test_subtest_name" "$(diff -u "$testname.$file1" "$testname.$file2")"
 		fi
     fi
 }
-- 
1.7.9

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 4 bytes --]

+1.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 667 bytes --]

On Wed,  1 Feb 2012 11:19:54 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> The down side of this approach is that diff argument order depends on
> test_expect_equal_file() argument order.  So sometimes we get diff
> from expected to actual results, and sometimes the other way around.
> But the files are always named correctly.

Actually, I think this last point is the most important thing to retain.
Consistency in the diffs makes reading test results much more efficient.
The order I don't much care about.  But seeing as we have been
consistent with a particular order for a while, it seems like more
effort than it's worth to change it.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Tomi Ollila]

On Wed, 01 Feb 2012 00:47:30 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Wed,  1 Feb 2012 11:19:54 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > The down side of this approach is that diff argument order depends on
> > test_expect_equal_file() argument order.  So sometimes we get diff
> > from expected to actual results, and sometimes the other way around.
> > But the files are always named correctly.
> 
> Actually, I think this last point is the most important thing to retain.
> Consistency in the diffs makes reading test results much more efficient.
> The order I don't much care about.  But seeing as we have been
> consistent with a particular order for a while, it seems like more
> effort than it's worth to change it.

how about enforcing it in the test suite ?

case $file2 in 
	*.expected|*.EXPECTED) ;; 
        *) echo "$file2" does not end with 'expected' >&2; exit 1
esac

> 
> jamie.

Tomi

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Dmitry Kurochkin]

On Wed, 01 Feb 2012 00:47:30 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Wed,  1 Feb 2012 11:19:54 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > The down side of this approach is that diff argument order depends on
> > test_expect_equal_file() argument order.  So sometimes we get diff
> > from expected to actual results, and sometimes the other way around.
> > But the files are always named correctly.
> 
> Actually, I think this last point is the most important thing to retain.
> Consistency in the diffs makes reading test results much more efficient.
> The order I don't much care about.  But seeing as we have been
> consistent with a particular order for a while, it seems like more
> effort than it's worth to change it.
> 

It is not true that we are consistent with test_expect_equal_file()
argument order.  If we were, I would not bother.  The problem is we are
not.  I remember that we already fixed argument order for
test_expect_equal() and/or test_expect_equal_file().  If we do not solve
this problem, we should make it a tradition.

Consistent diff would be good.  But IMO the current situation is worse:
we are *supposed* to have consistent diff output, but in reality we have
messed diff output.

Also please consider the following points:

* Usually one is looking at a single failing test.  So it is not like
  you have a series of inconsistent diffs.

* I personally can not remember the argument and diff order.  So each
  time I need to understand the diff, I look at the beginning to see
  which side is where anyway.

So IMHO diff order is not that important.  But I would like to see a
better solution.  Perhaps Tomi's proposal would be the one.

Regards,
  Dmitry

> jamie.

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Dmitry Kurochkin]

On Wed, 01 Feb 2012 10:55:50 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> On Wed, 01 Feb 2012 00:47:30 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > On Wed,  1 Feb 2012 11:19:54 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > > The down side of this approach is that diff argument order depends on
> > > test_expect_equal_file() argument order.  So sometimes we get diff
> > > from expected to actual results, and sometimes the other way around.
> > > But the files are always named correctly.
> > 
> > Actually, I think this last point is the most important thing to retain.
> > Consistency in the diffs makes reading test results much more efficient.
> > The order I don't much care about.  But seeing as we have been
> > consistent with a particular order for a while, it seems like more
> > effort than it's worth to change it.
> 
> how about enforcing it in the test suite ?
> 
> case $file2 in 
> 	*.expected|*.EXPECTED) ;; 
>         *) echo "$file2" does not end with 'expected' >&2; exit 1
> esac
> 

It would break some existing tests.  But I like it.

Regards,
  Dmitry

> > 
> > jamie.
> 
> Tomi

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Tomi Ollila]

There are at least these options here

1) go through all ~100 places where test_expect_equal_file is used
   and fix the call order: quick look tells that the offending uses
   are in dump-restore, hooks, search-limiting and symbol-hiding.

2) enforce "expected" filename has some format *and* fix all current
   uses of it. Add testbed_error () function which yells loudly ane exits...

3) guess which is output and which is expected from args so that 
   machine helps tester here (for both diff output & copied files)a

4) just copy compared files to some directory, those are named as
   basename of the original -- diff order still inconsistent.


I'd just go with option 1 and fix new *violations* when stumble upon one.

Tomi

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Dmitry Kurochkin]

On Wed, 01 Feb 2012 12:18:08 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> 
> There are at least these options here
> 
> 1) go through all ~100 places where test_expect_equal_file is used
>    and fix the call order: quick look tells that the offending uses
>    are in dump-restore, hooks, search-limiting and symbol-hiding.
> 
> 2) enforce "expected" filename has some format *and* fix all current
>    uses of it. Add testbed_error () function which yells loudly ane exits...
> 
> 3) guess which is output and which is expected from args so that 
>    machine helps tester here (for both diff output & copied files)a
> 
> 4) just copy compared files to some directory, those are named as
>    basename of the original -- diff order still inconsistent.
> 
> 
> I'd just go with option 1 and fix new *violations* when stumble upon one.
> 

Option 1 does not solve the problem.  New violations would apper and
need to be fixed again.  I am for option 2.

Regards,
  Dmitry

> Tomi

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 1673 bytes --]

On Wed, 01 Feb 2012 14:37:53 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> On Wed, 01 Feb 2012 12:18:08 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > 
> > There are at least these options here
> > 
> > 1) go through all ~100 places where test_expect_equal_file is used
> >    and fix the call order: quick look tells that the offending uses
> >    are in dump-restore, hooks, search-limiting and symbol-hiding.
> > 
> > 2) enforce "expected" filename has some format *and* fix all current
> >    uses of it. Add testbed_error () function which yells loudly ane exits...
> > 
> > 3) guess which is output and which is expected from args so that 
> >    machine helps tester here (for both diff output & copied files)a
> > 
> > 4) just copy compared files to some directory, those are named as
> >    basename of the original -- diff order still inconsistent.
> > 
> > 
> > I'd just go with option 1 and fix new *violations* when stumble upon one.
> > 
> 
> Option 1 does not solve the problem.  New violations would apper and
> need to be fixed again.  I am for option 2.

How is enforcing use of a particular filename better and more robust
than enforcing argument order?  You'll still have to force an arbitrary
heuristic.  And you'll still be vulnerable to people messing up the file
names (which actually seems easier to get wrong than messing up the
order).  And you'll have to have more code to parse the argument
strings.  And you'll still get inconsistent diffs.

If this is really a problem, I vote for 1.  In general, I am not in
favor of making the test suite more complicated than it needs to be.

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Dmitry Kurochkin]

Hi Jameson.

On Wed, 01 Feb 2012 09:24:32 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> On Wed, 01 Feb 2012 14:37:53 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > On Wed, 01 Feb 2012 12:18:08 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > > 
> > > There are at least these options here
> > > 
> > > 1) go through all ~100 places where test_expect_equal_file is used
> > >    and fix the call order: quick look tells that the offending uses
> > >    are in dump-restore, hooks, search-limiting and symbol-hiding.
> > > 
> > > 2) enforce "expected" filename has some format *and* fix all current
> > >    uses of it. Add testbed_error () function which yells loudly ane exits...
> > > 
> > > 3) guess which is output and which is expected from args so that 
> > >    machine helps tester here (for both diff output & copied files)a
> > > 
> > > 4) just copy compared files to some directory, those are named as
> > >    basename of the original -- diff order still inconsistent.
> > > 
> > > 
> > > I'd just go with option 1 and fix new *violations* when stumble upon one.
> > > 
> > 
> > Option 1 does not solve the problem.  New violations would apper and
> > need to be fixed again.  I am for option 2.
> 
> How is enforcing use of a particular filename better and more robust
> than enforcing argument order?

Filename check is a way to make sure the argument order is correct.

>  You'll still have to force an arbitrary
> heuristic.  And you'll still be vulnerable to people messing up the file
> names (which actually seems easier to get wrong than messing up the
> order).

Do you mean that people would start writing tests with filenames like:

  test_expect_equal_file EXPECTED1 EXPECTED2

?  That is possible, of course.  But do you seriously believe that
deliberately changing file names in a way that violates common sense and
is inconsistent with all other code is "easier" than writing "EXPECTED
OUTPUT" in the wrong order?  I do not think so.  And it would definately
be easier to catch during review.

>  And you'll have to have more code to parse the argument
> strings.

That is one case statement, with one non-empty case, with one error
call.

>  And you'll still get inconsistent diffs.
> 

No, you do not.  That is the point.

> If this is really a problem, I vote for 1.  In general, I am not in
> favor of making the test suite more complicated than it needs to be.
> 

Ok.  I plan to send a patch soon.  If my arguments do not convince
enough people, I will move along.

Regards,
  Dmitry

> jamie.

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Dmitry Kurochkin]

On Thu, 02 Feb 2012 03:42:29 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Hi Jameson.
> 
> On Wed, 01 Feb 2012 09:24:32 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > On Wed, 01 Feb 2012 14:37:53 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> > > On Wed, 01 Feb 2012 12:18:08 +0200, Tomi Ollila <tomi.ollila@iki.fi> wrote:
> > > > 
> > > > There are at least these options here
> > > > 
> > > > 1) go through all ~100 places where test_expect_equal_file is used
> > > >    and fix the call order: quick look tells that the offending uses
> > > >    are in dump-restore, hooks, search-limiting and symbol-hiding.
> > > > 
> > > > 2) enforce "expected" filename has some format *and* fix all current
> > > >    uses of it. Add testbed_error () function which yells loudly ane exits...
> > > > 
> > > > 3) guess which is output and which is expected from args so that 
> > > >    machine helps tester here (for both diff output & copied files)a
> > > > 
> > > > 4) just copy compared files to some directory, those are named as
> > > >    basename of the original -- diff order still inconsistent.
> > > > 
> > > > 
> > > > I'd just go with option 1 and fix new *violations* when stumble upon one.
> > > > 
> > > 
> > > Option 1 does not solve the problem.  New violations would apper and
> > > need to be fixed again.  I am for option 2.
> > 
> > How is enforcing use of a particular filename better and more robust
> > than enforcing argument order?
> 
> Filename check is a way to make sure the argument order is correct.
> 
> >  You'll still have to force an arbitrary
> > heuristic.  And you'll still be vulnerable to people messing up the file
> > names (which actually seems easier to get wrong than messing up the
> > order).
> 
> Do you mean that people would start writing tests with filenames like:
> 
>   test_expect_equal_file EXPECTED1 EXPECTED2
> 
> ?  That is possible, of course.  But do you seriously believe that
> deliberately changing file names in a way that violates common sense and
> is inconsistent with all other code is "easier" than writing "EXPECTED
> OUTPUT" in the wrong order?  I do not think so.  And it would definately
> be easier to catch during review.
> 
> >  And you'll have to have more code to parse the argument
> > strings.
> 
> That is one case statement, with one non-empty case, with one error
> call.
> 
> >  And you'll still get inconsistent diffs.
> > 
> 
> No, you do not.  That is the point.
> 
> > If this is really a problem, I vote for 1.  In general, I am not in
> > favor of making the test suite more complicated than it needs to be.
> > 
> 
> Ok.  I plan to send a patch soon.  If my arguments do not convince
> enough people, I will move along.
> 

The new patches, based on idea suggested by Tomi [1], are here:

  id:"1328141050-30356-1-git-send-email-dmitry.kurochkin@gmail.com"

Regards,
  Dmitry

[1] id:"m28vknaq5l.fsf@guru.guru-group.fi"

> Regards,
>   Dmitry
> 
> > jamie.

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[David Edmondson]

[-- Attachment #1: Type: text/plain, Size: 419 bytes --]

On Wed, 01 Feb 2012 09:24:32 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> If this is really a problem, I vote for 1.  In general, I am not in
> favor of making the test suite more complicated than it needs to be.

After listening to the debate, I agree. The documentation should state
that the order is 'expected actual' (or the other way around) and
offenders should be shot on sight^W^W^Wfixed.

[-- Attachment #2: Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Tomi Ollila]

On Thu, 02 Feb 2012 14:33:56 +0000, David Edmondson <dme@dme.org> wrote:
> On Wed, 01 Feb 2012 09:24:32 -0800, Jameson Graef Rollins <jrollins@finestructure.net> wrote:
> > If this is really a problem, I vote for 1.  In general, I am not in
> > favor of making the test suite more complicated than it needs to be.
> 
> After listening to the debate, I agree. The documentation should state
> that the order is 'expected actual' (or the other way around) and
> offenders should be shot on sight^W^W^Wfixed.

I've started to agree with Dmitry.

Why do something that computer can do -- to guide test writers to
give args in consistent order and provide suitable filenames.

Secondly as the output files are provided for human consumption
if there is consistent naming in expected output files helps 
developers getting parts of the big picture easier and finding 
right filenames easier.

... and the reviewers doesn't need to keep their plasma guns
handly.

Tomi

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[Jameson Graef Rollins]

[-- Attachment #1: Type: text/plain, Size: 901 bytes --]

On Wed,  1 Feb 2012 11:19:54 +0400, Dmitry Kurochkin <dmitry.kurochkin@gmail.com> wrote:
> Before the change, test_expect_equal_file() function treated the first
> argument as "actual output file" and the second argument as "expected
> output file".  When the test fails, the files are copied for later
> inspection.  The first files was copied to "$testname.output" and the
> second file to "$testname.expected".  The argument order for
> test_expect_equal_file() is often wrong which results in confusing
> diff output and incorrectly named files.

After thinking about this some more, I'm taking it all back.  I think
this is a fine solution, since it just goes with whatever name the test
is invoked with.  Since this is usually just OUTPUT and EXPECTED, it
should all be clear, and the diffs should be fine, even if the ordering
is permuted some places.  Sorry about all the chatter.

+1

jamie.

[-- Attachment #2: Type: application/pgp-signature, Size: 835 bytes --]

Re: [PATCH] test: make test_expect_equal_file() arguments flexible

[David Bremner]

Dmitry Kurochkin <dmitry.kurochkin@gmail.com> writes:

> Before the change, test_expect_equal_file() function treated the first
> argument as "actual output file" and the second argument as "expected
> output file".  When the test fails, the files are copied for later
> inspection.  The first files was copied to "$testname.output" and the
> second file to "$testname.expected".  The argument order for
> test_expect_equal_file() is often wrong which results in confusing
> diff output and incorrectly named files.

pushed, 

d