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
[PATCH] test: report summary even when aborting
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). 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