Re: factor tests: ugly long file names

[Stefano Lattarini]

Hi Bernhard.

On 10/27/2012 02:05 AM, Bernhard Voelker wrote:
> On 10/26/2012 07:55 PM, Stefano Lattarini wrote:
>> On 10/26/2012 06:45 PM, Bernhard Voelker wrote:
>>> WDYT?
>>>
>>> Not tested very thoroughly, but it seems to work.
>>>
>>> Have a nice day,
>>> Berny
>>>
>>> From d85f748f4cad3fadcc699c575891259f5122fddb Mon Sep 17 00:00:00 2001
>>> From: Bernhard Voelker <address@hidden>
>>> Date: Fri, 26 Oct 2012 18:40:25 +0200
>>> Subject: [PATCH] maint: refactor new tests for refactored factor
>>>
>> Wow, a tongue-twister summary line :-)
> 
> Thanks for the feedback.
> 
>> On the serious side, I've some nits below.
> 
> The patch wasn't meant to be ready to be pushed yet - just a working
> outline of the idea to move the factor-related test data into the
> tests/factor/ directory where it belongs.
>
Ah, OK.  I hadn't got that actually; so thanks for the clarification.

> Ok, lets see ...
> 
>>> * tests/local.mk (EXTRA_DIST): Add new tests/factor/setup.sh.
>>> (factor_tests): Rename the test names to t{00..36}.sh. Factor out
>>> the test data triples out.
>>> (p,q,t1,t2) Factor out the related magic numbers.
>>> $(factor_tests): Add dependency to new tests/factor/setup.sh.
>>> Call that script to generate the test scripts.
>>> * tests/factor/setup.sh: Add new script to create the test script
>>> from the template tests/factor/run.sh.
>>> Use test data and magic number factored out from above.
>>> * tests/factor/run.sh: Change this script into a real template.
>>> Add template variables START, END and CKSUM, replacing the code
>>> to split the test data from the test script's file name.
>>> Use the new template variables in the call to seq and for
>>> creating the exp file.
>>> ---
>>>  tests/factor/run.sh   |   17 ++++-----
>>>  tests/factor/setup.sh |   89 
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>  tests/local.mk        |   67 +++++++------------------------------
>>>  3 files changed, 108 insertions(+), 65 deletions(-)
>>>  create mode 100755 tests/factor/setup.sh
>>>
>>> diff --git a/tests/factor/run.sh b/tests/factor/run.sh
>>> index 6ff24c3..ed865ff 100755
>>> --- a/tests/factor/run.sh
>>> +++ b/tests/factor/run.sh
>>> @@ -1,9 +1,7 @@
>>>  #!/bin/sh
>>>  # Test the factor rewrite.
>>> -# Expect to be invoked via a file whose basename matches
>>> -# /^(\d+)\-(\d+)\-([\da-f]{40})\.sh$/
>>>  # The test is to run this command
>>> -# seq $1 $2 | factor | shasum -c --status <(echo $3  -)
>>> +# seq $START $END | factor | shasum -c --status <(echo $CKSUM  -)
>>>  # I.e., to ensure that the factorizations of integers $1..$2
>>>  # match what we expect.
>>>
>>> @@ -16,15 +14,14 @@ very_expensive_
>>>
>>>  print_ver_ factor seq
>>>
>>> -# Remove the ".sh" suffix:
>>> -t=${ME_%.sh}
>>> +# Template - __{START,END,CKSUM}__ is replaced by local.mk.
>>> +START=__START__
>>> +  END=__END__
>>> +CKSUM=__CKSUM__
>>>
>>> -# Make IFS include "-", so that a simple "set" will separate the args:
>>> -IFS=-$IFS
>>> -set $t
>>> -echo "$3  -" > exp
>>> +echo "$CKSUM  -" > exp
>>>
>>>  f=1
>>> -seq $1 $2 | factor | shasum -c --status exp && f=0
>>> +seq $START $END | factor | shasum -c --status exp && f=0
>>>
>>>  Exit $f
>>> diff --git a/tests/factor/setup.sh b/tests/factor/setup.sh
>>> new file mode 100755
>>> index 0000000..2acb5d3
>>> --- /dev/null
>>> +++ b/tests/factor/setup.sh
>>>
>> A better name would be something like factor/create-test.sh IMHO.
> 
> Renamed.
> 
>>> @@ -0,0 +1,89 @@
>>> +#!/bin/sh
>>>
>> Missing copyright notice (not that I'd care, but the FSF seems to
>> care deeply).
> 
> Good point. Copied from run.sh.
> 
>>> +# Create the factor test scripts
>>> +
>> Missing trailing full-stop.
> 
> Added.
> 
>>> +# Extract the test name from $1.
>>> +testname="$1"
>>>
>> Useless quoting, the following is clearer and perfectly portable:
>>
>>     testname=$1
> 
> Personal paranoia - if something goes wrong, quoting saves
> the day. ;-)
>
Alas, not always!

    $ ksh -c 'x="`echo "a b"`"; echo "$x"'
    ksh: : cannot execute [Is a directory]
    ksh: b: not found [No such file or directory]
    $ ksh -c 'x=`echo "a b"`; echo "$x"'
    a b

I think the Autoconf manual has other lovely examples ...

>>> +t=$( basename "$testname" )
>>>
>> You can save a fork by avoiding a call to basename:
>>
>>     t=${testname##*/}
> 
> Good point. I wasn't sure if that syntax is portable.
> I know it from old dtksh days.
>
It is portable to any decent POSIX shell.  And I think Coreutils assumes
that one is available on the build system.  But then, to ensure such a
shell is actually used, you'll have to invoke your script explicitly with
the configure-determined $(SHELL) from the make recipe below (thank you
Solaris for keeping an old non-POSIX shell as /bin/sh, sigh).

Jim, can you confirm it's OK to assume that $(SHELL) points to a POSIX
shell in the coreutils build system?

>>> +t=$( echo "${t%.sh}" )
>>> +
>> Useless use of echo, just use:
>>
>>    t=${t%.sh}
> 
> Fixed.
> 
>>> +ftdir=$( dirname "$testname" )
>>> +
>> You can similarly save a fork as follows:
>>
>>     case $testname in
>>       */*) ftdir=${testname%/*}
>>         *) ftdir=.;;
>>     esac
>>
>> Albeit this slightly complicates the code, so it might not be
>> worthwhile in the end...
> 
> I fixed that by your suggestion below (redirect stdin/stdout from
> the Makefile).
> 
>>> +# prefix of 2^64
>>> +p=184467440737
>>> +
>>> +# prefix of 2^96
>>> +q=79228162514264337593543
>>> +
>>> +# Each of these numbers has a Pollard rho factor larger than 2^64,
>>> +# and thus exercises some hard-to-reach code in factor.c.
>>> +t1=170141183460469225450570946617781744489
>>> +t2=170141183460469229545748130981302223887
>>> +
>>> +# Factors of the above:
>>> +# t1: 9223372036854775421 18446744073709551709
>>> +# t2: 9223372036854775643 18446744073709551709
>>> +
>>> +# Each test is a triple: lo, hi, sha1 of result.
>>> +# The test script, run.sh, runs seq lo hi|factor|sha1sum
>>> +# and verifies that the actual and expected checksums are the same.
>>> +# New tests must be added to tests/local.mk (factor_tests), too.
>>> +case ".$t" in
>>>
>> Useless quoting, simply using
>>
>>     case .$t in
>>
>> would be fine as well.
>>
>> Also, may I ask why you're using that leading dot?
> 
> Again paranoia. Changed.
> 
>>> +  .t00) d=0-10000000-a451244522b1b662c86cb3cbb55aee3e085a61a0 ;;
>>> +  .t01) d=10000000-20000000-c792a2e02f1c8536b5121f624b04039d20187016 ;;
>>> +  .t02) d=20000000-30000000-8115e8dff97d1674134ec054598d939a2a5f6113 ;;
>>> +  .t03) d=30000000-40000000-fe7b832c8e0ed55035152c0f9ebd59de73224a60 ;;
>>> +  .t04) d=40000000-50000000-b8786d66c432e48bc5b342ee3c6752b7f096f206 ;;
>>> +  .t05) d=50000000-60000000-a74fe518c5f79873c2b9016745b88b42c8fd3ede ;;
>>> +  .t06) d=60000000-70000000-689bc70d681791e5d1b8ac1316a05d0c4473d6db ;;
>>> +  .t07) d=70000000-80000000-d370808f2ab8c865f64c2ff909c5722db5b7d58d ;;
>>> +  .t08) d=80000000-90000000-7978aa66bf2bdb446398336ea6f02605e9a77581 ;;
>>> +  .t09) d=${t1}-${t1}-4622287c5f040cdb7b3bbe4d19d29a71ab277827 ;;
>>> +  .t10) d=${t2}-${t2}-dea308253708b57afad357e8c0d2a111460ef50e ;;
>>> +  .t11) 
>>> d=${p}08551616-${p}08651615-66c57cd58f4fb572df7f088d17e4f4c1d4f01bb1 ;;
>>> +  .t12) 
>>> d=${p}08651616-${p}08751615-729228e693b1a568ecc85b199927424c7d16d410 ;;
>>> +  .t13) 
>>> d=${p}08751616-${p}08851615-5a0c985017c2d285e4698f836f5a059e0b684563 ;;
>>> +  .t14) 
>>> d=${p}08851616-${p}08951615-0482295c514e371c98ce9fd335deed0c9c44a4f4 ;;
>>> +  .t15) 
>>> d=${p}08951616-${p}09051615-9c0e1105ac7c45e27e7bbeb5e213f530d2ad1a71 ;;
>>> +  .t16) 
>>> d=${p}09051616-${p}09151615-604366d2b1d75371d0679e6a68962d66336cd383 ;;
>>> +  .t17) 
>>> d=${p}09151616-${p}09251615-9192d2bdee930135b28d7160e6d395a7027871da ;;
>>> +  .t18) 
>>> d=${p}09251616-${p}09351615-bcf56ae55d20d700690cff4d3327b78f83fc01bf ;;
>>> +  .t19) 
>>> d=${p}09351616-${p}09451615-16b106398749e5f24d278ba7c58229ae43f650ac ;;
>>> +  .t20) 
>>> d=${p}09451616-${p}09551615-ad2c6ed63525f8e7c83c4c416e7715fa1bebc54c ;;
>>> +  .t21) 
>>> d=${p}09551616-${p}09651615-2b6f9c11742d9de045515a6627c27a042c49f8ba ;;
>>> +  .t22) 
>>> d=${p}09651616-${p}09751615-54851acd51c4819beb666e26bc0100dc9adbc310 ;;
>>> +  .t23) 
>>> d=${p}09751616-${p}09851615-6939c2a7afd2d81f45f818a159b7c5226f83a50b ;;
>>> +  .t24) 
>>> d=${p}09851616-${p}09951615-0f2c8bc011d2a45e2afa01459391e68873363c6c ;;
>>> +  .t25) 
>>> d=${p}09951616-${p}10051615-630dc2ad72f4c222bad1405e6c5bea590f92a98c ;;
>>> +  .t26) d=${q}940336-${q}942335-63cbd6313d78247b04d63bbbac50cb8f8d33ff71 ;;
>>> +  .t27) d=${q}942336-${q}944335-0d03d63653767173182491b86fa18f8f680bb036 ;;
>>> +  .t28) d=${q}944336-${q}946335-ca43bd38cd9f97cc5bb63613cb19643578640f0b ;;
>>> +  .t29) d=${q}946336-${q}948335-86d59545a0c13567fa96811821ea5cde950611b1 ;;
>>> +  .t30) d=${q}948336-${q}950335-c3740e702fa9c97e6cf00150860e0b936a141a6b ;;
>>> +  .t31) d=${q}950336-${q}952335-551c3c4c4640d86fda311b5c3006dac45505c0ce ;;
>>> +  .t32) d=${q}952336-${q}954335-b1b0b00463c2f853d70ef9c4f7a96de5cb614156 ;;
>>> +  .t33) d=${q}954336-${q}956335-8938a484a9ef6bb16478091d294fcde9f8ecea69 ;;
>>> +  .t34) d=${q}956336-${q}958335-d1ae6bc712d994f35edf55c785d71ddf31f16535 ;;
>>> +  .t35) d=${q}958336-${q}960335-2374919a89196e1fce93adfe779cb4664556d4b6 ;;
>>> +  .t36) d=${q}960336-${q}962335-569e4363e8d9e8830a187d9ab27365eef08abde1 ;;
>>> +  *)
>>> +    echo "$0: error: unknown test: $t" >&2
>>> +    exit 1
>>> +    ;;
>>> +esac
>>> +
>>> +# Make IFS include "-", so that a simple "set" will separate the args:
>>> +IFS=-$IFS
>>> +set $d
>>> +
>> Wouldn't it be better & safer to save and restore the original value of $IFS?
>> It's not really required for now, granted, but that might change in case the
>> script is modified in the future ...
> 
> I copied that part from run.sh and didn't think much about it. However, ...
> 
>> Or: why not use 'set' directly in the 'case' construct above?  That is,
>> instead of
>>
>>     .t00) d=0-10000000-a451244522b1b662c86cb3cbb55aee3e085a61a0 ;;
>>
>> you could do:
>>
>>     .t00) set 0 10000000 a451244522b1b662c86cb3cbb55aee3e085a61a0 ;;
> 
> ... that's a great idea: I also changed the indentation.
> Now, that part looks nicer - see below.
> 
>>> +# Create the test script from the template for this test
>>> +# by substituting the START, the END and the CKSUM.
>>> +sed \
>>> +  -e "s/__START__/$1/" \
>>> +  -e "s/__END__/$2/" \
>>> +  -e "s/__CKSUM__/$3/" \
>>> +  < "${ftdir}/run.sh" > "$testname" \
>>>
>> I'm not sure this would work in a VPATH setup sadly ...  You'd need
>> to reference $(srcdir) somewhere, but that is available only in the
>> Makefile.  Maybe you could simply make this script read from stdin
>> and write to stdout, and make the redirections in the Makefile recipe
>> itself?
> 
> Done.
> 
>>> +  && chmod a+x "$testname" \
>>> +  || exit 1
>>> +
>> Maybe you could also make the generated test read-only, so that it will
>> be more difficult for some "casual contributor" to try to modify it by
>> mistake:
>>
>>    && chmod a+x,a-w "$testname" \
>>    || exit 1
> 
> I'm not sure about that. If the user changes run.sh or create-test.sh,
> and the tests have to be rebuilt, then that would fail with "permission
> denied".
>
That is easily fixed: you should remove '$@' from the makefile recipe
(simply a "rm -f $@") before rebuilding it.  This read-only issue is
not a big deal though, so I'll stop pushing about it.

> Instead, I added a line into the template that this file is generated.
> 
> 
>>> +test -x "$testname"
>>>
>> A little too paranoid maybe?
> 
> Admitted ;-)
> 
>>> +exit $?
>>
>>> \ No newline at end of file
>>>
>> Huh?
> 
> Sorry, I was using geany, and `make syntax-check` didn't complain
> (although I'm not sure whether it inspected the new file).
> 
> Here's the new approach:
>
I've just tow more nits that is worth reporting ...

> [MEGA-SNIP]

> -$(factor_tests): tests/factor/run.sh
> +$(factor_tests): $(tf)/run.sh $(tf)/create-test.sh
>       $(AM_V_GEN)$(MKDIR_P) $(tf)
> -     $(AM_V_at)ln -f $(srcdir)/tests/factor/run.sh $@
> +     $(AM_V_at)$(srcdir)/$(tf)/create-test.sh $@ \
>
... you should invoke the new script with $(SHELL):

    $(AM_V_at)$(SHELL) $(srcdir)/$(tf)/create-test.sh $@ \

and ...

> +       < $(srcdir)/$(tf)/run.sh \
> +       > $@
> +     @chmod a+x $@
>
... this should be "$(AM_V_at)chmod a+x $@" rather than "@chmod a+x $@"

Regards,
  Stefano