[PATCH] test: aggregate-results updates

[PATCH] test: aggregate-results updates

[Tomi Ollila]

notmuch-test will now call aggregate-results.sh with file list
that it compiles based on the test ran, and aggregate-results
will report failure is any of the test files are missing.

With this notmuch-test no longer has to exit in non-parallel
run if some test fail to write its report file -- so it works
as parallel tests in this sense.

Changed test_done() in test-lib.sh write report file in one write(2),
so there is (even) less chance it being partially written. Also,
now it writes 'total' last and aggregate-results.sh expects this
line to exist in all report files for reporting to be successful.

Added 'set -eu' to notmuch-test and modified code to work with
these settings. That makes it harder to get mistakes slipped
into committed code.
---

testing all changed branches is somewhat hard; I did by adding
sleep 20 and file what effective removed created results files;
another change was to change 'total' to 'toxtal' in test_done,
then I run (at least) the following command lines:

make test
NOTMUCH_TESTS='T140-excludes.sh T740-body.sh' make test
NOTMUCH_TESTS='T140-excludes.sh T740-body.sh' NOTMUCH_TEST_SERIALIZE=t make test
NOTMUCH_TESTS=T160-json.sh NOTMUCH_TEST_SERIALIZE=t test/notmuch-test

 test/aggregate-results.sh | 22 ++++++++++++++++--
 test/notmuch-test         | 47 ++++++++++++++++++++++-----------------
 test/test-lib.sh          | 15 +++++++------
 3 files changed, 54 insertions(+), 30 deletions(-)

diff --git a/test/aggregate-results.sh b/test/aggregate-results.sh
index 75400e6e..6845fcf0 100755
--- a/test/aggregate-results.sh
+++ b/test/aggregate-results.sh
@@ -8,9 +8,16 @@ failed=0
 broken=0
 total=0
 all_skipped=0
+rep_failed=0
 
 for file
 do
+	if [ ! -f "$file" ]; then
+		echo "'$file' does not exist!"
+		rep_failed=$((rep_failed + 1))
+		continue
+	fi
+	has_total=0
 	while read type value
 	do
 		case $type in
@@ -24,18 +31,23 @@ do
 			broken=$((broken + value)) ;;
 		total)
 			total=$((total + value))
+			has_total=1
 			if [ "$value" -eq 0 ]; then
 				all_skipped=$((all_skipped + 1))
 			fi
 		esac
 	done <"$file"
+	if [ "$has_total" -eq 0 ]; then
+		echo "'$file' lacks 'total ...'; results may be inconsistent."
+		failed=$((failed + 1))
+	fi
 done
 
 pluralize_s () { [ "$1" -eq 1 ] && s='' || s='s'; }
 
 echo "Notmuch test suite complete."
 
-if [ "$fixed" -eq 0 ] && [ "$failed" -eq 0 ]; then
+if [ "$fixed" -eq 0 ] && [ "$failed" -eq 0 ] && [ "$rep_failed" -eq 0 ]; then
 	pluralize_s "$total"
 	printf "All $total test$s "
 	if [ "$broken" -eq 0 ]; then
@@ -70,10 +82,16 @@ if [ "$all_skipped" -ne 0 ]; then
 	echo "All tests in $all_skipped file$s skipped."
 fi
 
+if [ "$rep_failed" -ne 0 ]; then
+	pluralize_s "$rep_failed"
+	echo "$rep_failed test$s failed to report results."
+fi
+
 # Note that we currently do not consider skipped tests as failing the
 # build.
 
-if [ "$success" -gt 0 ] && [ "$fixed" -eq 0 ] && [ "$failed" -eq 0 ]
+if [ "$success" -gt 0 ] && [ "$fixed" -eq 0 ] &&
+	[ "$failed" -eq 0 ] && [ "$rep_failed" -eq 0 ]
 then
 	exit 0
 else
diff --git a/test/notmuch-test b/test/notmuch-test
index cbd33f93..ce142f7c 100755
--- a/test/notmuch-test
+++ b/test/notmuch-test
@@ -18,12 +18,14 @@ fi
 # Ensure NOTMUCH_SRCDIR and NOTMUCH_BUILDDIR are set.
 . $(dirname "$0")/export-dirs.sh || exit 1
 
+set -eu
+
 TESTS=
-for test in $NOTMUCH_TESTS; do
+for test in ${NOTMUCH_TESTS-}; do
     TESTS="$TESTS $NOTMUCH_SRCDIR/test/$test"
 done
 
-if [[ -z "$TESTS" ]]; then
+if [ -z "$TESTS" ]; then
     TESTS="$NOTMUCH_SRCDIR/test/T[0-9][0-9][0-9]-*.sh"
 fi
 
@@ -44,43 +46,46 @@ else
     TEST_TIMEOUT_CMD=""
 fi
 
-trap 'e=$?; kill $!; exit $e' HUP INT TERM
-
 META_FAILURE=
+RES=0
 # Run the tests
-if test -z "$NOTMUCH_TEST_SERIALIZE" && command -v parallel >/dev/null ; then
+if test -z "${NOTMUCH_TEST_SERIALIZE-}" && command -v parallel >/dev/null ; then
     test -t 1 && export COLORS_WITHOUT_TTY=t || :
     if parallel --version 2>&1 | grep -q GNU ; then
         echo "INFO: running tests with GNU parallel"
-        printf '%s\n' $TESTS | $TEST_TIMEOUT_CMD parallel
+        printf '%s\n' $TESTS | $TEST_TIMEOUT_CMD parallel || RES=$?
     else
         echo "INFO: running tests with moreutils parallel"
-        $TEST_TIMEOUT_CMD parallel -- $TESTS
+        $TEST_TIMEOUT_CMD parallel -- $TESTS || RES=$?
     fi
-    RES=$?
-    if [[ $RES != 0 ]]; then
+    if [ $RES != 0 ]; then
         META_FAILURE="parallel test suite returned error code $RES"
     fi
 else
+    trap 'e=$?; trap - 0; kill ${!-}; exit $e' 0 HUP INT TERM
     for test in $TESTS; do
         $TEST_TIMEOUT_CMD $test "$@" &
-        wait $!
-        # If the test failed without producing results, then it aborted,
-        # so we should abort, too.
-        RES=$?
-        testname=$(basename $test .sh)
-        if [[ $RES != 0 && ! -e "$NOTMUCH_BUILDDIR/test/test-results/$testname" ]]; then
-            META_FAILURE="Aborting on $testname (returned $RES)"
-            break
-        fi
+        wait $! && ev=0 || ev=$?
+        test $ev = 0 || RES=$ev
     done
+    trap - 0 HUP INT TERM
+    if [ $RES != 0 ]; then
+        META_FAILURE="some tests failed; first failed returned error code $RES"
+    fi
 fi
-trap - HUP INT TERM
 
 # Report results
+RESULT_FILES=
+for file in $TESTS
+do
+    file=${file##*/} # drop leading path components
+    file=${file%.sh} # drop trailing '.sh'
+    RESULT_FILES="$RESULT_FILES $NOTMUCH_BUILDDIR/test/test-results/$file"
+done
+
 echo
-$NOTMUCH_SRCDIR/test/aggregate-results.sh $NOTMUCH_BUILDDIR/test/test-results/*
-ev=$?
+$NOTMUCH_SRCDIR/test/aggregate-results.sh $RESULT_FILES && ev=0 || ev=$?
+
 if [ -n "$META_FAILURE" ]; then
     printf 'ERROR: %s\n' "$META_FAILURE"
     if [ $ev = 0 ]; then
diff --git a/test/test-lib.sh b/test/test-lib.sh
index d7c96a93..2dceb497 100644
--- a/test/test-lib.sh
+++ b/test/test-lib.sh
@@ -1021,15 +1021,16 @@ test_when_finished () {
 test_done () {
 	GIT_EXIT_OK=t
 	test_results_dir="$TEST_DIRECTORY/test-results"
-	mkdir -p "$test_results_dir"
+	test -d "$test_results_dir" || mkdir "$test_results_dir"
 	test_results_path="$test_results_dir/$this_test"
 
-	echo "total $test_count" >> $test_results_path
-	echo "success $test_success" >> $test_results_path
-	echo "fixed $test_fixed" >> $test_results_path
-	echo "broken $test_broken" >> $test_results_path
-	echo "failed $test_failure" >> $test_results_path
-	echo "" >> $test_results_path
+	printf %s\\n \
+		"success $test_success" \
+		"fixed $test_fixed" \
+		"broken $test_broken" \
+		"failed $test_failure" \
+		"total $test_count" \
+	    > $test_results_path
 
 	[ -n "$EMACS_SERVER" ] && test_emacs '(kill-emacs)'
 
-- 
2.25.1

Re: [PATCH] test: aggregate-results updates

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:
>
> testing all changed branches is somewhat hard; I did by adding
> sleep 20 and file what effective removed created results files;
> another change was to change 'total' to 'toxtal' in test_done,
> then I run (at least) the following command lines:

changing test_done results in pretty nonsensical output. I guess that's
OK?

$  NOTMUCH_TESTS=T310-emacs.sh ./notmuch-test

...

'/home/bremner/software/upstream/notmuch/test/test-results/T310-emacs' lacks 'total ...'; results may be inconsistent.
Notmuch test suite complete.
72/0 tests passed.
1 test failed.
-73 tests skipped.

Re: [PATCH] test: aggregate-results updates

[Tomi Ollila]

On Sat, Jun 05 2021, David Bremner wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>>
>> testing all changed branches is somewhat hard; I did by adding
>> sleep 20 and file what effective removed created results files;
>> another change was to change 'total' to 'toxtal' in test_done,
>> then I run (at least) the following command lines:
>
> changing test_done results in pretty nonsensical output. I guess that's
> OK?

Yes.

It is very unlikely that if something is written to file not all data were
written (as the amount of data to be written is low).
A bit less unlikely is the case that output file is created but no data
written.

Should such an extremely rare cases happen it should be enough just to
report it so it get noticed (and not add any extra code to handle such
cases any further). One checking the results would probably guess that
if output is as nonsensical as it is something went badly wrong.

Tomi

>
> $  NOTMUCH_TESTS=T310-emacs.sh ./notmuch-test
>
> ...
>
> '/home/bremner/software/upstream/notmuch/test/test-results/T310-emacs' lacks 'total ...'; results may be inconsistent.
> Notmuch test suite complete.
> 72/0 tests passed.
> 1 test failed.
> -73 tests skipped.
> _______________________________________________
> notmuch mailing list -- notmuch@notmuchmail.org
> To unsubscribe send an email to notmuch-leave@notmuchmail.org

Re: [PATCH] test: aggregate-results updates

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

>
> Should such an extremely rare cases happen it should be enough just to
> report it so it get noticed (and not add any extra code to handle such
> cases any further). One checking the results would probably guess that
> if output is as nonsensical as it is something went badly wrong.
>
> Tomi
>

Patch applied to master.

d

Re: [PATCH] test: aggregate-results updates

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> -	mkdir -p "$test_results_dir"
> +	test -d "$test_results_dir" || mkdir "$test_results_dir"

Lately I've notice some complaints during parallel test running

	mkdir: cannot create directory '/home/bremner/software/upstream/notmuch/test/test-results': File exists

It seems like this change might be the culprit. Can you explain why it
was needed?

Re: [PATCH] test: aggregate-results updates

[Tomi Ollila]

On Mon, Aug 02 2021, David Bremner wrote:

> Tomi Ollila <tomi.ollila@iki.fi> writes:
>
>> -	mkdir -p "$test_results_dir"
>> +	test -d "$test_results_dir" || mkdir "$test_results_dir"
>
> Lately I've notice some complaints during parallel test running
>
> 	mkdir: cannot create directory '/home/bremner/software/upstream/notmuch/test/test-results': File exists
>
> It seems like this change might be the culprit. Can you explain why it
> was needed?

ah, parallel test non-atomicity is the problem; fix would be to restore -p to mkdir
(so it does not fail when another program executed mkdir between that test
check and mkdir execution)

i.e.

test -d "$test_results_dir" || mkdir -p "$test_results_dir"


Checking the dir with shell builtin drops need to execute mkdir in external
process, which makes the tests run a bit snappier (in small increments).


Tomi

Re: [PATCH] test: aggregate-results updates

[David Bremner]

Tomi Ollila <tomi.ollila@iki.fi> writes:

> On Mon, Aug 02 2021, David Bremner wrote:
>
>> Tomi Ollila <tomi.ollila@iki.fi> writes:
>>
>>> -	mkdir -p "$test_results_dir"
>>> +	test -d "$test_results_dir" || mkdir "$test_results_dir"
>>
>> Lately I've notice some complaints during parallel test running
>>
>> 	mkdir: cannot create directory '/home/bremner/software/upstream/notmuch/test/test-results': File exists
>>
>> It seems like this change might be the culprit. Can you explain why it
>> was needed?
>
> ah, parallel test non-atomicity is the problem; fix would be to restore -p to mkdir
> (so it does not fail when another program executed mkdir between that test
> check and mkdir execution)
>

Switched back to mkdir -p here, the occasional test failure is worse
than the potential slowdown.

d