Re: [PATCH] t5310-pack-bitmaps: fix bogus 'pack-objects to file can use bitmap' test

From: SZEDER Gábor <szeder.dev@gmail.com>
Date: Aug 17, 2018, 20:15 UTC
Message-ID: <CAM0VKj=rw14MbLbXvuJiiwf05Txo1tu6A8xzy+_3RHOytX8mXA@mail.gmail.com>
In-Reply-To: <eeb04c94-50c1-13ee-880b-ea380031a685@gmail.com>

On Fri, Aug 17, 2018 at 9:27 PM Andrei Rybak <rybak.a.v@gmail.com> wrote:

Show 40 quoted lines

>
> On 17/08/18 19:39, SZEDER Gábor wrote:
> >
> > See, we have quite a few tests that extract repetitive common tasks
> > into helper functions, which sometimes includes preparing the expected
> > results and running 'test_cmp', e.g. something like this
> > (oversimplified) example:
> >
> >   check_cmd () {
> >         git cmd $1 >actual &&
> >         echo "$2" >expect &&
> >         test_cmp expect actual
> >   }
> >
> >   check_cmd --foo    FOO
> >   check_cmd --no-foo ""
>
> I've only had time to look into this from t0001 up to t0008-ignores.sh, where
> test_check_ignore does this. If these helper functions need to allow comparing
> empty files -- how about adding special variation of cmp functions for cases
> like this: test_cmp_allow_empty and test_i18ncmp_allow_empty?
>
> I think it would be a good trade-off to allow these helper functions to skip
> checking emptiness of arguments for test_cmp. Such patch will require only
> s/test_cmp/&_allow_empty/ for these helper functions and it will help catch
> cases as bogus test in t5310.
>
> I'll try something like the following on the weekend:
>
>         test_cmp() {
>                 if test "$1" != - && ! test -s "$1"
>                 then
>                         echo >&4 "error: trying to compare empty file '$1'"
>                         return 1
>                 fi
>                 if test "$2" != - && ! test -s "$2"
>                 then
>                         echo >&4 "error: trying to compare empty file '$2'"
>                         return 1
>                 fi

Yeah, these conditions are what I instrumented 'test_cmp' with (except I used 'error "bug in test script ..."') to find callsites that could be converted to 'test_must_be_empty', that's how I found the bug fixed in this patch. However, it resulted in a lot of errors from the cases mentioned in my previous email. Then I reached out to Bash and tried this:

  test_cmp() {
         if test -n "$BASH_VERSION" &&
            test "${FUNCNAME[1]}" = "test_eval_inner_" &&
            test "$1" != "-" &&
            test ! -s "$1"
         then
                 error "bug in test script: using test_cmp to check
empty file; use test_must_be_empty instead"
         fi
         $GIT_TEST_CMP "$@"
  }

i.e. to limit the check callsites where 'test_cmp' is called directly from within a test_expect_{success,failure} block. This is better, almost all errors could be converted to 'test_must_be_empty' stright away; I have the patches almost ready. There are, however, a few parametric cases that choke on this: where we run 'test_cmp' in a loop, e.g. 'cvs update (-p)' in t9400, and I think there was a case where the 'test_expect_success' block is within a function.

Show 9 quoted lines

>                 test_cmp_allow_empty "$@"
>         }
>
>         test_cmp_allow_empty() {
>                 $GIT_TEST_CMP "$@"
>         }
>
> (I'm not sure about redirections in test lib functions. The two if's would
> probably be in a separate function to be re-used by test_i18ncmp.)