Re: [PATCH] test: report summary even when aborting
Thanks for the feedback, Tomi! On Sat 2019-05-25 22:41:58 +0300, Tomi Ollila wrote: > On Sat, May 25 2019, Daniel Kahn Gillmor wrote: > >> In certain cases of test suite failure, the summary report was not >> being printed. In particular, any failure on the parallel test suite, >> and any aborted test in the serialized test suite would end up hiding >> the summary. >> >> It's better to always show the summary where we can (while preserving >> the return code). > > This is an improvement, but it looks like it is possible > aggregate-results.sh prints everything good (like all 3 tests passed) > since it may be that there are only good logs in test-results/. > > That might, in some cases, be confusing (exit value of notmuch-test > is not always visible). Yes, i share your concern about confusion, and i tried to stave off that confusion by making sure that there was something printed in each of the fallthrough cases. But you're right that i didn't do that well enough. v2 of my patch also moves the printed warning to after aggregate-results.sh output, and prefixes it with "ERROR:", so that it's more obvious that this is why we are terminating non-zero. > One option could be giving $RES to aggregate-results.sh and if it is > not zero then that could print different text -- just that it requires > a bit more coding... I'm not convinced that's a worthwhile tradeoff. i don't want to add an option parser to aggregate-results.sh, and it uses all of "$@" already. So in the simplest implementation we'd be passing some sort of environment variable to it -- but that variable wouldn't do anything other than print what notmuch-test is already printing. certainly if we just passed $RES we'd have even less information about the context of where/how the meta-failure happened. My v2 patch does a better job of this, hopefully. So if someone has strong feelings about this and wants to update aggregate-results.sh instead, feel free to do so; i wouldn't block it, but hopefully the current proposal is acceptable. thanks for the review, --dkg signature.asc Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH] test: report summary even when aborting
On Sat, May 25 2019, Daniel Kahn Gillmor wrote: > In certain cases of test suite failure, the summary report was not > being printed. In particular, any failure on the parallel test suite, > and any aborted test in the serialized test suite would end up hiding > the summary. > > It's better to always show the summary where we can (while preserving > the return code). This is an improvement, but it looks like it is possible aggregate-results.sh prints everything good (like all 3 tests passed) since it may be that there are only good logs in test-results/. That might, in some cases, be confusing (exit value of notmuch-test is not always visible). One option could be giving $RES to aggregate-results.sh and if it is not zero then that could print different text -- just that it requires a bit more coding... Tomi > > Signed-off-by: Daniel Kahn Gillmor > --- > test/notmuch-test | 11 --- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/test/notmuch-test b/test/notmuch-test > index 50ed8721..d835e152 100755 > --- a/test/notmuch-test > +++ b/test/notmuch-test > @@ -45,6 +45,8 @@ else > fi > > trap 'e=$?; kill $!; exit $e' HUP INT TERM > + > +RES=0 > # Run the tests > if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel >/dev/null ; then > test -t 1 && export COLORS_WITHOUT_TTY=t || : > @@ -58,7 +60,6 @@ if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel > >/dev/null ; then > RES=$? > if [[ $RES != 0 ]]; then > echo "parallel test suite returned error code $RES" > -exit $RES > fi > else > for test in $TESTS; do > @@ -69,7 +70,7 @@ else > RES=$? > testname=$(basename $test .sh) > if [[ $RES != 0 && ! -e > "$NOTMUCH_BUILDDIR/test/test-results/$testname" ]]; then > -exit $RES > +echo "Aborting on $testname (returned $RES)" > fi > done > fi > @@ -78,7 +79,11 @@ trap - HUP INT TERM > # Report results > echo > $NOTMUCH_SRCDIR/test/aggregate-results.sh > $NOTMUCH_BUILDDIR/test/test-results/* > -ev=$? > +if [ "$RES" = 0 ]; then > +ev=$? > +else > +ev=$RES > +fi > > # Clean up > rm -rf $NOTMUCH_BUILDDIR/test/test-results > -- > 2.20.1 > > ___ > notmuch mailing list > notmuch@notmuchmail.org > https://notmuchmail.org/mailman/listinfo/notmuch ___ notmuch mailing list notmuch@notmuchmail.org https://notmuchmail.org/mailman/listinfo/notmuch
