Re: factor tests: ugly long file names

[Stefano Lattarini]

On 10/26/2012 06:45 PM, Bernhard Voelker wrote:
> On 10/26/2012 10:34 AM, Jim Meyering wrote:
>> Bernhard Voelker wrote:
>>> I was just wondering if we shouldn't refactor the new factor tests.
>>>
>>> They have ugly and long file names (the test data), and have identical 
>>> content
>>> (which is redundancy which in turn should be avoided IMO):
>>>
>>> $ md5sum *.sh
>>> 31c3a9e8b5dad969dafd07c348c48233
>>> 0-10000000-a451244522b1b662c86cb3cbb55aee3e085a61a0.sh
>>
>> As you've seen, the factorization, ahem, is already done, in a sense.
>> Those test cases are recorded in factored form in tests/local.mk.
>> No line there is longer than 80.
>>
>> While there are many of these long-named factor tests, the only file
>> saved in version control is run.sh, and those long-named files are
>> merely temporaries that are hard-linked to run.sh.
> 
> yes, I've seen that (on the second glance).
> 
>> However, as you've seen, the resulting (expanded) file names
>> are very long.  We could easily shave off a few bytes by
>> recording "START-N" rather than "START-END" pairs.
>>
>> -  
>> $(tf)/$(p)08551616-$(p)08651615-66c57cd58f4fb572df7f088d17e4f4c1d4f01bb1.sh \
>> +  $(tf)/$(p)08551616-99999-66c57cd58f4fb572df7f088d17e4f4c1d4f01bb1.sh \
>>
>> -  $(tf)/$(t1)-$(t1)-4622287c5f040cdb7b3bbe4d19d29a71ab277827.sh \
>> +  $(tf)/$(t1)-0-e5826a8ddfe2c1281e5dadd90a874eda.sh \
>>
>>
>> With this matching change:
>>
>> diff --git a/tests/factor/run.sh b/tests/factor/run.sh
>> index 6ff24c3..d63f733 100755
>> --- a/tests/factor/run.sh
>> +++ b/tests/factor/run.sh
>> @@ -3,8 +3,8 @@
>>  # 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  -)
>> -# I.e., to ensure that the factorizations of integers $1..$2
>> +# seq $1 $(expr $1 + $2) | factor | shasum -c --status <(echo $3  -)
>> +# I.e., to ensure that the factorizations of integers $1..$1+$2
>>  # match what we expect.
>>
>>  # Copyright (C) 2012 Free Software Foundation, Inc.
>> @@ -25,6 +25,7 @@ set $t
>>  echo "$3  -" > exp
>>
>>  f=1
>> -seq $1 $2 | factor | shasum -c --status exp && f=0
>> +end=$(expr $1 + $2)
>> +seq $1 $end | factor | shasum -c --status exp && f=0
>>
>>  Exit $f
>>
>> If "make check" output line length were really important
>> --------------------------------
>>
>> A simple additional change would be to use md5sum in place of sha1sum.
>> We don't need the added strength/length of sha1sum for this.
>>
>> You could save more by encoding the checksum in base64 rather than hex,
>> but with that, the mechanics may get involved, so I wouldn't bother.
>>
>> Another simple change: use only 8 or 10 hex digits instead
>> of the full checksum.
>>
>> You're welcome to pursue it.
> 
> As the tests are created by tests/local.mk, I rather thought of going
> away from hardlinks, and, instead, use run.sh as a template by exchanging
> START, END, and CKSUM variables into factor-{01,02,03,04,...}.sh.
> Something like the following.
> 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 :-)

On the serious side, I've some nits below.

> * 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.

> @@ -0,0 +1,89 @@
> +#!/bin/sh
>
Missing copyright notice (not that I'd care, but the FSF seems to
care deeply).

> +# Create the factor test scripts
> +
Missing trailing full-stop.

> +# Extract the test name from $1.
> +testname="$1"
>
Useless quoting, the following is clearer and perfectly portable:

    testname=$1

> +t=$( basename "$testname" )
>
You can save a fork by avoiding a call to basename:

    t=${testname##*/}

> +t=$( echo "${t%.sh}" )
> +
Useless use of echo, just use:

   t=${t%.sh}

> +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...

> +# 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?

> +  .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 ...

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 ;;

> +# 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?

> +  && 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

> +test -x "$testname"
>
A little too paranoid maybe?

> +exit $?

> \ No newline at end of file
>
Huh?

> diff --git a/tests/local.mk b/tests/local.mk
> index f31c8b0..7ca95c1 100644
> --- a/tests/local.mk
> +++ b/tests/local.mk
> @@ -96,6 +96,7 @@ EXTRA_DIST +=                       \
>    tests/d_type-check         \
>    tests/envvar-check         \
>    tests/factor/run.sh                \
> +  tests/factor/setup.sh              \
>    tests/filefrag-extent-compare \
>    tests/fiemap-capable               \
>    tests/init.sh                      \
> @@ -625,65 +626,21 @@ all_tests =                                     \
>    tests/touch/trailing-slash.sh                      \
>    $(all_root_tests)
> 
> -# 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 tests 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.
> +# See tests/factor/setup.sh.
>  tf = tests/factor
>  factor_tests = \
> -  $(tf)/0-10000000-a451244522b1b662c86cb3cbb55aee3e085a61a0.sh \
> -  $(tf)/10000000-20000000-c792a2e02f1c8536b5121f624b04039d20187016.sh \
> -  $(tf)/20000000-30000000-8115e8dff97d1674134ec054598d939a2a5f6113.sh \
> -  $(tf)/30000000-40000000-fe7b832c8e0ed55035152c0f9ebd59de73224a60.sh \
> -  $(tf)/40000000-50000000-b8786d66c432e48bc5b342ee3c6752b7f096f206.sh \
> -  $(tf)/50000000-60000000-a74fe518c5f79873c2b9016745b88b42c8fd3ede.sh \
> -  $(tf)/60000000-70000000-689bc70d681791e5d1b8ac1316a05d0c4473d6db.sh \
> -  $(tf)/70000000-80000000-d370808f2ab8c865f64c2ff909c5722db5b7d58d.sh \
> -  $(tf)/80000000-90000000-7978aa66bf2bdb446398336ea6f02605e9a77581.sh \
> -  $(tf)/$(t1)-$(t1)-4622287c5f040cdb7b3bbe4d19d29a71ab277827.sh \
> -  $(tf)/$(t2)-$(t2)-dea308253708b57afad357e8c0d2a111460ef50e.sh \
> -  
> $(tf)/$(p)08551616-$(p)08651615-66c57cd58f4fb572df7f088d17e4f4c1d4f01bb1.sh \
> -  
> $(tf)/$(p)08651616-$(p)08751615-729228e693b1a568ecc85b199927424c7d16d410.sh \
> -  
> $(tf)/$(p)08751616-$(p)08851615-5a0c985017c2d285e4698f836f5a059e0b684563.sh \
> -  
> $(tf)/$(p)08851616-$(p)08951615-0482295c514e371c98ce9fd335deed0c9c44a4f4.sh \
> -  
> $(tf)/$(p)08951616-$(p)09051615-9c0e1105ac7c45e27e7bbeb5e213f530d2ad1a71.sh \
> -  
> $(tf)/$(p)09051616-$(p)09151615-604366d2b1d75371d0679e6a68962d66336cd383.sh \
> -  
> $(tf)/$(p)09151616-$(p)09251615-9192d2bdee930135b28d7160e6d395a7027871da.sh \
> -  
> $(tf)/$(p)09251616-$(p)09351615-bcf56ae55d20d700690cff4d3327b78f83fc01bf.sh \
> -  
> $(tf)/$(p)09351616-$(p)09451615-16b106398749e5f24d278ba7c58229ae43f650ac.sh \
> -  
> $(tf)/$(p)09451616-$(p)09551615-ad2c6ed63525f8e7c83c4c416e7715fa1bebc54c.sh \
> -  
> $(tf)/$(p)09551616-$(p)09651615-2b6f9c11742d9de045515a6627c27a042c49f8ba.sh \
> -  
> $(tf)/$(p)09651616-$(p)09751615-54851acd51c4819beb666e26bc0100dc9adbc310.sh \
> -  
> $(tf)/$(p)09751616-$(p)09851615-6939c2a7afd2d81f45f818a159b7c5226f83a50b.sh \
> -  
> $(tf)/$(p)09851616-$(p)09951615-0f2c8bc011d2a45e2afa01459391e68873363c6c.sh \
> -  
> $(tf)/$(p)09951616-$(p)10051615-630dc2ad72f4c222bad1405e6c5bea590f92a98c.sh \
> -  $(tf)/$(q)940336-$(q)942335-63cbd6313d78247b04d63bbbac50cb8f8d33ff71.sh \
> -  $(tf)/$(q)942336-$(q)944335-0d03d63653767173182491b86fa18f8f680bb036.sh \
> -  $(tf)/$(q)944336-$(q)946335-ca43bd38cd9f97cc5bb63613cb19643578640f0b.sh \
> -  $(tf)/$(q)946336-$(q)948335-86d59545a0c13567fa96811821ea5cde950611b1.sh \
> -  $(tf)/$(q)948336-$(q)950335-c3740e702fa9c97e6cf00150860e0b936a141a6b.sh \
> -  $(tf)/$(q)950336-$(q)952335-551c3c4c4640d86fda311b5c3006dac45505c0ce.sh \
> -  $(tf)/$(q)952336-$(q)954335-b1b0b00463c2f853d70ef9c4f7a96de5cb614156.sh \
> -  $(tf)/$(q)954336-$(q)956335-8938a484a9ef6bb16478091d294fcde9f8ecea69.sh \
> -  $(tf)/$(q)956336-$(q)958335-d1ae6bc712d994f35edf55c785d71ddf31f16535.sh \
> -  $(tf)/$(q)958336-$(q)960335-2374919a89196e1fce93adfe779cb4664556d4b6.sh \
> -  $(tf)/$(q)960336-$(q)962335-569e4363e8d9e8830a187d9ab27365eef08abde1.sh
> +  $(tf)/t00.sh $(tf)/t01.sh $(tf)/t02.sh $(tf)/t03.sh $(tf)/t04.sh \
> +  $(tf)/t05.sh $(tf)/t06.sh $(tf)/t07.sh $(tf)/t08.sh $(tf)/t09.sh \
> +  $(tf)/t10.sh $(tf)/t11.sh $(tf)/t12.sh $(tf)/t13.sh $(tf)/t14.sh \
> +  $(tf)/t15.sh $(tf)/t16.sh $(tf)/t17.sh $(tf)/t18.sh $(tf)/t19.sh \
> +  $(tf)/t20.sh $(tf)/t21.sh $(tf)/t22.sh $(tf)/t23.sh $(tf)/t24.sh \
> +  $(tf)/t25.sh $(tf)/t26.sh $(tf)/t27.sh $(tf)/t28.sh $(tf)/t29.sh \
> +  $(tf)/t30.sh $(tf)/t31.sh $(tf)/t32.sh $(tf)/t33.sh $(tf)/t34.sh \
> +  $(tf)/t35.sh $(tf)/t36.sh
> 
> -$(factor_tests): tests/factor/run.sh
> +$(factor_tests): tests/factor/run.sh tests/factor/setup.sh
>       $(AM_V_GEN)$(MKDIR_P) $(tf)
> -     $(AM_V_at)ln -f $(srcdir)/tests/factor/run.sh $@
> +     $(AM_V_at)$(srcdir)/tests/factor/setup.sh $@
> 
>  CLEANFILES += $(factor_tests)
> 

Regards,
  Stefano