[PATCH] test: report summary even when aborting

[PATCH] test: report summary even when aborting

[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 <dkg@fifthhorseman.net>
---
 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

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

[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 <dkg@fifthhorseman.net>
> ---
>  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

[PATCH v2] test: report summary even when aborting

[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).  If we do abort due to this high-level failure,
though, we should also announce to the user that we're doing so as
close to the end of the process as possible, to make it easier to find
the problem.

Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>
---
 test/notmuch-test | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/test/notmuch-test b/test/notmuch-test
index 50ed8721..b58fd3b3 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -45,6 +45,8 @@ else
 fi
 
 trap 'e=$?; kill $!; exit $e' HUP INT TERM
+
+META_FAILURE=
 # Run the tests
 if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel >/dev/null ; then
     test -t 1 && export COLORS_WITHOUT_TTY=t || :
@@ -57,8 +59,7 @@ if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel >/dev/null ; then
     fi
     RES=$?
     if [[ $RES != 0 ]]; then
-        echo "parallel test suite returned error code $RES"
-        exit $RES
+        META_FAILURE="parallel test suite returned error code $RES"
     fi
 else
     for test in $TESTS; do
@@ -69,7 +70,8 @@ else
         RES=$?
         testname=$(basename $test .sh)
         if [[ $RES != 0 && ! -e "$NOTMUCH_BUILDDIR/test/test-results/$testname" ]]; then
-            exit $RES
+            META_FAILURE="Aborting on $testname (returned $RES)"
+            break
         fi
     done
 fi
@@ -79,6 +81,12 @@ trap - HUP INT TERM
 echo
 $NOTMUCH_SRCDIR/test/aggregate-results.sh $NOTMUCH_BUILDDIR/test/test-results/*
 ev=$?
+if [ -n "$META_FAILURE" ]; then
+    printf 'ERROR: %s\n' "$META_FAILURE"
+    if [ $ev = 0 ]; then
+        ev=$RES
+    fi
+fi
 
 # Clean up
 rm -rf $NOTMUCH_BUILDDIR/test/test-results
-- 
2.20.1

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

[Daniel Kahn Gillmor]

[-- Attachment #1: Type: text/plain, Size: 2118 bytes --]

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 227 bytes --]

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

[Tomi Ollila]

On Sun, May 26 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).  If we do abort due to this high-level failure,
> though, we should also announce to the user that we're doing so as
> close to the end of the process as possible, to make it easier to find
> the problem.
>
> Signed-off-by: Daniel Kahn Gillmor <dkg@fifthhorseman.net>

LGTM.

Tomi


> ---
>  test/notmuch-test | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)
>
> diff --git a/test/notmuch-test b/test/notmuch-test
> index 50ed8721..b58fd3b3 100755
> --- a/test/notmuch-test
> +++ b/test/notmuch-test
> @@ -45,6 +45,8 @@ else
>  fi
>  
>  trap 'e=$?; kill $!; exit $e' HUP INT TERM
> +
> +META_FAILURE=
>  # Run the tests
>  if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel >/dev/null ; then
>      test -t 1 && export COLORS_WITHOUT_TTY=t || :
> @@ -57,8 +59,7 @@ if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel >/dev/null ; then
>      fi
>      RES=$?
>      if [[ $RES != 0 ]]; then
> -        echo "parallel test suite returned error code $RES"
> -        exit $RES
> +        META_FAILURE="parallel test suite returned error code $RES"
>      fi
>  else
>      for test in $TESTS; do
> @@ -69,7 +70,8 @@ else
>          RES=$?
>          testname=$(basename $test .sh)
>          if [[ $RES != 0 && ! -e "$NOTMUCH_BUILDDIR/test/test-results/$testname" ]]; then
> -            exit $RES
> +            META_FAILURE="Aborting on $testname (returned $RES)"
> +            break
>          fi
>      done
>  fi
> @@ -79,6 +81,12 @@ trap - HUP INT TERM
>  echo
>  $NOTMUCH_SRCDIR/test/aggregate-results.sh $NOTMUCH_BUILDDIR/test/test-results/*
>  ev=$?
> +if [ -n "$META_FAILURE" ]; then
> +    printf 'ERROR: %s\n' "$META_FAILURE"
> +    if [ $ev = 0 ]; then
> +        ev=$RES
> +    fi
> +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

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

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> 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.
>

pushed to master,

d

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

[David Bremner]

Daniel Kahn Gillmor <dkg@fifthhorseman.net> writes:

> 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.

pushed to master