Hi Eric, On Mon, 24 Jan 2022, Eric Sunshine wrote: > On Mon, Jan 24, 2022 at 3:02 PM Johannes Schindelin via GitGitGadget > <gitgitgadget@xxxxxxxxx> 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@xxxxxx> > > --- > > 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. Good catch! > > +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? That is actually not the case. Compare to what 25715419bf4 (CI: don't run "make test" twice in one job, 2021-11-23) did: it removed code that _also_ did not specifically prevent `make test` from running when `make all` failed. The clue to the riddle is this line in `ci/lib.sh`: set -ex The `-e` part lets the script fail whenever any command fails (unless it is part of an `if`/`while` condition, or properly chained with `||`). This line is actually touched by the "ci/run-build-and-tests: add some structure to the GitHub workflow output" patch in this patch series, which breaks it apart into the `set -e` and the `set -x` part (so that the latter can be called later in GitHub workflows, to unclutter the output a bit). Ciao, Dscho
