Patchwork test case fixes for Solaris

login
register
mail settings
Submitter Danek Duvall
Date Aug. 21, 2013, 8:50 p.m.
Message ID <20130821205014.GD2367@smelly.us.oracle.com>
Download mbox | patch
Permalink /patch/2236/
State Changes Requested
Headers show

Comments

Danek Duvall - Aug. 21, 2013, 8:50 p.m.
# HG changeset patch
# User Danek Duvall <danek.duvall@oracle.com>
# Date 1377115773 25200
# Node ID 5e7ec26663958bc8fc51ee3ab517274268b4f498
# Parent  6ac206fb6f27492a98f46bbff090407ee1b1de72
test case fixes for Solaris

Solaris versions of grep, diff, tail, and sed are different from the GNU
versions, causing some test cases to fail.  Work around these differences
in OS-independent ways.
Matt Mackall - Aug. 21, 2013, 9:18 p.m.
On Wed, 2013-08-21 at 13:50 -0700, Danek Duvall wrote:
> # HG changeset patch
> # User Danek Duvall <danek.duvall@oracle.com>
> # Date 1377115773 25200
> # Node ID 5e7ec26663958bc8fc51ee3ab517274268b4f498
> # Parent  6ac206fb6f27492a98f46bbff090407ee1b1de72
> test case fixes for Solaris
> 
> Solaris versions of grep, diff, tail, and sed are different from the GNU
> versions, causing some test cases to fail.  Work around these differences
> in OS-independent ways.

Horrors. How did these persist so long?

I think we probably shouldn't accept these without corresponding checks
in contrib/check-code.py otherwise these will just be recurring problems
that people on more mainstream Unices won't notice. Please separate out
each class of breakage into a separate patch and include a corresponding
check.

Also, please start your patch summaries with a relevant keyword, like
"solaris:".
Danek Duvall - Aug. 21, 2013, 9:41 p.m.
Matt Mackall wrote:

> On Wed, 2013-08-21 at 13:50 -0700, Danek Duvall wrote:
> > # HG changeset patch
> > # User Danek Duvall <danek.duvall@oracle.com>
> > # Date 1377115773 25200
> > # Node ID 5e7ec26663958bc8fc51ee3ab517274268b4f498
> > # Parent  6ac206fb6f27492a98f46bbff090407ee1b1de72
> > test case fixes for Solaris
> > 
> > Solaris versions of grep, diff, tail, and sed are different from the GNU
> > versions, causing some test cases to fail.  Work around these differences
> > in OS-independent ways.
> 
> Horrors. How did these persist so long?

Because it's been a long time since I've done an upgrade of Mercurial in
Solaris, and I haven't been running the bits in between myself (and I guess
no one else is running the tests on Solaris, or cares).  That's largely due
to the need each time to upgrade this internal extension that we have, and
fix up the man pages to work with the Solaris *roff tools, which is a
tedious process, but I've made some changes to my build process that should
make the latter significantly easier.

I do want to set up automated nightly builds on Solaris, but it's been low
on my priority list.

> I think we probably shouldn't accept these without corresponding checks
> in contrib/check-code.py otherwise these will just be recurring problems
> that people on more mainstream Unices won't notice. Please separate out
> each class of breakage into a separate patch and include a corresponding
> check.

Okay.  Can I consider this confirmation that the workarounds I used were
appropriate?  I considered submitting some check-code fixes to find these,
but had some questions:

  - I know that GNU head and tail at some point in the past started
    disallowing args like "-8" and +8", and required the use of -n, but
    then I think backed off of that change.  So I'm not sure what the right
    thing is here.

  - I have no idea how to catch the problem in test-parse-date.t, since the
    contextual link between the missing newline and the sed that might not
    be able to recognize the last line of a file without it is tenuous.  Is
    that something we can just leave to more frequent testing?

> Also, please start your patch summaries with a relevant keyword, like
> "solaris:".

Sure.

Thanks,
Danek
Matt Mackall - Aug. 21, 2013, 9:54 p.m.
On Wed, 2013-08-21 at 14:41 -0700, Danek Duvall wrote:
> Matt Mackall wrote:
> 
> > On Wed, 2013-08-21 at 13:50 -0700, Danek Duvall wrote:
> > > # HG changeset patch
> > > # User Danek Duvall <danek.duvall@oracle.com>
> > > # Date 1377115773 25200
> > > # Node ID 5e7ec26663958bc8fc51ee3ab517274268b4f498
> > > # Parent  6ac206fb6f27492a98f46bbff090407ee1b1de72
> > > test case fixes for Solaris
> > > 
> > > Solaris versions of grep, diff, tail, and sed are different from the GNU
> > > versions, causing some test cases to fail.  Work around these differences
> > > in OS-independent ways.
> > 
> > Horrors. How did these persist so long?
> 
> Because it's been a long time since I've done an upgrade of Mercurial in
> Solaris, and I haven't been running the bits in between myself (and I guess
> no one else is running the tests on Solaris, or cares).

I would have thought the Mercurial-on-Solaris community would be larger
than the set using Cadmium.

> > I think we probably shouldn't accept these without corresponding checks
> > in contrib/check-code.py otherwise these will just be recurring problems
> > that people on more mainstream Unices won't notice. Please separate out
> > each class of breakage into a separate patch and include a corresponding
> > check.
> 
> Okay.  Can I consider this confirmation that the workarounds I used were
> appropriate?

Well I can't say I was super-thrilled by them, as we've been trying to
remove filtery-hackery. I'd rather just replace the problem bits with
approaches that avoid the problem. Diff doesn't work portably? If the
files are small, how about cat?

But even where that's done, we'll want the check-code rule to avoid
creep-back.

>   - I know that GNU head and tail at some point in the past started
>     disallowing args like "-8" and +8", and required the use of -n, but
>     then I think backed off of that change.  So I'm not sure what the right
>     thing is here.

Probably complains on some machines still.

>   - I have no idea how to catch the problem in test-parse-date.t, since the
>     contextual link between the missing newline and the sed that might not
>     be able to recognize the last line of a file without it is tenuous.  Is
>     that something we can just leave to more frequent testing?
> 
> > Also, please start your patch summaries with a relevant keyword, like
> > "solaris:".
> 
> Sure.
> 
> Thanks,
> Danek
Danek Duvall - Aug. 21, 2013, 10:14 p.m.
Matt Mackall wrote:

> I would have thought the Mercurial-on-Solaris community would be larger
> than the set using Cadmium.

I would expect so, too, but I just don't know.  I guess another possibility
is that they're putting /usr/gnu/bin first in $PATH, in which case this all
runs just fine (at least on S11).

> > > I think we probably shouldn't accept these without corresponding checks
> > > in contrib/check-code.py otherwise these will just be recurring problems
> > > that people on more mainstream Unices won't notice. Please separate out
> > > each class of breakage into a separate patch and include a corresponding
> > > check.
> > 
> > Okay.  Can I consider this confirmation that the workarounds I used were
> > appropriate?
> 
> Well I can't say I was super-thrilled by them, as we've been trying to
> remove filtery-hackery. I'd rather just replace the problem bits with
> approaches that avoid the problem. Diff doesn't work portably? If the
> files are small, how about cat?

Seems to be "diff -u" specifically when there's no difference.  We could
make the rule that when you expect no difference, you use plain "diff", or
"cmp", or something, though of course when something goes wrong, and there
*is* a difference, you either get less useful output (context diffs) or no
output at all (cmp).  test-command-template.t uses both cmp and diff,
running diff only if cmp finds differences.

I could write python code using difflib, but that seems like overkill.

I probably would gravitate towards using context diffs for no output
(assuming that you can write a test that checks both the command and its
output), or the cmp/diff combo if I can figure out that regex.  But I'm not
sure I'm qualified to make the decision here.

I'm really not sure how else to work around the lack of grep -a.

> But even where that's done, we'll want the check-code rule to avoid
> creep-back.
> 
> >   - I know that GNU head and tail at some point in the past started
> >     disallowing args like "-8" and +8", and required the use of -n, but
> >     then I think backed off of that change.  So I'm not sure what the right
> >     thing is here.
> 
> Probably complains on some machines still.

Right.  So if we have to deal with both, then we need an alternative to
tail.  You can use "sed -n 'x,$p'" as an alternative to "tail +x", but I'm
not sure how to emulate the "normal" tail operation with other shell
commands.

Again, I could write this in python, but ...

So I'd welcome your (or anyone else's) suggestions.

Thanks,
Danek
Matt Mackall - Aug. 22, 2013, 8:06 p.m.
On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote:
> Seems to be "diff -u" specifically when there's no difference.  We could
> make the rule that when you expect no difference, you use plain "diff", or
> "cmp", or something, though of course when something goes wrong, and there
> *is* a difference, you either get less useful output (context diffs) or no
> output at all (cmp).  test-command-template.t uses both cmp and diff,
> running diff only if cmp finds differences.

I'm not super-worried about the case where the test fails and we get
weird output.

> I could write python code using difflib, but that seems like overkill.

Indeed.

> I probably would gravitate towards using context diffs for no output
> (assuming that you can write a test that checks both the command and its
> output), or the cmp/diff combo if I can figure out that regex.  But I'm not
> sure I'm qualified to make the decision here.
> 
> I'm really not sure how else to work around the lack of grep -a.

Perhaps use our inline Python support:

$ foo > bar
>>> for line in open('bar'):
...     if 'baz' in line: print line

> > But even where that's done, we'll want the check-code rule to avoid
> > creep-back.
> > 
> > >   - I know that GNU head and tail at some point in the past started
> > >     disallowing args like "-8" and +8", and required the use of -n, but
> > >     then I think backed off of that change.  So I'm not sure what the right
> > >     thing is here.
> > 
> > Probably complains on some machines still.
> 
> Right.  So if we have to deal with both, then we need an alternative to
> tail.  You can use "sed -n 'x,$p'" as an alternative to "tail +x", but I'm
> not sure how to emulate the "normal" tail operation with other shell
> commands.

Looks like the two places that are using it are legacy from before we
had glob/regex support built into the test format.
Danek Duvall - Aug. 22, 2013, 11:02 p.m.
Matt Mackall wrote:

> On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote:
> > Seems to be "diff -u" specifically when there's no difference.  We could
> > make the rule that when you expect no difference, you use plain "diff", or
> > "cmp", or something, though of course when something goes wrong, and there
> > *is* a difference, you either get less useful output (context diffs) or no
> > output at all (cmp).  test-command-template.t uses both cmp and diff,
> > running diff only if cmp finds differences.
> 
> I'm not super-worried about the case where the test fails and we get
> weird output.

Okay.

> > I probably would gravitate towards using context diffs for no output
> > (assuming that you can write a test that checks both the command and its
> > output), or the cmp/diff combo if I can figure out that regex.  But I'm not
> > sure I'm qualified to make the decision here.
> > 
> > I'm really not sure how else to work around the lack of grep -a.
> 
> Perhaps use our inline Python support:
> 
> $ foo > bar
> >>> for line in open('bar'):
> ...     if 'baz' in line: print line

Nice; done.

> > > But even where that's done, we'll want the check-code rule to avoid
> > > creep-back.
> > > 
> > > >   - I know that GNU head and tail at some point in the past started
> > > >     disallowing args like "-8" and +8", and required the use of -n, but
> > > >     then I think backed off of that change.  So I'm not sure what the right
> > > >     thing is here.
> > > 
> > > Probably complains on some machines still.
> > 
> > Right.  So if we have to deal with both, then we need an alternative to
> > tail.  You can use "sed -n 'x,$p'" as an alternative to "tail +x", but I'm
> > not sure how to emulate the "normal" tail operation with other shell
> > commands.
> 
> Looks like the two places that are using it are legacy from before we
> had glob/regex support built into the test format.

Not sure I follow how that would allow us to pipe a set of N lines of a
command's output into a file.  In particular, I don't see how tail's use in
test-import-merge.t can be converted to that.

I'll also note that "tail -2" is already used in test-copy.t (changed to
remove -n back in 2006, apparently on request from a Sun engineer) and
hasn't caused any problems.  Plus "head" is used in numerous places without
-n.  Though I'll note that on Solaris, for some idiotic reason
/usr/bin/head supports -n, while /usr/bin/tail doesn't.  It's been that way
as far back as I can tell (1994 or so), so I suppose it's possible that for
whatever time period the GNU versions required -n, they may have only done
so for one command and not the other for similarly inscrutable reasons.

At any rate, given the rampant disuse of -n, should I just leave this
change alone and add a test to prevent it from coming back?

Thanks,
Danek
Danek Duvall - Aug. 22, 2013, 11:13 p.m.
Danek Duvall wrote:

> Matt Mackall wrote:
> 
> > On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote:
> > > Seems to be "diff -u" specifically when there's no difference.  We could
> > > make the rule that when you expect no difference, you use plain "diff", or
> > > "cmp", or something, though of course when something goes wrong, and there
> > > *is* a difference, you either get less useful output (context diffs) or no
> > > output at all (cmp).  test-command-template.t uses both cmp and diff,
> > > running diff only if cmp finds differences.
> > 
> > I'm not super-worried about the case where the test fails and we get
> > weird output.
> 
> Okay.

Oh, but I'm still not sure what to do for a test here, as "diff -u" is just
fine as long as there's output; it's only if there is none that it's
problematic.  Usage is all over the place:

  - three instances of using "cmp" before "diff -u"

  - one instance of piping "diff -u" through "grep"

  - one instance of actually expecting output

  - three instances of expecting no output (I missed one, since the bzr tests
    aren't run on my machine)

Seems like the simplest testable option is to convert all calls to diff to
not use -u, remove all the cmp/grep calls, and convert the one bit of
output to be context diffs.  Does that work for you?

Thanks,
Danek
Danek Duvall - Aug. 22, 2013, 11:17 p.m.
Danek Duvall wrote:

> Danek Duvall wrote:
> 
> > Matt Mackall wrote:
> > 
> > > On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote:
> > > > Seems to be "diff -u" specifically when there's no difference.  We could
> > > > make the rule that when you expect no difference, you use plain "diff", or
> > > > "cmp", or something, though of course when something goes wrong, and there
> > > > *is* a difference, you either get less useful output (context diffs) or no
> > > > output at all (cmp).  test-command-template.t uses both cmp and diff,
> > > > running diff only if cmp finds differences.
> > > 
> > > I'm not super-worried about the case where the test fails and we get
> > > weird output.
> > 
> > Okay.
> 
> Oh, but I'm still not sure what to do for a test here, as "diff -u" is just
> fine as long as there's output; it's only if there is none that it's
> problematic.  Usage is all over the place:
> 
>   - three instances of using "cmp" before "diff -u"
> 
>   - one instance of piping "diff -u" through "grep"
> 
>   - one instance of actually expecting output
> 
>   - three instances of expecting no output (I missed one, since the bzr tests
>     aren't run on my machine)
> 
> Seems like the simplest testable option is to convert all calls to diff to
> not use -u, remove all the cmp/grep calls, and convert the one bit of
> output to be context diffs.  Does that work for you?

Dammit, I need to investigate more before hitting send.  The one instance
in test-glog.t of diff -u is in a function which is used dozens of times,
numerous ones with actual output.  Should I convert them all?

Thanks,
Danek
Matt Mackall - Aug. 22, 2013, 11:48 p.m.
On Thu, 2013-08-22 at 16:13 -0700, Danek Duvall wrote:
> Danek Duvall wrote:
> 
> > Matt Mackall wrote:
> > 
> > > On Wed, 2013-08-21 at 15:14 -0700, Danek Duvall wrote:
> > > > Seems to be "diff -u" specifically when there's no difference.  We could
> > > > make the rule that when you expect no difference, you use plain "diff", or
> > > > "cmp", or something, though of course when something goes wrong, and there
> > > > *is* a difference, you either get less useful output (context diffs) or no
> > > > output at all (cmp).  test-command-template.t uses both cmp and diff,
> > > > running diff only if cmp finds differences.
> > > 
> > > I'm not super-worried about the case where the test fails and we get
> > > weird output.
> > 
> > Okay.
> 
> Oh, but I'm still not sure what to do for a test here, as "diff -u" is just
> fine as long as there's output; it's only if there is none that it's
> problematic.  Usage is all over the place:
> 
>   - three instances of using "cmp" before "diff -u"

Don't care.

>   - one instance of piping "diff -u" through "grep"

Ugh.

>   - one instance of actually expecting output

This sort of thing can be replaced with cat as the files are one line.

>   - three instances of expecting no output (I missed one, since the bzr tests
>     aren't run on my machine)

Ok.

> Seems like the simplest testable option is to convert all calls to diff to
> not use -u, remove all the cmp/grep calls, and convert the one bit of
> output to be context diffs.  Does that work for you?

Sounds good.

Patch

diff --git a/tests/test-hgweb-commands.t b/tests/test-hgweb-commands.t
--- a/tests/test-hgweb-commands.t
+++ b/tests/test-hgweb-commands.t
@@ -1256,8 +1256,8 @@  commit message with Japanese Kanji 'Noh'
 Graph json escape of multibyte character
 
   $ "$TESTDIR/get-with-headers.py" 127.0.0.1:$HGPORT 'graph/' \
-  >     | grep -a '^var data ='
-  var data = [["061dd13ba3c3", [0, 1], [[0, 0, 1, -1, ""]], "\\u80fd", "test", "1970-01-01", ["unstable", true], ["tip"], ["something"]], ["cad8025a2e87", [0, 1], [[0, 0, 1, 3, "FF0000"]], "branch commit with null character: \x00", "test", "1970-01-01", ["unstable", false], [], []], ["1d22e65f027e", [0, 1], [[0, 0, 1, 3, ""]], "branch", "test", "1970-01-01", ["stable", true], [], []], ["a4f92ed23982", [0, 1], [[0, 0, 1, 3, ""]], "Added tag 1.0 for changeset 2ef0ac749a14", "test", "1970-01-01", ["default", true], [], []], ["2ef0ac749a14", [0, 1], [], "base", "test", "1970-01-01", ["default", false], ["1.0"], ["anotherthing"]]]; (esc)
+  >     | cat -v | grep '^var data ='
+  var data = [["061dd13ba3c3", [0, 1], [[0, 0, 1, -1, ""]], "\\u80fd", "test", "1970-01-01", ["unstable", true], ["tip"], ["something"]], ["cad8025a2e87", [0, 1], [[0, 0, 1, 3, "FF0000"]], "branch commit with null character: ^@", "test", "1970-01-01", ["unstable", false], [], []], ["1d22e65f027e", [0, 1], [[0, 0, 1, 3, ""]], "branch", "test", "1970-01-01", ["stable", true], [], []], ["a4f92ed23982", [0, 1], [[0, 0, 1, 3, ""]], "Added tag 1.0 for changeset 2ef0ac749a14", "test", "1970-01-01", ["default", true], [], []], ["2ef0ac749a14", [0, 1], [], "base", "test", "1970-01-01", ["default", false], ["1.0"], ["anotherthing"]]]; (esc)
 
 capabilities
 
diff --git a/tests/test-highlight.t b/tests/test-highlight.t
--- a/tests/test-highlight.t
+++ b/tests/test-highlight.t
@@ -522,7 +522,7 @@  hgweb fileannotate, raw
   $ echo "" >> b
   $ echo "" >> b
   $ echo "" >> b
-  $ diff -u b a
+  $ diff -u b a | grep '^[-+@ ]' || :
 
 hgweb filerevision, raw
 
@@ -531,7 +531,7 @@  hgweb filerevision, raw
   $ echo "200 Script output follows" > b
   $ echo "" >> b
   $ hg cat primes.py >> b
-  $ diff -u b a
+  $ diff -u b a | grep '^[-+@ ]' || :
 
 hgweb highlightcss friendly
 
diff --git a/tests/test-import-merge.t b/tests/test-import-merge.t
--- a/tests/test-import-merge.t
+++ b/tests/test-import-merge.t
@@ -129,7 +129,7 @@  Test that --exact on a bad header doesn'
   $ echo a>>a
   $ hg ci -m3
   $ hg export 2 | head -7 > ../a.patch
-  $ hg export tip | tail -n +8 >> ../a.patch
+  $ hg export tip | tail +8 >> ../a.patch
 
   $ cd ..
   $ hg clone -qr0 repo3 repo3-clone
diff --git a/tests/test-parse-date.t b/tests/test-parse-date.t
--- a/tests/test-parse-date.t
+++ b/tests/test-parse-date.t
@@ -242,7 +242,7 @@  Test issue 3764 (interpreting 'today' an
   >>> yesterday = (datetime.date.today() - datetime.timedelta(days=1)).strftime("%b %d")
   >>> dates = open('dates', 'w')
   >>> dates.write(today + '\n')
-  >>> dates.write(yesterday)
+  >>> dates.write(yesterday + '\n')
   >>> dates.close()
   $ hg ci -d "`sed -n '1p' dates`" -m "today is a good day to code"
   $ hg log -d today --template '{desc}\n'