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.

Co-authored-by: Robert Hensing <[email protected]>

mk/debug-test.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-set -eu
+set -eu -o pipefail
 
 test=$1
 

mk/run-test.sh

@@ -1,6 +1,6 @@
 #!/usr/bin/env bash
 
-set -u
+set -eu -o pipefail
 
 red=""
 green=""
@@ -22,8 +22,7 @@ fi
 
 run_test () {
     (init_test 2>/dev/null > /dev/null)
-    log="$(run_test_proper 2>&1)"
-    status=$?
+    log="$(run_test_proper 2>&1)" && status=0 || status=$?
 }
 
 run_test

tests/binary-cache.sh

@@ -15,11 +15,11 @@ outPath=$(nix-build dependencies.nix --no-out-link)
 nix copy --to file://$cacheDir $outPath
 
 # Test copying build logs to the binary cache.
-nix log --store file://$cacheDir $outPath 2>&1 | grep 'is not available'
+expect 1 nix log --store file://$cacheDir $outPath 2>&1 | grep 'is not available'
 nix store copy-log --to file://$cacheDir $outPath
 nix log --store file://$cacheDir $outPath | grep FOO
 rm -rf $TEST_ROOT/var/log/nix
-nix log $outPath 2>&1 | grep 'is not available'
+expect 1 nix log $outPath 2>&1 | grep 'is not available'
 nix log --substituters file://$cacheDir $outPath | grep FOO
 
 # Test copying build logs from the binary cache.
@@ -78,8 +78,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
 
@@ -126,21 +126,21 @@ clearStore
 rm -v $(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.*input-0" $TEST_ROOT/log
-grep -q "copying path.*input-2" $TEST_ROOT/log
-grep -q "copying path.*top" $TEST_ROOT/log
+grepQuiet "copying path.*input-0" $TEST_ROOT/log
+grepQuiet "copying path.*input-2" $TEST_ROOT/log
+grepQuiet "copying path.*top" $TEST_ROOT/log
 
 
 # Idem, but without cached .narinfo.
 clearStore
 clearCacheCache
 
 nix-build --substituters "file://$cacheDir" --no-require-sigs dependencies.nix -o $TEST_ROOT/result 2>&1 | tee $TEST_ROOT/log
-grep -q "don't know how to build" $TEST_ROOT/log
-grep -q "building.*input-1" $TEST_ROOT/log
-grep -q "building.*input-2" $TEST_ROOT/log
-grep -q "copying path.*input-0" $TEST_ROOT/log
-grep -q "copying path.*top" $TEST_ROOT/log
+grepQuiet "don't know how to build" $TEST_ROOT/log
+grepQuiet "building.*input-1" $TEST_ROOT/log
+grepQuiet "building.*input-2" $TEST_ROOT/log
+grepQuiet "copying path.*input-0" $TEST_ROOT/log
+grepQuiet "copying path.*top" $TEST_ROOT/log
 
 
 # Create a signed binary cache.

tests/build-delete.sh

@@ -2,8 +2,6 @@ source common.sh
 
 clearStore
 
-set -o pipefail
-
 # https://github.com/NixOS/nix/issues/6572
 issue_6572_independent_outputs() {
     nix build -f multiple-outputs.nix --json independent --no-link > $TEST_ROOT/independent.json

tests/build-dry.sh

@@ -54,7 +54,7 @@ clearCache
 
 RES=$(nix build -f dependencies.nix --dry-run --json)
 
-if [[ -z "$NIX_TESTS_CA_BY_DEFAULT" ]]; then
+if [[ -z "${NIX_TESTS_CA_BY_DEFAULT-}" ]]; then
     echo "$RES" | jq '.[0] | [
         (.drvPath | test("'$NIX_STORE_DIR'.*\\.drv")),
         (.outputs.out | test("'$NIX_STORE_DIR'"))

tests/build-remote.sh

@@ -7,7 +7,7 @@ unset NIX_STATE_DIR
 function join_by { local d=$1; shift; echo -n "$1"; shift; printf "%s" "${@/#/$d}"; }
 
 EXTRA_SYSTEM_FEATURES=()
-if [[ -n "$CONTENT_ADDRESSED" ]]; then
+if [[ -n "${CONTENT_ADDRESSED-}" ]]; then
     EXTRA_SYSTEM_FEATURES=("ca-derivations")
 fi
 
@@ -42,25 +42,26 @@ testPrintOutPath=$(nix build -L -v -f $file --no-link --print-out-paths --max-jo
 
 [[ $testPrintOutPath =~ store.*build-remote ]]
 
-set -o pipefail
-
 # Ensure that input1 was built on store1 due to the required feature.
-nix path-info --store $TEST_ROOT/machine1 --all \
-  | grep builder-build-remote-input-1.sh \
-  | grep -v builder-build-remote-input-2.sh \
-  | grep -v builder-build-remote-input-3.sh
+output=$(nix path-info --store $TEST_ROOT/machine1 --all)
+echo "$output" | grepQuiet builder-build-remote-input-1.sh
+echo "$output" | grepQuietInverse builder-build-remote-input-2.sh
+echo "$output" | grepQuietInverse builder-build-remote-input-3.sh
+unset output
 
 # Ensure that input2 was built on store2 due to the required feature.
-nix path-info --store $TEST_ROOT/machine2 --all \
-  | grep -v builder-build-remote-input-1.sh \
-  | grep builder-build-remote-input-2.sh \
-  | grep -v builder-build-remote-input-3.sh
+output=$(nix path-info --store $TEST_ROOT/machine2 --all)
+echo "$output" | grepQuietInverse builder-build-remote-input-1.sh
+echo "$output" | grepQuiet builder-build-remote-input-2.sh
+echo "$output" | grepQuietInverse builder-build-remote-input-3.sh
+unset output
 
 # Ensure that input3 was built on store3 due to the required feature.
-nix path-info --store $TEST_ROOT/machine3 --all \
-  | grep -v builder-build-remote-input-1.sh \
-  | grep -v builder-build-remote-input-2.sh \
-  | grep builder-build-remote-input-3.sh
+output=$(nix path-info --store $TEST_ROOT/machine3 --all)
+echo "$output" | grepQuietInverse builder-build-remote-input-1.sh
+echo "$output" | grepQuietInverse builder-build-remote-input-2.sh
+echo "$output" | grepQuiet builder-build-remote-input-3.sh
+unset output
 
 
 for i in input1 input3; do

tests/build.sh

@@ -2,8 +2,6 @@ source common.sh
 
 clearStore
 
-set -o pipefail
-
 # Make sure that 'nix build' returns all outputs by default.
 nix build -f multiple-outputs.nix --json a b --no-link | jq --exit-status '
   (.[0] |

tests/ca/build.sh

@@ -19,7 +19,7 @@ testRemoteCache () {
     local outPath=$(buildAttr dependentNonCA 1)
     nix copy --to file://$cacheDir $outPath
     clearStore
-    buildAttr dependentNonCA 1 --option substituters file://$cacheDir --no-require-sigs |& (! grep "building dependent-non-ca")
+    buildAttr dependentNonCA 1 --option substituters file://$cacheDir --no-require-sigs |& grepQuietInverse "building dependent-non-ca"
 }
 
 testDeterministicCA () {

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/check.sh

@@ -37,7 +37,7 @@ checkBuildTempDirRemoved $TEST_ROOT/log
 
 nix-build check.nix -A deterministic --argstr checkBuildId $checkBuildId \
     --no-out-link --check --keep-failed 2> $TEST_ROOT/log
-if grep -q 'may not be deterministic' $TEST_ROOT/log; then false; fi
+if grepQuiet 'may not be deterministic' $TEST_ROOT/log; then false; fi
 checkBuildTempDirRemoved $TEST_ROOT/log
 
 nix-build check.nix -A nondeterministic --argstr checkBuildId $checkBuildId \

tests/common.sh

@@ -1,4 +1,4 @@
-set -e
+set -eu -o pipefail
 
 if [[ -z "${COMMON_SH_SOURCED-}" ]]; then
 

tests/common/vars-and-functions.sh.in

@@ -1,4 +1,4 @@
-set -e
+set -eu -o pipefail
 
 if [[ -z "${COMMON_VARS_AND_FUNCTIONS_SH_SOURCED-}" ]]; then
 
@@ -157,7 +157,7 @@ requireDaemonNewerThan () {
 }
 
 canUseSandbox() {
-    if [[ ! $_canUseSandbox ]]; then
+    if [[ ! ${_canUseSandbox-} ]]; then
         echo "Sandboxing not supported, skipping this test..."
         return 1
     fi
@@ -170,13 +170,38 @@ 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
-    "$@" || res="$?"
+    "$@" && res=0 || res="$?"
     if [[ $res -ne $expected ]]; then
-        echo "Expected '$expected' but got '$res' while running '$*'"
+        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
@@ -191,7 +216,7 @@ needLocalStore() {
 
 # Just to make it easy to find which tests should be fixed
 buggyNeedLocalStore() {
-  needLocalStore
+  needLocalStore "$1"
 }
 
 enableFeatures() {
@@ -210,6 +235,35 @@ onError() {
     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 "$@"
+}
+
+# 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
+}
+
 trap onError ERR
 
 fi # COMMON_VARS_AND_FUNCTIONS_SH_SOURCED

tests/compute-levels.sh

@@ -3,5 +3,5 @@ source common.sh
 if [[ $(uname -ms) = "Linux x86_64" ]]; then
     # x86_64 CPUs must always support the baseline
     # microarchitecture level.
-    nix -vv --version | grep -q "x86_64-v1-linux"
+    nix -vv --version | grepQuiet "x86_64-v1-linux"
 fi

tests/db-migration.sh

@@ -2,7 +2,7 @@
 
 # Only run this if we have an older Nix available
 # XXX: This assumes that the `daemon` package is older than the `client` one
-if [[ -z "$NIX_DAEMON_PACKAGE" ]]; then
+if [[ -z "${NIX_DAEMON_PACKAGE-}" ]]; then
     exit 99
 fi
 
@@ -12,7 +12,7 @@ killDaemon
 
 # Fill the db using the older Nix
 PATH_WITH_NEW_NIX="$PATH"
-export PATH="$NIX_DAEMON_PACKAGE/bin:$PATH"
+export PATH="${NIX_DAEMON_PACKAGE}/bin:$PATH"
 clearStore
 nix-build simple.nix --no-out-link
 nix-store --generate-binary-cache-key cache1.example.org $TEST_ROOT/sk1 $TEST_ROOT/pk1

tests/dependencies.sh

@@ -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/eval-store.sh

@@ -2,7 +2,7 @@ source common.sh
 
 # Using `--eval-store` with the daemon will eventually copy everything
 # to the build store, invalidating most of the tests here
-needLocalStore
+needLocalStore "“--eval-store” doesn't achieve much with the daemon"
 
 eval_store=$TEST_ROOT/eval-store
 

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/fetchClosure.sh

@@ -42,13 +42,13 @@ if [[ "$NIX_REMOTE" != "daemon" ]]; then
 fi
 
 # 'toPath' set to empty string should fail but print the expected path.
-nix eval -v --json --expr "
+expectStderr 1 nix eval -v --json --expr "
   builtins.fetchClosure {
     fromStore = \"file://$cacheDir\";
     fromPath = $nonCaPath;
     toPath = \"\";
   }
-" 2>&1 | grep "error: rewriting.*$nonCaPath.*yielded.*$caPath"
+" | grep "error: rewriting.*$nonCaPath.*yielded.*$caPath"
 
 # If fromPath is CA, then toPath isn't needed.
 nix copy --to file://$cacheDir $caPath