Re: [PATCH 2/9] ci/run-build-and-tests: take a more high-level view

[Eric Sunshine <sunshine@sunshineco.com>]

On Mon, Jan 24, 2022 at 3:02 PM Johannes Schindelin via GitGitGadget
<gitgitgadget@gmail.com> wrote:
> In the web UI of GitHub workflows, failed runs are presented with the
> job step that failed auto-expanded. In the current setup, this is not
> helpful at all because that shows only the output of `prove`, which says
> which test failed, but not in what way.
>
> What would help understand the reader what went wrong is the verbose
> test output of the failed test.
>
> The logs of the failed runs do contain that verbose test output, but it
> is shown in the _next_ step (which is marked as succeeding, and is
> therefore _not_ auto-expanded). Anyone not intimately familiar with this
> would completely miss the verbose test output, being left mostly
> puzzled with the test failures.
>
> We are about to show the failed test cases' output in the _same_ step,
> so that the user has a much easier time to figure out what was going
> wrong.
>
> But first, we must partially revert the change that tried to improve the
> CI runs by combining the `Makefile` targets to build into a single
> `make` invocation. That might have sounded like a good idea at the time,
> but it does make it rather impossible for the CI script to determine
> whether the _build_ failed, or the _tests_. If the tests were run at
> all, that is.
>
> So let's go back to calling `make` for the build, and call `make test`
> separately so that we can easily detect that _that_ invocation failed,
> and react appropriately.
>
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
> ---
> diff --git a/ci/run-build-and-tests.sh b/ci/run-build-and-tests.sh
> @@ -10,7 +10,7 @@ windows*) cmd //c mklink //j t\\.prove "$(cygpath -aw "$cache_dir/.prove")";;
> -export MAKE_TARGETS="all test"
> +run_tests=t
>
>  case "$jobname" in
>  linux-gcc)
> @@ -41,14 +41,18 @@ pedantic)
>         # Don't run the tests; we only care about whether Git can be
>         # built.
>         export DEVOPTS=pedantic
> -       export MAKE_TARGETS=all
> +       run_tests=
>         ;;
>  esac
>
>  # Any new "test" targets should not go after this "make", but should
>  # adjust $MAKE_TARGETS. Otherwise compilation-only targets above will
>  # start running tests.
> -make $MAKE_TARGETS

The comment talking about MAKE_TARGETS seems out of date now that
MAKE_TARGETS has been removed from this script.

> +make
> +if test -n "$run_tests"
> +then
> +       make test
> +fi
>  check_unignored_build_artifacts

This changes behavior, doesn't it? Wth the original "make all test",
if the `all` target failed, then the `test` target would not be
invoked. However, with the revised code, `make test` is invoked even
if `make all` fails. Is that behavior change significant? Do we care
about it?