unittest.bash: Correctly handle failures due to "errexit" in tests. This will get rid of all the "ghost flakes" where tests crashed with no apparant reason printed into our logs. Now a stack trace is printed and an easy to understand failure reason, too.

--
MOS_MIGRATED_REVID=110142957

5 files changed, 56 insertions, 8 deletions

diff --git a/scripts/bash_completion_test.sh b/scripts/bash_completion_test.sh
index d45c0b7661..3eed6fbc86 100755
--- a/scripts/bash_completion_test.sh
+++ b/scripts/bash_completion_test.sh
@@ -170,8 +170,10 @@ source ${COMPLETION}
 assert_expansion_function() {
   local ws=${PWD}
   local function="$1" displacement="$2" type="$3" expected="$4" current="$5"
-  assert_equals "$(echo -e "${expected}")" \
-      "$(eval "_bazel__${function} \"${ws}\" \"${displacement}\" \"${current}\" \"${type}\"")"
+  disable_errexit
+  local actual_result=$(eval "_bazel__${function} \"${ws}\" \"${displacement}\" \"${current}\" \"${type}\"")
+  enable_errexit
+  assert_equals "$(echo -ne "${expected}")" "${actual_result}"
 }
 
 test_expand_rules_in_package() {
diff --git a/src/test/shell/bazel/test-setup.sh b/src/test/shell/bazel/test-setup.sh
index 25a61c114b..4130740399 100755
--- a/src/test/shell/bazel/test-setup.sh
+++ b/src/test/shell/bazel/test-setup.sh
@@ -354,7 +354,6 @@ function setup_objc_test_support() {
 workspaces=()
 # Set-up a new, clean workspace with only the tools installed.
 function create_new_workspace() {
-  set -e
   new_workspace_dir=${1:-$(mktemp -d ${TEST_TMPDIR}/workspace.XXXXXXXX)}
   rm -fr ${new_workspace_dir}
   mkdir -p ${new_workspace_dir}
diff --git a/src/test/shell/testenv.sh b/src/test/shell/testenv.sh
index acb5cdda9f..7843f9a654 100755
--- a/src/test/shell/testenv.sh
+++ b/src/test/shell/testenv.sh
@@ -17,7 +17,8 @@
 # Common utility file for Bazel shell tests
 #
 
-set -eu
+# Enable errexit with pretty stack traces.
+enable_errexit
 
 # Print message in "$1" then exit with status "$2"
 die () {
diff --git a/src/test/shell/unittest.bash b/src/test/shell/unittest.bash
index dd31839c95..4caa93bc02 100644
--- a/src/test/shell/unittest.bash
+++ b/src/test/shell/unittest.bash
@@ -81,6 +81,23 @@
   { echo "unittest.bash only works with bash!" >&2; exit 1; }
 
 DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
+
+#### Configuration variables (may be overridden by testenv.sh or the suite):
+
+# This function may be called by testenv.sh or a test suite to enable errexit
+# in a way that enables us to print pretty stack traces when something fails.
+function enable_errexit() {
+  set -o errtrace
+  set -eu
+  trap __test_terminated_err ERR
+}
+
+function disable_errexit() {
+  set +o errtrace
+  set +eu
+  trap - ERR
+}
+
 source ${DIR}/testenv.sh || { echo "testenv.sh not found!" >&2; exit 1; }
 
 #### Global variables:
@@ -122,8 +139,8 @@ TEST_script="$(pwd)/$0"         # Full path to test script
 #### Internal functions
 
 function __show_log() {
-    echo "-- Actual output: ------------------------------------------------------"
-    cat $TEST_log
+    echo "-- Test log: -----------------------------------------------------------"
+    [[ -e $TEST_log ]] && cat $TEST_log || echo "(Log file did not exist.)"
     echo "------------------------------------------------------------------------"
 }
 
@@ -404,7 +421,7 @@ function __update_shards() {
 # Usage: __test_terminated <signal-number>
 # Handler that is called when the test terminated unexpectedly
 function __test_terminated() {
-   __show_log >&2
+    __show_log >&2
     echo "$TEST_name FAILED: terminated by signal $1." >&2
     TEST_passed="false"
     __show_stack
@@ -412,6 +429,33 @@ function __test_terminated() {
     exit 1
 }
 
+# Usage: __test_terminated_err
+# Handler that is called when the test terminated unexpectedly due to "errexit".
+function __test_terminated_err() {
+    # When a subshell exits due to signal ERR, its parent shell also exits,
+    # thus the signal handler is called recursively and we print out the
+    # error message and stack trace multiple times. We're only interested
+    # in the first one though, as it contains the most information, so ignore
+    # all following.
+    if [[ -f $TEST_TMPDIR/__err_handled ]]; then
+      exit 1
+    fi
+    __show_log >&2
+    if [[ ! -z "$TEST_name" ]]; then
+      echo -n "$TEST_name "
+    fi
+    echo "FAILED: terminated because this command returned a non-zero status:" >&2
+    touch $TEST_TMPDIR/__err_handled
+    TEST_passed="false"
+    __show_stack
+    # If $TEST_name is still empty, the test suite failed before we even started
+    # to run tests, so we shouldn't call tear_down.
+    if [[ ! -z "$TEST_name" ]]; then
+      tear_down
+    fi
+    exit 1
+}
+
 # Usage: __trap_with_arg <handler> <signals ...>
 # Helper to install a trap handler for several signals preserving the signal
 # number, so that the signal number is available to the trap handler.
@@ -528,6 +572,7 @@ function run_suite() {
         ATEXIT=
 
         # Run test in a subshell.
+        rm -f $TEST_TMPDIR/__err_handled
         __trap_with_arg __test_terminated INT KILL PIPE TERM ABRT FPE ILL QUIT SEGV
         (
           timestamp >$TEST_TMPDIR/__ts_start
diff --git a/third_party/ijar/test/ijar_test.sh b/third_party/ijar/test/ijar_test.sh
index 93f858690c..db9a3a7f4d 100755
--- a/third_party/ijar/test/ijar_test.sh
+++ b/third_party/ijar/test/ijar_test.sh
@@ -15,7 +15,6 @@
 # TODO(bazel-team) test that modifying the source in a non-interface
 #   changing way results in the same -interface.jar.
 
-
 DIR=$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)
 
 ## Inputs
@@ -97,6 +96,7 @@ function check_consistent_file_contents() {
   local actual="$(cat $1 | ${MD5SUM} | awk '{ print $1; }')"
   local filename="$(echo $1 | ${MD5SUM} | awk '{ print $1; }')"
   local expected="$actual"
+  disable_errexit
   if $(echo "${expected_output}" | grep -q "^${filename} "); then
     echo "${expected_output}" | grep -q "^${filename} ${actual}$" || {
       ls -l "$1"
@@ -106,6 +106,7 @@ function check_consistent_file_contents() {
     expected_output="$expected_output$filename $actual
 "
   fi
+  enable_errexit
 }
 
 function set_up() {