lib/bats: Extract lib/bats/helper-function

First part of #156. Though it's still a work in progress, the speedup in
the tests with this new behavior added to the functions from
`lib/testing/environment` is substantial enough that I'm eager to get it
merged sooner than later. The other changes will be more extensive, so
this will reduce the amount of noise in the pull requests as well.

Also, I finally grokked Bats enough to understand why processing
`BATS_PREVIOUS_STACK_TRACE` is unnecessary. I've removed that
processing everywhere, and added a thorough comment to
`restore_bats_shell_options`.

Author: mbland

lib/bats/assertion-test-helpers

@@ -311,9 +311,5 @@ __return_from_expect_assertion() {
     unset 'BATS_CURRENT_STACK_TRACE[0]'
     set -eET
   fi
-  if [[ "${BATS_PREVIOUS_STACK_TRACE[0]}" =~ $pattern ]]; then
-    unset 'BATS_PREVIOUS_STACK_TRACE[0]'
-    set -eET
-  fi
   return "${1:-0}"
 }

lib/bats/assertions

@@ -465,10 +465,6 @@ return_from_bats_assertion() {
     unset 'BATS_CURRENT_STACK_TRACE[0]'
     set -eET
   fi
-  if [[ "${BATS_PREVIOUS_STACK_TRACE[0]}" =~ $target_stack_item_pattern ]]; then
-    unset 'BATS_PREVIOUS_STACK_TRACE[0]'
-    set -eET
-  fi
   return "$result"
 }
 

lib/bats/helper-function

@@ -0,0 +1,68 @@
+#! /usr/bin/env bash
+#
+# Utilities for writing helper functions for Bats tests
+
+# The first line of every Bats helper function must call `set` with this
+# argument. See the function comments for `restore_bats_shell_options`.
+export DISABLE_BATS_SHELL_OPTIONS='+eET'
+
+# Restore shell options disabled with `set "$DISABLE_BATS_SHELL_OPTIONS"`.
+#
+# Ensures a Bats helper function failure points to its call, not its internals.
+# This is critical for writing Bats assertion functions.
+#
+# You must ensure that `set "$DISABLE_BATS_SHELL_OPTIONS"` is in effect prior to
+# calling this function, and that your Bats helper function calls this function
+# directly through every return path (i.e. you can't delegate the call to
+# another helper function). See the comments at the top of `lib/bats/assertions`
+# for usage instructions and patterns.
+#
+# Notice that each public assertion starts with:
+#
+#   set "$DISABLE_BATS_SHELL_OPTIONS"`
+#
+# and that `DISABLE_BATS_SHELL_OPTIONS` is set to `+eET`. Bats uses `set -e`,
+# `set -E`, and `set -T` (in `tests/bats/libexec/bats-exec-test`) to make sure
+# failing commands are pinpointed and their stack traces are collected. This is
+# almost always the desired behavior.
+#
+# When it comes to test assertions, however, we want the stack trace to point to
+# the assertion call itself, not the line within its implementation at which a
+# condition triggered the failure. Otherwise, it produces a bit of mental strain
+# when reviewing test failures to identify the location of the failing assertion
+# in the test case itself.
+#
+# Starting an assertion with `set "$DISABLE_BATS_SHELL_OPTIONS"` (i.e. `set
+# +eET`) disables the `set -e`, `set -E`, and `set -T` shell options, which
+# prevents the functions and commands it calls from updating the Bats stack
+# traces. However, by itself, this still leaves the function, file, and line
+# number where the assertion was defined in the Bats stack traces. It's also
+# important to reinstate `set -eET` upon returning, but we want to make it easy
+# to write new assertions composed from existing assertions by reinstating these
+# options only when returning from the outermost assertion.
+#
+# This function solves both aspects of the problem by removing the immediate
+# caller from the Bats stack traces and reinstating `set -eET` if it is the
+# outermost assertion function, which will be the only one pushed onto the Bats
+# stacks prior to calling `set "$DISABLE_BATS_SHELL_OPTIONS"`.
+#
+# Arguments:
+#   result:  Return value of the calling assertion; defaults to 0
+restore_bats_shell_options() {
+  local result="${1:-0}"
+  local target_stack_item_pattern=" ${FUNCNAME[1]} ${BASH_SOURCE[1]}$"
+
+  # After removing our caller from BATS_CURRENT_STACK_TRACE and restoring the
+  # Bats shell options, the `return` call at the end of the function will fire
+  # `bats_debug_trap`, which sets BATS_CURRENT_STACK_TRACE to
+  # BATS_PREVIOUS_STACK_TRACE.
+  #
+  # If `result` is nonzero, `bats_error_trap` will fire and set
+  # BATS_ERROR_STACK_TRACE to BATS_PREVIOUS_STACK_TRACE and fail the test.
+  if [[ "${BATS_CURRENT_STACK_TRACE[0]}" =~ $target_stack_item_pattern ]]; then
+    unset 'BATS_CURRENT_STACK_TRACE[0]'
+    set -eET
+  fi
+  return "$result"
+}
+export -f restore_bats_shell_options

lib/testing/environment

@@ -12,6 +12,7 @@
 # `$_GO_CORE_DIR/lib/bats/helpers`. See the comments for those functions for
 # further details.
 
+. "$_GO_CORE_DIR/lib/bats/helper-function"
 . "$_GO_CORE_DIR/lib/bats/helpers"
 
 # Avoid having to fold our test strings. Tests that verify folding behavior will
@@ -40,7 +41,9 @@ readonly TEST_GO_PLUGINS_DIR="$TEST_GO_SCRIPTS_DIR/plugins"
 # Call this from `teardown` if your tests create any files or directories in
 # `TEST_GO_ROOTDIR`.
 @go.remove_test_go_rootdir() {
+  set "$DISABLE_BATS_SHELL_OPTIONS"
   remove_bats_test_dirs
+  restore_bats_shell_options "$?"
 }
 
 # Creates an executable `./go` script as `TEST_GO_SCRIPT`
@@ -51,6 +54,7 @@ readonly TEST_GO_PLUGINS_DIR="$TEST_GO_SCRIPTS_DIR/plugins"
 # Arguments:
 #   ...:  Lines comprising the `./go` script
 @go.create_test_go_script() {
+  set "$DISABLE_BATS_SHELL_OPTIONS"
   create_bats_test_script 'go' \
     ". '$_GO_CORE_DIR/go-core.bash' '$TEST_GO_SCRIPTS_RELATIVE_DIR'" \
     "$@"
@@ -60,6 +64,7 @@ readonly TEST_GO_PLUGINS_DIR="$TEST_GO_SCRIPTS_DIR/plugins"
   if [[ ! -d "$TEST_GO_SCRIPTS_DIR" ]]; then
     mkdir "$TEST_GO_SCRIPTS_DIR"
   fi
+  restore_bats_shell_options "$?"
 }
 
 # Sets `_GO_CMD` to make for neater test output when running `TEST_GO_SCRIPT`
@@ -83,7 +88,9 @@ test-go() {
 #   script_name:  Name of the command script
 #   ...:          Lines comprising the `./go` command script
 @go.create_test_command_script() {
+  set "$DISABLE_BATS_SHELL_OPTIONS"
   create_bats_test_script "$TEST_GO_SCRIPTS_RELATIVE_DIR/$1" "${@:2}"
+  restore_bats_shell_options "$?"
 }
 
 # Creates empty "parent" and "child" command scripts in `TEST_GO_SCRIPTS_DIR`
@@ -98,6 +105,7 @@ test-go() {
 #   parent_name:  Name of the "parent" command script
 #   ...:          Names of "child" command scripts
 @go.create_parent_and_subcommands() {
+  set "$DISABLE_BATS_SHELL_OPTIONS"
   local parent="$1"
   shift
   local subcommand
@@ -107,6 +115,7 @@ test-go() {
   for subcommand in "$@"; do
     @go.create_test_command_script "$parent.d/$subcommand"
   done
+  restore_bats_shell_options "$?"
 }
 
 # Assigns the results of `@go.compgen` to a caller-defined test data array
@@ -118,9 +127,8 @@ test-go() {
 #   result:  Name of the caller-declared output array
 #   ...:     Arguments passed directly to `@go.compgen`
 @go.test_compgen() {
-  . "$_GO_CORE_DIR/lib/bats/assertions"
-  set "$BATS_ASSERTION_DISABLE_SHELL_OPTIONS"
+  set "$DISABLE_BATS_SHELL_OPTIONS"
   . "$_GO_USE_MODULES" 'complete' 'strings'
   @go.split $'\n' "$(@go.compgen "${@:2}")" "$1"
-  return_from_bats_assertion
+  restore_bats_shell_options "$?"
 }

tests/bats-helper-function.bats

@@ -0,0 +1,117 @@
+#! /usr/bin/env bats
+
+load environment
+
+TEST_SCRIPT="$BATS_TMPDIR/test-script"
+NUM_ERRORS=
+
+setup() {
+  NUM_ERRORS='0'
+}
+
+teardown() {
+  rm "$TEST_SCRIPT"
+}
+
+create_test_script() {
+  local test_case_name="$1"
+  local test_case_impl=("${@:2}")
+  local script=('#! /usr/bin/env bats'
+    ''
+    ". '$_GO_CORE_DIR/lib/bats/helper-function'"
+    ''
+    'foo() {'
+    '  set "$DISABLE_BATS_SHELL_OPTIONS"'
+    '  restore_bats_shell_options "$1"'
+    '}'
+    ''
+    'bar() {'
+    '  set "$DISABLE_BATS_SHELL_OPTIONS"'
+    '  foo "$1"'
+    '  restore_bats_shell_options "$?"'
+    '}'
+    ''
+    "@test \"$test_case_name\" {"
+    "${test_case_impl[@]}"
+    '}')
+  printf '%s\n' "${script[@]}" >"$TEST_SCRIPT"
+  chmod 755 "$TEST_SCRIPT"
+}
+
+check_status() {
+  local expected="$1"
+  if [[ "$status" -ne "$expected" ]]; then
+    printf 'Expected status %d, got: %d\n' "$expected" "$status" >&2
+    ((++NUM_ERRORS))
+  fi
+}
+
+check_line() {
+  local lineno="$1"
+  local expected="$2"
+  if [[ "${lines[$lineno]}" != "$expected" ]]; then
+    printf "line %d doesn't match expected:\n" "$lineno" >&2
+    printf '  expected: "%s"\n' "$expected" >&2
+    printf '  actual:   "%s"\n' "${lines[$lineno]}" >&2
+    ((++NUM_ERRORS))
+  fi
+}
+
+check_line_matches() {
+  local lineno="$1"
+  local pattern="$2"
+  if [[ ! "${lines[$lineno]}" =~ $pattern ]]; then
+    printf "line %d doesn't match pattern:\n" "$lineno" >&2
+    printf '  pattern: "%s"\n' "$pattern" >&2
+    printf '  value:   "%s"\n' "${lines[$lineno]}" >&2
+    ((++NUM_ERRORS))
+  fi
+}
+
+finish() {
+  if [[ "$NUM_ERRORS" -ne '0' ]]; then
+    printf 'Total errors: %d\n' "$NUM_ERRORS" >&2
+    printf 'OUTPUT:\n%s\n' "$output" >&2
+    return 1
+  fi
+}
+
+@test "$SUITE: helper function passes" {
+  create_test_script 'foo passes' 'foo'
+  run "$_GO_BATS_PATH" "$TEST_SCRIPT"
+  check_status '0'
+  check_line '0' '1..1'
+  check_line '1' 'ok 1 foo passes'
+  finish
+}
+
+@test "$SUITE: helper function fails" {
+  create_test_script 'foo fails' 'foo 1'
+  run "$_GO_BATS_PATH" "$TEST_SCRIPT"
+  check_status '1'
+  check_line 0 '1..1'
+  check_line 1 'not ok 1 foo fails'
+  check_line_matches 2 "# \(in test file $TEST_SCRIPT, line [1-9][0-9]*\)" \
+  check_line 3 "#   \`foo 1' failed"
+  finish
+}
+
+@test "$SUITE: nested helper function passes" {
+  create_test_script 'bar passes' 'bar'
+  run "$_GO_BATS_PATH" "$TEST_SCRIPT"
+  check_status '0'
+  check_line '0' '1..1'
+  check_line '1' 'ok 1 bar passes'
+  finish
+}
+
+@test "$SUITE: nested helper function fails" {
+  create_test_script 'bar fails' 'bar 1'
+  run "$_GO_BATS_PATH" "$TEST_SCRIPT"
+  check_status '1'
+  check_line 0 '1..1'
+  check_line 1 'not ok 1 bar fails'
+  check_line_matches 2 "# \(in test file $TEST_SCRIPT, line [1-9][0-9]*\)" \
+  check_line 3 "#   \`bar 1' failed"
+  finish
+}