Harden tests' bash

Use `set -u` and `set -o pipefail` to catch accidental mistakes and
failures more strongly.

 - `set -u` catches the use of undefined variables
 - `set -o pipefail` catches failures (like `set -e`) earlier in the
   pipeline.

This makes the tests a bit more robust. It is nice to read code not
worrying about these spurious success paths (via uncaught) errors
undermining the tests. Indeed, I caught some bugs doing this.

There are a few tests where we run a command that should fail, and then
search its output to make sure the failure message is one that we
expect. Before, since the `grep` was the last command in the pipeline
the exit code of those failing programs was silently ignored. Now with
`set -o pipefail` it won't be, and we have to do something so the
expected failure doesn't accidentally fail the test.

To do that we use `expect` and a new `expectStderr` to check for the
exact failing exit code. See the comments on each for why.

`grep -q` is replaced with `grepQuiet`, see the comments on that
function for why.

`grep -v` when we just want the exit code is replaced with `grepInverse,
see the comments on that function for why.

`grep -q -v` together is, surprise surprise, replaced with
`grepQuietInverse`, which is both combined.

(cherry picked from commit c118361)

mk/run_test.sh

@@ -1,6 +1,6 @@
-#!/bin/sh
+#!/usr/bin/env bash
 
-set -u
+set -eu -o pipefail
 
 red=""
 green=""
@@ -15,8 +15,9 @@ if [ -t 1 ]; then
     normal=""
 fi
 (cd $(dirname $1) && env ${TESTS_ENVIRONMENT} init.sh 2>/dev/null > /dev/null)
-log="$(cd $(dirname $1) && env ${TESTS_ENVIRONMENT} $(basename $1) 2>&1)"
-status=$?
+
+log="$(cd $(dirname $1) && env ${TESTS_ENVIRONMENT} $(basename $1) 2>&1)" && status=0 || status=$?
+
 if [ $status -eq 0 ]; then
   echo "$post_run_msg [${green}PASS$normal]"
 elif [ $status -eq 99 ]; then

tests/binary-cache.sh

@@ -62,8 +62,8 @@ mv $nar $nar.good
 mkdir -p $TEST_ROOT/empty
 nix-store --dump $TEST_ROOT/empty | xz > $nar
 
-nix-build --substituters "file://$cacheDir" --no-require-sigs dependencies.nix -o $TEST_ROOT/result 2>&1 | tee $TEST_ROOT/log
-grep -q "hash mismatch" $TEST_ROOT/log
+expect 1 nix-build --substituters "file://$cacheDir" --no-require-sigs dependencies.nix -o $TEST_ROOT/result 2>&1 | tee $TEST_ROOT/log
+grepQuiet "hash mismatch" $TEST_ROOT/log
 
 mv $nar.good $nar
 
@@ -110,7 +110,7 @@ clearStore
 rm $(grep -l "StorePath:.*dependencies-input-2" $cacheDir/*.narinfo)
 
 nix-build --substituters "file://$cacheDir" --no-require-sigs dependencies.nix -o $TEST_ROOT/result 2>&1 | tee $TEST_ROOT/log
-grep -q "copying path" $TEST_ROOT/log
+grepQuiet "copying path" $TEST_ROOT/log
 
 
 if [ -n "$HAVE_SODIUM" ]; then

tests/build-dry.sh

@@ -20,7 +20,7 @@ nix build -f dependencies.nix --dry-run 2>&1 | grep "will be built"
 
 # TODO: XXX: FIXME: #1793
 # Disable this part of the test until the problem is resolved:
-if [ -n "$ISSUE_1795_IS_FIXED" ]; then
+if [ -n "${ISSUE_1795_IS_FIXED-}" ]; then
 clearStore
 clearCache
 

tests/check-refs.sh

@@ -8,14 +8,14 @@ dep=$(nix-build -o $RESULT check-refs.nix -A dep)
 
 # test1 references dep, not itself.
 test1=$(nix-build -o $RESULT check-refs.nix -A test1)
-(! nix-store -q --references $test1 | grep -q $test1)
-nix-store -q --references $test1 | grep -q $dep
+nix-store -q --references $test1 | grepQuietInverse $test1
+nix-store -q --references $test1 | grepQuiet $dep
 
 # test2 references src, not itself nor dep.
 test2=$(nix-build -o $RESULT check-refs.nix -A test2)
-(! nix-store -q --references $test2 | grep -q $test2)
-(! nix-store -q --references $test2 | grep -q $dep)
-nix-store -q --references $test2 | grep -q aux-ref
+nix-store -q --references $test2 | grepQuietInverse $test2
+nix-store -q --references $test2 | grepQuietInverse $dep
+nix-store -q --references $test2 | grepQuiet aux-ref
 
 # test3 should fail (unallowed ref).
 (! nix-build -o $RESULT check-refs.nix -A test3)

tests/check-reqs.sh

@@ -8,8 +8,8 @@ nix-build -o $RESULT check-reqs.nix -A test1
 
 (! nix-build -o $RESULT check-reqs.nix -A test2)
 (! nix-build -o $RESULT check-reqs.nix -A test3)
-(! nix-build -o $RESULT check-reqs.nix -A test4) 2>&1 | grep -q 'check-reqs-dep1'
-(! nix-build -o $RESULT check-reqs.nix -A test4) 2>&1 | grep -q 'check-reqs-dep2'
+(! nix-build -o $RESULT check-reqs.nix -A test4) 2>&1 | grepQuiet 'check-reqs-dep1'
+(! nix-build -o $RESULT check-reqs.nix -A test4) 2>&1 | grepQuiet 'check-reqs-dep2'
 (! nix-build -o $RESULT check-reqs.nix -A test5)
 (! nix-build -o $RESULT check-reqs.nix -A test6)
 

tests/common.sh.in

@@ -1,6 +1,6 @@
-set -e
+set -eu -o pipefail
 
-if [[ -z "$COMMON_SH_SOURCED" ]]; then
+if [[ -z "${COMMON_SH_SOURCED-}" ]]; then
 
 COMMON_SH_SOURCED=1
 
@@ -24,7 +24,7 @@ if [[ -n $NIX_STORE ]]; then
 fi
 export _NIX_IN_TEST=$TEST_ROOT/shared
 export _NIX_TEST_NO_LSOF=1
-export NIX_REMOTE=$NIX_REMOTE_
+export NIX_REMOTE=${NIX_REMOTE_-}
 unset NIX_PATH
 export TEST_HOME=$TEST_ROOT/test-home
 export HOME=$TEST_HOME
@@ -91,12 +91,14 @@ startDaemon() {
     # ‘nix-daemon’ should have an option to fork into the background.
     rm -f $NIX_DAEMON_SOCKET_PATH
     PATH=$DAEMON_PATH nix-daemon &
+    pidDaemon=$!
     for ((i = 0; i < 30; i++)); do
         if [[ -S $NIX_DAEMON_SOCKET_PATH ]]; then break; fi
         sleep 1
     done
-    pidDaemon=$!
     trap "killDaemon" EXIT
+    # Save for if daemon is killed
+    NIX_REMOTE_OLD=$NIX_REMOTE
     export NIX_REMOTE=daemon
 }
 
@@ -108,14 +110,15 @@ killDaemon() {
     done
     kill -9 $pidDaemon || true
     wait $pidDaemon || true
+    # Restore old nix remote
+    NIX_REMOTE=$NIX_REMOTE_OLD
     trap "" EXIT
 }
 
 restartDaemon() {
   [[ -z "${pidDaemon:-}" ]] && return 0
 
   killDaemon
-  unset NIX_REMOTE
   startDaemon
 }
 
@@ -146,7 +149,7 @@ requireDaemonNewerThan () {
 }
 
 canUseSandbox() {
-    if [[ ! $_canUseSandbox ]]; then
+    if [[ ! ${_canUseSandbox-} ]]; then
         echo "Sandboxing not supported, skipping this test..."
         return 1
     fi
@@ -159,15 +162,43 @@ fail() {
     exit 1
 }
 
+# Run a command failing if it didn't exit with the expected exit code.
+#
+# Has two advantages over the built-in `!`:
+#
+# 1. `!` conflates all non-0 codes. `expect` allows testing for an exact
+# code.
+#
+# 2. `!` unexpectedly negates `set -e`, and cannot be used on individual
+# pipeline stages with `set -o pipefail`. It only works on the entire
+# pipeline, which is useless if we want, say, `nix ...` invocation to
+# *fail*, but a grep on the error message it outputs to *succeed*.
 expect() {
     local expected res
     expected="$1"
     shift
     set +e
-    "$@"
-    res="$?"
+    "$@" && res=0 || res="$?"
     set -e
-    [[ $res -eq $expected ]]
+    if [[ $res -ne $expected ]]; then
+        echo "Expected '$expected' but got '$res' while running '${*@Q}'" >&2
+        return 1
+    fi
+    return 0
+}
+
+# Better than just doing `expect ... >&2` because the "Expected..."
+# message below will *not* be redirected.
+expectStderr() {
+    local expected res
+    expected="$1"
+    shift
+    "$@" 2>&1 && res=0 || res="$?"
+    if [[ $res -ne $expected ]]; then
+        echo "Expected '$expected' but got '$res' while running '${*@Q}'" >&2
+        return 1
+    fi
+    return 0
 }
 
 needLocalStore() {
@@ -179,7 +210,36 @@ needLocalStore() {
 
 # Just to make it easy to find which tests should be fixed
 buggyNeedLocalStore () {
-  needLocalStore
+  needLocalStore "$1"
+}
+
+# `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 "$@"
+}
+
+# A shorthand, `> /dev/null` is a bit noisy.
+#
+# `grep -q` would seem to do this, no function necessary, but it is a
+# bad fit with pipes and `set -o pipefail`: `-q` will exit after the
+# first match, and then subsequent writes will result in broken pipes.
+#
+# Note that reproducing the above is a bit tricky as it depends on
+# non-deterministic properties such as the timing between the match and
+# 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.
+grepQuiet() {
+    grep "$@" > /dev/null
+}
+
+# The previous two, combined
+grepQuietInverse() {
+    ! grep "$@" > /dev/null
 }
 
 set -x

tests/dependencies.sh

@@ -22,7 +22,7 @@ nix-store -q --graph "$outPath" > $TEST_ROOT/graph
 if test -n "$dot"; then
     # Does it parse?
     $dot < $TEST_ROOT/graph
-fi    
+fi
 
 nix-store -q --tree "$outPath" | grep '+---.*dependencies-input-2'
 
@@ -36,10 +36,10 @@ deps=$(nix-store -quR "$drvPath")
 echo "output closure contains $deps"
 
 # The output path should be in the closure.
-echo "$deps" | grep -q "$outPath"
+echo "$deps" | grepQuiet "$outPath"
 
 # Input-1 is not retained.
-if echo "$deps" | grep -q "dependencies-input-1"; then exit 1; fi
+if echo "$deps" | grepQuiet "dependencies-input-1"; then exit 1; fi
 
 # Input-2 is retained.
 input2OutPath=$(echo "$deps" | grep "dependencies-input-2")
@@ -49,4 +49,4 @@ nix-store -q --referrers-closure "$input2OutPath" | grep "$outPath"
 
 # Check that the derivers are set properly.
 test $(nix-store -q --deriver "$outPath") = "$drvPath"
-nix-store -q --deriver "$input2OutPath" | grep -q -- "-input-2.drv" 
+nix-store -q --deriver "$input2OutPath" | grepQuiet -- "-input-2.drv"

tests/export-graph.sh

@@ -4,7 +4,7 @@ clearStore
 clearProfiles
 
 checkRef() {
-    nix-store -q --references $TEST_ROOT/result | grep -q "$1"'$' || fail "missing reference $1"
+    nix-store -q --references $TEST_ROOT/result | grepQuiet "$1"'$' || fail "missing reference $1"
 }
 
 # Test the export of the runtime dependency graph.

tests/fetchurl.sh

@@ -60,7 +60,7 @@ hash=$(nix-hash --flat --type sha256 $nar)
 outPath=$(nix-build '<nix/fetchurl.nix>' --argstr url file://$nar --argstr sha256 $hash \
           --arg unpack true --argstr name xyzzy --no-out-link)
 
-echo $outPath | grep -q 'xyzzy'
+echo $outPath | grepQuiet 'xyzzy'
 
 test -x $outPath/fetchurl.sh
 test -L $outPath/symlink

tests/function-trace.sh

@@ -10,17 +10,15 @@ expect_trace() {
             --trace-function-calls \
             --expr "$expr" 2>&1 \
             | grep "function-trace" \
-            | sed -e 's/ [0-9]*$//'
-    );
+            | sed -e 's/ [0-9]*$//' \
+            || true
+    )
 
     echo -n "Tracing expression '$expr'"
-    set +e
     msg=$(diff -swB \
                <(echo "$expect") \
                <(echo "$actual")
-    );
-    result=$?
-    set -e
+    ) && result=0 || result=$?
     if [ $result -eq 0 ]; then
         echo " ok."
     else
@@ -81,5 +79,3 @@ function-trace exited undefined position at
 function-trace entered (string):1:1 at
 function-trace exited (string):1:1 at
 "
-
-set -e

tests/hash.sh

@@ -2,7 +2,7 @@ source common.sh
 
 try () {
     printf "%s" "$2" > $TEST_ROOT/vector
-    hash=$(nix hash-file --base16 $EXTRA --type "$1" $TEST_ROOT/vector)
+    hash=$(nix hash-file --base16 ${EXTRA-} --type "$1" $TEST_ROOT/vector)
     if test "$hash" != "$3"; then
         echo "hash $1, expected $3, got $hash"
         exit 1

tests/lang.sh

@@ -2,9 +2,9 @@ source common.sh
 
 export TEST_VAR=foo # for eval-okay-getenv.nix
 
-nix-instantiate --eval -E 'builtins.trace "Hello" 123' 2>&1 | grep -q Hello
-(! nix-instantiate --show-trace --eval -E 'builtins.addErrorContext "Hello" 123' 2>&1 | grep -q Hello)
-nix-instantiate --show-trace --eval -E 'builtins.addErrorContext "Hello" (throw "Foo")' 2>&1 | grep -q Hello
+nix-instantiate --eval -E 'builtins.trace "Hello" 123' 2>&1 | grepQuiet Hello
+nix-instantiate --show-trace --eval -E 'builtins.addErrorContext "Hello" 123' 2>&1 | grepQuietInverse Hello
+expectStderr 1 nix-instantiate --show-trace --eval -E 'builtins.addErrorContext "Hello" (throw "Foo")' 2>&1 | grepQuiet Hello
 
 set +x
 

tests/local.mk

@@ -31,7 +31,8 @@ nix_tests = \
   search.sh \
   nix-copy-ssh.sh \
   post-hook.sh \
-  function-trace.sh
+  function-trace.sh \
+  test-infra.sh \
   # parallel.sh
 
 install-tests += $(foreach x, $(nix_tests), tests/$(x))

tests/misc.sh

@@ -3,17 +3,17 @@ source common.sh
 # Tests miscellaneous commands.
 
 # Do all commands have help?
-#nix-env --help | grep -q install
-#nix-store --help | grep -q realise
-#nix-instantiate --help | grep -q eval
-#nix-hash --help | grep -q base32
+#nix-env --help | grepQuiet install
+#nix-store --help | grepQuiet realise
+#nix-instantiate --help | grepQuiet eval
+#nix-hash --help | grepQuiet base32
 
 # Can we ask for the version number?
 nix-env --version | grep "$version"
 
 # Usage errors.
-nix-env --foo 2>&1 | grep "no operation"
-nix-env -q --foo 2>&1 | grep "unknown flag"
+expect 1 nix-env --foo 2>&1 | grep "no operation"
+expect 1 nix-env -q --foo 2>&1 | grep "unknown flag"
 
 # Eval Errors.
-nix-instantiate --eval -E 'let a = {} // a; in a.foo' 2>&1 | grep "infinite recursion encountered, at .*(string).*:1:15$"
+expect 1 nix-instantiate --eval -E 'let a = {} // a; in a.foo' 2>&1 | grep "infinite recursion encountered, at .*(string).*:1:15$"

tests/multiple-outputs.sh

@@ -13,8 +13,8 @@ echo "evaluating c..."
 # outputs.
 drvPath=$(nix-instantiate multiple-outputs.nix -A c)
 #[ "$drvPath" = "$drvPath2" ]
-grep -q 'multiple-outputs-a.drv",\["first","second"\]' $drvPath
-grep -q 'multiple-outputs-b.drv",\["out"\]' $drvPath
+grepQuiet 'multiple-outputs-a.drv",\["first","second"\]' $drvPath
+grepQuiet 'multiple-outputs-b.drv",\["out"\]' $drvPath
 
 # While we're at it, test the ‘unsafeDiscardOutputDependency’ primop.
 outPath=$(nix-build multiple-outputs.nix -A d --no-out-link)

tests/nar-access.sh

@@ -34,8 +34,8 @@ diff -u baz.cat-nar $storePath/foo/baz
 [[ $(nix ls-store --json -R $storePath/foo/bar) = '{"type":"regular","size":0}' ]]
 
 # Test missing files.
-nix ls-store --json -R $storePath/xyzzy 2>&1 | grep 'does not exist in NAR'
-nix ls-store $storePath/xyzzy 2>&1 | grep 'does not exist'
+expect 1 nix ls-store --json -R $storePath/xyzzy 2>&1 | grep 'does not exist in NAR'
+expect 1 nix ls-store $storePath/xyzzy 2>&1 | grep 'does not exist'
 
 # Test failure to dump.
 if nix-store --dump $storePath >/dev/full ; then