Re: [PATCH] test: report summary even when aborting

2019-05-26 Thread Daniel Kahn Gillmor
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

2019-05-25 Thread Tomi Ollila
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

2019-05-25 Thread Daniel Kahn Gillmor
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