Harden tests' bash #5754

Ericson2314 · 2021-12-09T15:43:02Z

Motivation

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.

Context

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.

`expectStderr`

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

`grepQuiet`

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

`grepInverse`

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

`grepQuietInverse`

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

Checklist for maintainers

Maintainers: tick if completed or explain if not relevant

agreed on idea
agreed on implementation strategy
tests, as appropriate
- functional tests - tests/**.sh
- unit tests - src/*/tests
- integration tests - tests/nixos/*
documentation in the manual
code and comments are self-explanatory
commit message explains why the change was made
new feature or incompatible change: updated release notes

stale · 2022-06-13T02:15:20Z

I marked this as stale due to inactivity. → More info

roberth · 2023-02-24T06:12:21Z

Writing tests in "modern bash" will be great.

! cmd || false might look redundant, but it is in fact necessary to make set -e insist that ! cmd does in fact fail (return a non-0 exit status).

I am a bit puzzled by this. How would I reproduce this problem?

$ (set -euo pipefail; ({ ! sh -c 'exit 0'; } | cat; echo $?)); echo $?
1

$ (set -euo pipefail; ({ ! sh -c 'exit 2'; } | cat; echo $?)); echo $?
0
0

If this is related to pipefail, perhaps we could prioritize unset/-u first, although of course I'd prefer the whole thing if feasible.

Ericson2314 · 2023-02-24T13:02:34Z

It's a set -e thing, unrelated to pipefile. It just came up because pipefail means "more" -set -e, and so now these failing commands which newly have the abilty to abort the test must be dealt with.

Here is a simaple example:

bash -e -c 'true; echo foo'
bash -e -c '! true; echo bar'
bash -e -c '! true || false; echo baz'

prints

foo
bar

roberth · 2023-02-24T13:28:38Z

Perhaps we should avoid the ! + errexit/-e interaction altogether.

expectExit() {
  local r=0 expect=$1
  shift
  "$@" || r=$?
  if [[ $r != $expect ]]; then
    echo "Got exit code $r, expecting $expect from: ${*@Q}" 1>&2
    return 1
  fi
}

$ (set -e; ! true; echo bar)
bar

$ (set -e; expectExit 1 true; echo bar)
Got exit code 0, expecting 1 from: 'true'

thufschmitt

Awesome (and it managed to find a couple fishy things, so it proved its worth).

Just one real concern regarding the removal tests/plugins/libplugintest.$(SO_EXT) from the test dependencies, but looks good otherwise

thufschmitt · 2023-03-03T09:19:12Z

tests/function-trace.sh

@@ -10,7 +10,8 @@ expect_trace() {
 --trace-function-calls \
 --expr "$expr" 2>&1 \
 | grep "function-trace" \
- | sed -e 's/ [0-9]*$//'
+ | sed -e 's/ [0-9]*$//' \
+ || true


(nit) Since there's a set +e just below, I guess we can just move it a few lines above to make the whole function more consistent

Ah I have a way to just get rid of all the set +e instead 😀

Does that mean that you want to keep doing more work on this? Or do you mean as a follow-up?

I did as part of this PR (small change), but now I have a seemingly unrelated failure I couldn't reproduce just running make installcheck. Frustrating..

tests/local.mk

thufschmitt · 2023-03-03T09:22:21Z

tests/nix-shell.sh

@@ -96,6 +96,8 @@ nix develop -f "$shellDotNix" shellDrv -c echo foo |& grep -q foo
 # Test 'nix print-dev-env'.
 [[ $(nix print-dev-env -f "$shellDotNix" shellDrv --json | jq -r .variables.arr1.value[2]) = '3 4' ]]

+set +u # TODO fix?
+
 source <(nix print-dev-env -f "$shellDotNix" shellDrv)


Opened #7951 to track that

Ericson2314 · 2023-03-05T22:36:04Z

I changed one little thing (in the overall runner) and now this broken pipe error in nix flake registry | grep -q '^global showed up. Anyone have any idea what might be causing that?

roberth · 2023-03-06T00:26:29Z

I changed one little thing (in the overall runner) and now this broken pipe error in nix flake registry | grep -q '^global showed up. Anyone have any idea what might be causing that?

Could be grep -q exiting too soon, while nix still needs to write to its stdout pipe whose other end is now dead.

   -q, --quiet, --silent
         Quiet; do not write anything to standard output.  Exit immediately with zero status if any match is found, even if an error was detected.

Maybe use >/dev/null instead of -q?

Ericson2314 · 2023-03-07T18:52:16Z