Preserve scratch artifacts after runner and hook failures

CHANGELOG.md

@@ -1,5 +1,9 @@
 # 0.7.1
 
+* Retain scratch data for hook failures, aborts, invalid runner settings, and report
+  errors under `--keep-tmpdir=failure`, including handled runner failures. Preserve
+  original exit statuses and avoid cleanup of unresolved scratch paths.
+
 * Honor native Bazel test filters, sharding, and runner fail-fast. Validate filters
   and shard settings before hooks run, acknowledge shard support, keep reports
   local to each shard, and allow empty shards without masking an empty selection.

README.md

@@ -17,7 +17,9 @@ Run one of the following commands to get detailed information on the actual bash
 The flags can be used on the `bazel run` and `bazel test` commands (the latter requiring `--test_arg=...`).
 
 Use `--keep-tmpdir=never|failure|always` to control scratch retention; the default is `failure`.
-A retained path is printed at shutdown. Bazel itself may still remove a sandbox unless the test runs
+Failure retention includes nonzero exits, failed cases or hooks, aborted runs, invalid runner settings,
+and report-writing errors, even when the runner status is handled by its caller. `never` deletes
+owned scratch data on failure; `always` retains it on success too. A retained path is printed at shutdown. Bazel itself may still remove a sandbox unless the test runs
 locally or with `--sandbox_debug`.
 
 ### Functionality

bashtest/README.md

@@ -27,6 +27,23 @@ The macro supports directory and manifest-only runfiles on Linux, macOS, and
 Windows x86-64 with an automatically provisioned MSYS2 runtime. See the root [installation requirements](../README.md#installation-and-requirements)
 for Windows setup and resolving data dependencies with `rlocation`.
 
+## Scratch retention
+
+`--keep-tmpdir=failure` is the default. It preserves the runner-owned
+`${BASHTEST_TMPDIR}` after a nonzero process exit, a failed case or lifecycle hook,
+an aborted run, invalid runner settings, or a report-writing error. Handling a
+nonzero `test_runner` return does not erase that failure for retention purposes.
+Failed expectations outside the runner also retain their scratch data, including
+failures recorded by waited descendant shells. Successful runs remove it.
+
+`--keep-tmpdir=never` removes owned scratch data even on failure;
+`--keep-tmpdir=always` preserves it even on success. Cleanup preserves the original
+nonzero process exit status, including signal-derived statuses. If creating the
+scratch directory fails, cleanup leaves the caller's `${TEST_TMPDIR}` alone.
+Only the runner-created directory is removed, and retained paths are printed to
+stderr. Bazel may still delete its sandbox; use local execution or
+`--sandbox_debug` when investigating retained artifacts.
+
 ## Command failure handling
 
 `expect_success command args...` checks for exit status 0.

bashtest/bashtest.sh

@@ -271,6 +271,8 @@ _BASHTEST_HAS_ERROR=0
 _BASHTEST_SUGGEST_UPDATE=0
 _BASHTEST_INIT_FAILED=0
 _BASHTEST_DONE_FAILED=0
+_BASHTEST_RUNNER_FAILED=0
+_BASHTEST_OWNS_TMPDIR=0
 _BASHTEST_XML_RUNNING=0
 _BASHTEST_XML_STARTED=0
 _BASHTEST_XML_ACTIVE=
@@ -302,15 +304,21 @@ function _bashtest_cleanup () {
         echo >&2 "ERROR: Cannot write JUnit report: ${_BASHTEST_XML_PATH}"
         [[ "${status}" != 0 ]] || status=1
     fi
-    local keep=0
-    if [[ "${_BASHTEST_KEEP_TMPDIR}" == "always" ]] \
-        || { [[ "${_BASHTEST_KEEP_TMPDIR}" == "failure" ]] && [[ "${_BASHTEST_NUM_FAIL}" != "0" ]]; }; then
-        keep=1
-    fi
-    if [[ "${keep}" == "0" ]]; then
-        rm -rf "${BASHTEST_TMPDIR}"
-    else
-        echo >&2 "Preserved test scratch directory: ${BASHTEST_TMPDIR}"
+    # Scratch creation can itself fail. Never clean up an unset/unresolved path.
+    if [[ "${_BASHTEST_OWNS_TMPDIR}" == 1 ]]; then
+        local keep=0
+        if [[ "${_BASHTEST_KEEP_TMPDIR}" == always ]]; then
+            keep=1
+        elif [[ "${_BASHTEST_KEEP_TMPDIR}" == failure ]]; then
+            if [[ "${status}" != 0 || "${_BASHTEST_RUNNER_FAILED}" != 0 || "${_BASHTEST_NUM_FAIL}" != 0 || "${_BASHTEST_INIT_FAILED}" != 0 || "${_BASHTEST_DONE_FAILED}" != 0 || "${_BASHTEST_HAS_ERROR}" != 0 ]] || [[ -s "${_BASHTEST_FAILURE_FILE:-}" ]]; then
+                keep=1
+            fi
+        fi
+        if [[ "${keep}" == 0 ]]; then
+            rm -rf "${BASHTEST_TMPDIR}"
+        else
+            echo >&2 "Preserved test scratch directory: ${BASHTEST_TMPDIR}"
+        fi
     fi
     exit "${status}"
 }
@@ -323,6 +331,7 @@ trap 'exit 143' TERM
 mkdir -p "${TEST_TMPDIR:=/tmp/$(date "+%Y%m%dT%H%M%S")}"
 BASHTEST_TMPDIR="$(mktemp -d -p "${TEST_TMPDIR}")"
 declare -r BASHTEST_TMPDIR
+_BASHTEST_OWNS_TMPDIR=1
 # Descendant shells inherit this path, so expectation failures survive subshells.
 _BASHTEST_FAILURE_FILE="${BASHTEST_TMPDIR}/current-failure"
 : > "${_BASHTEST_FAILURE_FILE}"
@@ -693,6 +702,7 @@ function test_runner() {
     if [[ "${_BASHTEST_STRICT_COMMANDS}" == 1 ]]; then
         _bashtest_strict_check_context
         if [[ "${_BASHTEST_STRICT_CONTEXT_OK}" == 0 ]]; then
+            _BASHTEST_RUNNER_FAILED=1
             echo >&2 'ERROR: --strict-commands requires a standalone test_runner call, outside conditional or status-masking contexts.'
             _bashtest_xml_case error test_runner "Strict command checking is disabled by a conditional runner call."
             _BASHTEST_XML_RUNNING=0
@@ -701,6 +711,7 @@ function test_runner() {
         fi
     fi
     if ! _bashtest_selection_init; then
+        _BASHTEST_RUNNER_FAILED=1
         _bashtest_xml_case error test_runner "Invalid test selection or sharding configuration."
         _BASHTEST_XML_RUNNING=0
         _bashtest_xml_write || return 1
@@ -771,6 +782,7 @@ function test_runner() {
         _bashtest_xml_case error test_runner "No tests were run."
     fi
     if ! _bashtest_xml_write; then
+        _BASHTEST_RUNNER_FAILED=1
         echo >&2 "ERROR: Cannot write JUnit report: ${_BASHTEST_XML_PATH}"
         return 1
     fi
@@ -780,10 +792,12 @@ function test_runner() {
     echo >&2 "FAIL: ${_BASHTEST_NUM_FAIL}"
     echo "----------"
     if [[ "${_BASHTEST_INIT_FAILED}" -ne "0" ]]; then
+        _BASHTEST_RUNNER_FAILED=1
         echo >&2 "ERROR: Test initialization failed."
         return 1
     fi
     if [[ "${_BASHTEST_NUM_FAIL}" != "0" ]]; then
+        _BASHTEST_RUNNER_FAILED=1
         echo >&2 "ERROR: Some tests failed."
         if [[ "${_BASHTEST_SUGGEST_UPDATE}" -gt 0 ]]; then
             echo ""
@@ -793,10 +807,12 @@ function test_runner() {
         fi
         return 1
     elif [[ "${empty_shard}" == 0 && "${_BASHTEST_NUM_PASS}" == 0 ]]; then
+        _BASHTEST_RUNNER_FAILED=1
         echo >&2 "ERROR: No tests were run."
         return 2
     fi
     if [[ "${_BASHTEST_DONE_FAILED}" != "0" ]]; then
+        _BASHTEST_RUNNER_FAILED=1
         echo >&2 "ERROR: Test shutdown failed."
         return 1
     fi

bashtest/tests/junit_test.sh

@@ -218,4 +218,53 @@ PROBE
     expect_output_not_contains INIT "${output}"
 }
 
+# shellcheck disable=SC2317 # Functions are discovered by test_runner.
+test::scratch_retention() {
+    local root probe status expected output scratch mode retained
+    root="$(test_tmpdir retention)"
+    cat > "${root}/probe.sh" <<'PROBE'
+set -euo pipefail
+# shellcheck disable=SC1090,SC1091,SC2154
+source "${mboworks_bashtest}"
+printf '%s' "${BASHTEST_TMPDIR}" > "${SCRATCH_RECORD}"
+printf evidence > "${BASHTEST_TMPDIR}/evidence"
+test::test_init() { if [[ "${PROBE}" == setup ]]; then return 7; fi; }
+test::body() {
+    case "${PROBE}" in
+        failure) expect_eq yes no ;;
+        abort) exit 42 ;;
+        report) rmdir "${REPORT_DIRECTORY}" ;;
+    esac
+}
+test::test_done() { if [[ "${PROBE}" == teardown ]]; then return 7; fi; }
+test_runner
+PROBE
+    for probe in pass failure setup teardown abort report; do
+        for mode in failure never always; do
+            status=0
+            expected=1
+            [[ "${probe}" != pass ]] || expected=0
+            [[ "${probe}" != abort ]] || expected=42
+            output="$(_junit_probe PROBE="${probe}" SCRATCH_RECORD="${root}/scratch.path" \
+                REPORT_DIRECTORY="${root}/reports-${probe}-${mode}" GTEST_OUTPUT="xml:${root}/reports-${probe}-${mode}/result.xml" \
+                "${BASH}" "${root}/probe.sh" --keep-tmpdir="${mode}" 2>&1)" || status="${?}"
+            expect_eq "${expected}" "${status}"
+            if [[ "${probe}" == report ]]; then
+                expect_output_contains 'Cannot write JUnit report' "${output}"
+            fi
+            scratch="$(cat "${root}/scratch.path")"
+            retained=0
+            if [[ "${mode}" == always || ( "${mode}" == failure && "${expected}" != 0 ) ]]; then retained=1; fi
+            if [[ "${retained}" == 1 ]]; then
+                expect_success test -d "${scratch}"
+                expect_eq evidence "$(cat "${scratch}/evidence")"
+                expect_output_contains "Preserved test scratch directory: ${scratch}" "${output}"
+            else
+                expect_success test ! -e "${scratch}"
+                expect_output_not_contains 'Preserved test scratch directory:' "${output}"
+            fi
+        done
+    done
+}
+
 test_runner

tools/junit_test.py

@@ -387,6 +387,75 @@ def test_write_failure_after_passing_tests_remains_failure(self):
         self.assertIn(b'Cannot write JUnit report', result.stderr)
         self.assertFalse((directory / 'result.xml').exists())
 
+    def test_scratch_retention_modes_and_original_exit_status(self):
+        record = self.root / 'scratch.path'
+        prefix = 'printf "%s" "$BASHTEST_TMPDIR" > "$SCRATCH_RECORD"\n' \
+                 'printf evidence > "$BASHTEST_TMPDIR/evidence"\n'
+        for output in (True, False):
+            for mode in ('never', 'failure', 'always'):
+                for command, status in ((':', 0), ('return 7', 1), ('exit 42', 42)):
+                    with self.subTest(output=output, mode=mode, command=command):
+                        result = self.run_probe(prefix + f'test::body() {{ {command}; }}',
+                                                f'--keep-tmpdir={mode}', output=output,
+                                                SCRATCH_RECORD=str(record))
+                        self.assertEqual(result.returncode, status, result.stderr)
+                        scratch = Path(record.read_text())
+                        retained = mode == 'always' or (mode == 'failure' and status != 0)
+                        self.assertEqual(scratch.exists(), retained)
+                        self.assertEqual(b'Preserved test scratch directory:' in result.stderr, retained)
+                        if retained:
+                            self.assertEqual((scratch / 'evidence').read_text(), 'evidence')
+
+    def test_scratch_retained_for_hooks_aborts_and_invalid_selection(self):
+        record = self.root / 'scratch.path'
+        prefix = 'printf "%s" "$BASHTEST_TMPDIR" > "$SCRATCH_RECORD"\n'
+        for output in (True, False):
+            for body, args, status in (
+                    ('test::test_init() { return 3; }\ntest::body() { :; }', (), 1),
+                    ('test::test_done() { return 3; }\ntest::body() { :; }', (), 1),
+                    ('test::body() { exit 0; }', (), 1),
+                    ('test::body() { echo "$UNDEFINED_RETENTION_PROBE"; }', (), 1),
+                    ('test::body() { kill -TERM $$; }', (), 143),
+                    ('test::body() { :; }', ('--test_filter=[',), 1),
+                    ('test::body() { :; }', ('--test_filter=absent',), 2)):
+                with self.subTest(output=output, body=body, args=args):
+                    result = self.run_probe(prefix + body, *args, output=output,
+                                            SCRATCH_RECORD=str(record))
+                    self.assertEqual(result.returncode, status, result.stderr)
+                    self.assertTrue(Path(record.read_text()).is_dir())
+                    self.assertIn(b'Preserved test scratch directory:', result.stderr)
+        # A conditional caller can handle the runner status, but the diagnostic
+        # artifacts must survive even when the program ultimately exits 0.
+        result = self.run_probe(prefix + 'test::body() { :; }', '--test_filter=[',
+                                output=False, runner='test_runner || :', SCRATCH_RECORD=str(record))
+        self.assertEqual(result.returncode, 0, result.stderr)
+        self.assertTrue(Path(record.read_text()).is_dir())
+
+    def test_scratch_retained_for_report_write_errors(self):
+        record = self.root / 'scratch.path'
+        directory = self.root / 'report directory'
+        prefix = 'printf "%s" "$BASHTEST_TMPDIR" > "$SCRATCH_RECORD"\n'
+        result = self.run_probe(prefix + 'test::body() { rmdir "$REPORT_DIRECTORY"; }',
+                                f'--gtest_output=xml:{directory}/result.xml',
+                                SCRATCH_RECORD=str(record), REPORT_DIRECTORY=str(directory))
+        self.assertEqual(result.returncode, 1, result.stderr)
+        self.assertTrue(Path(record.read_text()).is_dir())
+        self.assertIn(b'Cannot write JUnit report', result.stderr)
+
+    def test_scratch_creation_failure_never_cleans_the_caller_path(self):
+        blocked = self.root / 'not a directory'
+        blocked.write_text('caller-owned')
+        caller = self.root / 'caller scratch'
+        caller.mkdir()
+        sentinel = caller / 'sentinel'
+        sentinel.write_text('preserve me')
+        result = self.run_probe('', output=False, TEST_TMPDIR=str(blocked), BASHTEST_TMPDIR=str(caller))
+        self.assertNotEqual(result.returncode, 0)
+        self.assertNotIn(b'unbound variable', result.stderr)
+        self.assertNotIn(b'Preserved test scratch directory:', result.stderr)
+        self.assertEqual(blocked.read_text(), 'caller-owned')
+        self.assertEqual(sentinel.read_text(), 'preserve me')
+
     def test_disabled_reports_and_invalid_output_fail_closed(self):
         for environment in ({'GTEST_OUTPUT': ''}, {'output': False}):
             result = self.run_probe('test::pass() { :; }', **environment)