Re: [PATCH 2/9] ci/run-build-and-tests: take a more high-level view
- From
Eric Sunshine <sunshine@sunshineco.com>
- Date
- Jan 24, 2022, 23:22 UTC
- Message-ID
- <CAPig+cTk142o5xkN0jHB6y1h=4zYBwhpwdTxG0oO3Ox+xqdgdQ@mail.gmail.com>
- In-Reply-To
- <d2ff51bb5dacfe084166de106e9a864c902f0f36.1643050574.git.gitgitgadget@gmail.com>
On Mon, Jan 24, 2022 at 3:02 PM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote:
Show 51 quoted lines
> 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.
Show 6 quoted lines
> +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?