Re: [PATCH 7/9] tests: include detailed trace logs with --write-junit-xml upon failure

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Eric,

On Tue, 4 Sep 2018, Eric Sunshine wrote:

> On Mon, Sep 3, 2018 at 5:10 PM Johannes Schindelin via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> > So let's do something different in VSTS: let's run all the tests with
> > `--quiet` first, and only if a failure is encountered, try to trace the
> > commands as they are executed. [...]
> >
> > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> > ---
> > diff --git a/t/test-lib.sh b/t/test-lib.sh
> > @@ -445,10 +452,37 @@ test_ok_ () {
> >  test_failure_ () {
> >         if test -n "$write_junit_xml"
> >         then
> > +               if test -z "$GIT_TEST_TEE_OUTPUT_FILE"
> > +               then
> > +                       case "$(type kill_p4d 2>/dev/null | head -n 1)" in
> > +                       *function*) kill_p4d;;
> > +                       esac
> > +
> > +                       case "$(type stop_git_daemon 2>/dev/null |
> > +                               head -n 1)" in
> > +                       *function*) stop_git_daemon;;
> > +                       esac
> 
> In the long run, it might make more sense, and be more scalable, to
> have those scripts define a "prepare_for_rerun" variable or function
> which this code then runs generically rather than having special
> knowledge of those facilities.
> 
> I could imagine, for instance, test-lib.sh defining a no-op:
> 
>     test_failure_prepare_rerun () {}
> 
> and then each of those scripts overriding the function:
> 
>     # in lib-git-p4.sh
>     test_failure_prepare_rerun () {
>         kill_p4d
>     }
> 
>     # in lib-git-daemon.sh
>     test_failure_prepare_rerun () {
>         stop_git_daemon
>     }

Or we could implement `test_atexit` (similar to `test_when_finished`, but
to be executed at `test_done` time). I guess that's what the p4 and daemon
tests really needed to begin with (and probably also the apache2-using
tests).

> 
> > +                       # re-run with --verbose-log
> > +                       echo "# Re-running: $junit_rerun_options_sq" >&2
> > +
> > +                       cd "$TEST_DIRECTORY" &&
> > +                       eval "${TEST_SHELL_PATH}" "$junit_rerun_options_sq" \
> > +                               >/dev/null 2>&1
> > +                       status=$?
> > +
> > +                       say_color "" "$(test 0 = $status ||
> > +                               echo "not ")ok $test_count - (re-ran with trace)"
> > +                       say "1..$test_count"
> > +                       GIT_EXIT_OK=t
> > +                       exit $status
> > +               fi
> > +
> >                 junit_insert="<failure message=\"not ok $test_count -"
> >                 junit_insert="$junit_insert $(xml_attr_encode "$1")\">"
> >                 junit_insert="$junit_insert $(xml_attr_encode \
> > -                       "$(printf '%s\n' "$@" | sed 1d)")"
> > +                       "$(cat "$GIT_TEST_TEE_OUTPUT_FILE")")"
> > +               >"$GIT_TEST_TEE_OUTPUT_FILE"
> >                 junit_insert="$junit_insert</failure>"
> >                 write_junit_xml_testcase "$1" "      $junit_insert"
> >         fi
> 
> This junit-related stuff is getting pretty lengthy. I wonder if it
> would make sense to pull it out to its own function at some point
> (again, in the long run).

Now that you mention it... I agree. This is getting long.

In the short run, I have two things to consider, though: I want to make
this work first, then think about introducing a layer of abstraction, and
I want to go on vacation tomorrow.

So I agree that this is something to be considered in the long run, i.e.
not right now ;-)

Thanks,
Dscho