From 783a8341ee2dedb8fc0790e803a5f3d0362d67d9 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Tue, 16 Jul 2024 01:28:28 +0200
Subject: [PATCH 1/6] tests/functional: Support negative codes in expect,
 expectStderr

---
 tests/functional/common/vars-and-functions.sh |  6 ++++--
 tests/functional/test-infra.sh                | 19 +++++++++++++++++++
 2 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/tests/functional/common/vars-and-functions.sh b/tests/functional/common/vars-and-functions.sh
index 4316a30d5..e5cc04bb3 100644
--- a/tests/functional/common/vars-and-functions.sh
+++ b/tests/functional/common/vars-and-functions.sh
@@ -236,7 +236,8 @@ expect() {
     expected="$1"
     shift
     "$@" && res=0 || res="$?"
-    if [[ $res -ne $expected ]]; then
+    # also match "negative" codes, which wrap around to >127
+    if [[ $res -ne $expected && $res -ne $[256 + expected] ]]; then
         echo "Expected exit code '$expected' but got '$res' from command ${*@Q}" >&2
         return 1
     fi
@@ -250,7 +251,8 @@ expectStderr() {
     expected="$1"
     shift
     "$@" 2>&1 && res=0 || res="$?"
-    if [[ $res -ne $expected ]]; then
+    # also match "negative" codes, which wrap around to >127
+    if [[ $res -ne $expected && $res -ne $[256 + expected] ]]; then
         echo "Expected exit code '$expected' but got '$res' from command ${*@Q}" >&2
         return 1
     fi
diff --git a/tests/functional/test-infra.sh b/tests/functional/test-infra.sh
index 37322b356..f6f84eae9 100755
--- a/tests/functional/test-infra.sh
+++ b/tests/functional/test-infra.sh
@@ -13,6 +13,25 @@ expect 1 false
 # `expect` will fail when we get it wrong
 expect 1 expect 0 false
 
+function ret() {
+  return $1
+}
+
+# `expect` can call functions, not just executables
+expect 0 ret 0
+expect 1 ret 1
+
+# `expect` supports negative exit codes
+expect -1 ret -1
+
+# or high positive ones, equivalent to negative ones
+expect 255 ret 255
+expect 255 ret -1
+expect -1 ret 255
+
+# but it doesn't confuse negative exit codes with positive ones
+expect 1 expect -10 ret 10
+
 noisyTrue () {
     echo YAY! >&2
     true

From f2df3f0c6c78cb742a87dbe2d2f9bcf5d5395795 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Tue, 16 Jul 2024 01:40:14 +0200
Subject: [PATCH 2/6] tests/vars-and-functions: Add callerPrefix helper

---
 tests/functional/common/vars-and-functions.sh | 39 +++++++++++++++++++
 tests/functional/test-infra.sh                |  4 ++
 2 files changed, 43 insertions(+)

diff --git a/tests/functional/common/vars-and-functions.sh b/tests/functional/common/vars-and-functions.sh
index e5cc04bb3..062f3d9f9 100644
--- a/tests/functional/common/vars-and-functions.sh
+++ b/tests/functional/common/vars-and-functions.sh
@@ -297,6 +297,45 @@ onError() {
     done
 }
 
+# Prints an error message prefix referring to the last call into this file.
+# Ignores `expect` and `expectStderr` calls.
+# Set a special exit code when test suite functions are misused, so that
+# functions like expectStderr won't mistake them for expected Nix CLI errors.
+# Suggestion: -101 (negative to indicate very abnormal, and beyond the normal
+#             range of signals)
+# Example (showns as string): 'repl.sh:123: in call to grepQuiet: '
+# This function is inefficient, so it should only be used in error messages.
+callerPrefix() {
+  # Find the closes caller that's not from this file
+  local i file line fn savedFn
+  # Use `caller`
+  for i in $(seq 0 100); do
+    caller $i > /dev/null || {
+      if [[ -n "${file:-}" ]]; then
+        echo "$file:$line: ${savedFn+in call to $savedFn: }"
+      fi
+      break
+    }
+    line="$(caller $i | cut -d' ' -f1)"
+    fn="$(caller $i | cut -d' ' -f2)"
+    file="$(caller $i | cut -d' ' -f3)"
+    if [[ $file != "${BASH_SOURCE[0]}" ]]; then
+      echo "$file:$line: ${savedFn+in call to $savedFn: }"
+      return
+    fi
+    case "$fn" in
+      # Ignore higher order functions that don't report any misuse of themselves
+      # This way a misuse of a foo in `expectStderr 1 foo` will be reported as
+      # calling foo, not expectStderr.
+      expect|expectStderr|callerPrefix)
+        ;;
+      *)
+        savedFn="$fn"
+        ;;
+    esac
+  done
+}
+
 # `grep -v` doesn't work well for exit codes. We want `!(exist line l. l
 # matches)`. It gives us `exist line l. !(l matches)`.
 #
diff --git a/tests/functional/test-infra.sh b/tests/functional/test-infra.sh
index f6f84eae9..93e0bd64b 100755
--- a/tests/functional/test-infra.sh
+++ b/tests/functional/test-infra.sh
@@ -88,6 +88,10 @@ funBang () {
 expect 1 funBang
 unset funBang
 
+# callerPrefix can be used by the test framework to improve error messages
+# it reports about our call site here
+echo "<[$(callerPrefix)]>" | grepQuiet -F "<[test-infra.sh:$LINENO: ]>"
+
 # `grep -v -q` is not what we want for exit codes, but `grepInverse` is
 # Avoid `grep -v -q`. The following line proves the point, and if it fails,
 # we'll know that `grep` had a breaking change or `-v -q` may not be portable.

From 644b97ce2574fe22a3fe14daeb6a3d0711d75731 Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Tue, 16 Jul 2024 01:41:22 +0200
Subject: [PATCH 3/6] tests/functional: Make our grep* helpers reject newlines
 in the query

Newlines behave like *OR*; not "and then".
---
 tests/functional/common/vars-and-functions.sh | 19 ++++++++++++++++---
 tests/functional/test-infra.sh                |  5 +++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/tests/functional/common/vars-and-functions.sh b/tests/functional/common/vars-and-functions.sh
index 062f3d9f9..6cce08fbc 100644
--- a/tests/functional/common/vars-and-functions.sh
+++ b/tests/functional/common/vars-and-functions.sh
@@ -336,13 +336,24 @@ callerPrefix() {
   done
 }
 
+checkGrepArgs() {
+    local arg
+    for arg in "$@"; do
+        if [[ "$arg" != "${arg//$'\n'/_}" ]]; then
+            echo "$(callerPrefix)newline not allowed in arguments; grep would try each line individually as if connected by an OR operator" >&2
+            return -101
+        fi
+    done
+}
+
 # `grep -v` doesn't work well for exit codes. We want `!(exist line l. l
 # matches)`. It gives us `exist line l. !(l matches)`.
 #
 # `!` normally doesn't work well with `set -e`, but when we wrap in a
 # function it *does*.
 grepInverse() {
-    ! grep "$@"
+    checkGrepArgs "$@" && \
+      ! grep "$@"
 }
 
 # A shorthand, `> /dev/null` is a bit noisy.
@@ -357,12 +368,14 @@ grepInverse() {
 # the producer into the pipe. But rest assured we've seen it happen in
 # CI reliably.
 grepQuiet() {
-    grep "$@" > /dev/null
+    checkGrepArgs "$@" && \
+      grep "$@" > /dev/null
 }
 
 # The previous two, combined
 grepQuietInverse() {
-    ! grep "$@" > /dev/null
+    checkGrepArgs "$@" && \
+      ! grep "$@" > /dev/null
 }
 
 # Return the number of arguments
diff --git a/tests/functional/test-infra.sh b/tests/functional/test-infra.sh
index 93e0bd64b..983b4e860 100755
--- a/tests/functional/test-infra.sh
+++ b/tests/functional/test-infra.sh
@@ -108,3 +108,8 @@ unset res
 res=$(set -eu -o pipefail; echo foo | expect 1 grepQuietInverse foo | wc -c)
 (( res == 0 ))
 unset res
+
+# `grepQuiet` does not allow newlines in its arguments, because grep quietly
+# treats them as multiple queries.
+( echo foo; echo bar; ) | expectStderr -101 grepQuiet $'foo\nbar' \
+  | grepQuiet -E 'test-infra\.sh:[0-9]+: in call to grepQuiet: newline not allowed in arguments; grep would try each line individually as if connected by an OR operator'

From 41a03738d63b94366d96dfc3e5cfb052c0ad5e2a Mon Sep 17 00:00:00 2001
From: Robert Hensing <robert@roberthensing.nl>
Date: Tue, 16 Jul 2024 01:54:12 +0200
Subject: [PATCH 4/6] tests/functional: Also keep plain grep calls safe from
 newlines

---
 tests/functional/common/vars-and-functions.sh | 20 ++++++++++++++++---
 tests/functional/test-infra.sh                |  4 ++++
 2 files changed, 21 insertions(+), 3 deletions(-)

diff --git a/tests/functional/common/vars-and-functions.sh b/tests/functional/common/vars-and-functions.sh
index 6cce08fbc..7a399f6d4 100644
--- a/tests/functional/common/vars-and-functions.sh
+++ b/tests/functional/common/vars-and-functions.sh
@@ -351,9 +351,12 @@ checkGrepArgs() {
 #
 # `!` normally doesn't work well with `set -e`, but when we wrap in a
 # function it *does*.
+#
+# `command grep` lets us avoid re-checking the args by going directly to the
+# executable.
 grepInverse() {
     checkGrepArgs "$@" && \
-      ! grep "$@"
+      ! command grep "$@"
 }
 
 # A shorthand, `> /dev/null` is a bit noisy.
@@ -367,15 +370,26 @@ grepInverse() {
 # the closing of the pipe, the buffering of the pipe, and the speed of
 # the producer into the pipe. But rest assured we've seen it happen in
 # CI reliably.
+#
+# `command grep` lets us avoid re-checking the args by going directly to the
+# executable.
 grepQuiet() {
     checkGrepArgs "$@" && \
-      grep "$@" > /dev/null
+      command grep "$@" > /dev/null
 }
 
 # The previous two, combined
 grepQuietInverse() {
     checkGrepArgs "$@" && \
-      ! grep "$@" > /dev/null
+      ! command grep "$@" > /dev/null
+}
+
+# Wrap grep to remove its newline footgun; see checkGrepArgs.
+# Note that we keep the checkGrepArgs calls in the other helpers, because some
+# of them are negated and that would defeat this check.
+grep() {
+    checkGrepArgs "$@" && \
+      command grep "$@"
 }
 
 # Return the number of arguments
diff --git a/tests/functional/test-infra.sh b/tests/functional/test-infra.sh
index 983b4e860..1dab069fb 100755
--- a/tests/functional/test-infra.sh
+++ b/tests/functional/test-infra.sh
@@ -113,3 +113,7 @@ unset res
 # treats them as multiple queries.
 ( echo foo; echo bar; ) | expectStderr -101 grepQuiet $'foo\nbar' \
   | grepQuiet -E 'test-infra\.sh:[0-9]+: in call to grepQuiet: newline not allowed in arguments; grep would try each line individually as if connected by an OR operator'
+
+# We took the blue pill and woke up in a world where `grep` is moderately safe.
+expectStderr -101 grep $'foo\nbar' \
+  | grepQuiet -E 'test-infra\.sh:[0-9]+: in call to grep: newline not allowed in arguments; grep would try each line individually as if connected by an OR operator'

From 4457cebe05c444facc68db281c58f90a87f785ce Mon Sep 17 00:00:00 2001
From: Robert Hensing <roberth@users.noreply.github.com>
Date: Tue, 23 Jul 2024 10:24:18 +0200
Subject: [PATCH 5/6] Update comment in tests//vars-and-functions.sh

Co-authored-by: tomberek <tomberek@users.noreply.github.com>
---
 tests/functional/common/vars-and-functions.sh | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/tests/functional/common/vars-and-functions.sh b/tests/functional/common/vars-and-functions.sh
index 7a399f6d4..954435593 100644
--- a/tests/functional/common/vars-and-functions.sh
+++ b/tests/functional/common/vars-and-functions.sh
@@ -306,7 +306,8 @@ onError() {
 # Example (showns as string): 'repl.sh:123: in call to grepQuiet: '
 # This function is inefficient, so it should only be used in error messages.
 callerPrefix() {
-  # Find the closes caller that's not from this file
+  # Find the closest caller that's not from this file
+  # using the bash `caller` builtin.
   local i file line fn savedFn
   # Use `caller`
   for i in $(seq 0 100); do

From baa28159d316e2c506fee8a82f66726c9305f68a Mon Sep 17 00:00:00 2001
From: Robert Hensing <roberth@users.noreply.github.com>
Date: Thu, 25 Jul 2024 15:38:02 +0200
Subject: [PATCH 6/6] Update tests/functional/test-infra.sh

Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
---
 tests/functional/test-infra.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/functional/test-infra.sh b/tests/functional/test-infra.sh
index 1dab069fb..d02a11b46 100755
--- a/tests/functional/test-infra.sh
+++ b/tests/functional/test-infra.sh
@@ -111,7 +111,7 @@ unset res
 
 # `grepQuiet` does not allow newlines in its arguments, because grep quietly
 # treats them as multiple queries.
-( echo foo; echo bar; ) | expectStderr -101 grepQuiet $'foo\nbar' \
+{ echo foo; echo bar; } | expectStderr -101 grepQuiet $'foo\nbar' \
   | grepQuiet -E 'test-infra\.sh:[0-9]+: in call to grepQuiet: newline not allowed in arguments; grep would try each line individually as if connected by an OR operator'
 
 # We took the blue pill and woke up in a world where `grep` is moderately safe.