From bfb9eb87fe98f96969188df9df866e15800bd55b Mon Sep 17 00:00:00 2001
From: John Ericson <John.Ericson@Obsidian.Systems>
Date: Thu, 16 Mar 2023 16:00:20 -0400
Subject: [PATCH] Cleanup	test skipping

- Try not to put cryptic "99" in many places

  Factor out `exit 99` into `skipTest` function

- Alows make sure skipping a test is done with a reason

  `skipTest` takes a mandatory argument

- Separate pure conditionals vs side-effectful test skipping.

  "require daemon" already had this, but "sandbox support" did not.
---
 tests/build-remote.sh                 |  4 ++--
 tests/common/vars-and-functions.sh.in | 25 ++++++++++++++++---------
 tests/db-migration.sh                 |  6 +++---
 tests/fetchGit.sh                     |  5 +----
 tests/fetchGitRefs.sh                 |  5 +----
 tests/fetchGitSubmodules.sh           |  5 +----
 tests/fetchMercurial.sh               |  5 +----
 tests/flakes/common.sh                |  7 -------
 tests/flakes/mercurial.sh             |  5 +----
 tests/gc-runtime.sh                   |  2 +-
 tests/linux-sandbox.sh                |  4 ++--
 tests/plugins.sh                      |  3 +--
 tests/recursive.sh                    |  2 +-
 tests/shell.sh                        |  2 +-
 tests/user-envs-migration.sh          |  2 +-
 15 files changed, 33 insertions(+), 49 deletions(-)

diff --git a/tests/build-remote.sh b/tests/build-remote.sh
index 6da128d1b..78e12b477 100644
--- a/tests/build-remote.sh
+++ b/tests/build-remote.sh
@@ -1,5 +1,5 @@
-if ! canUseSandbox; then exit 99; fi
-if ! [[ $busybox =~ busybox ]]; then exit 99; fi
+requireSandboxSupport
+[[ $busybox =~ busybox ]] || skipTest "no busybox"
 
 unset NIX_STORE_DIR
 unset NIX_STATE_DIR
diff --git a/tests/common/vars-and-functions.sh.in b/tests/common/vars-and-functions.sh.in
index e4a773f34..a9e6c802f 100644
--- a/tests/common/vars-and-functions.sh.in
+++ b/tests/common/vars-and-functions.sh.in
@@ -152,21 +152,29 @@ isDaemonNewer () {
   [[ $(nix eval --expr "builtins.compareVersions ''$daemonVersion'' ''$requiredVersion''") -ge 0 ]]
 }
 
+skipTest () {
+    echo "$1, skipping this test..." >&2
+    exit 99
+}
+
 requireDaemonNewerThan () {
-  isDaemonNewer "$1" || exit 99
+    isDaemonNewer "$1" || skipTest "Daemon is too old"
 }
 
 canUseSandbox() {
-    if [[ ! ${_canUseSandbox-} ]]; then
-        echo "Sandboxing not supported, skipping this test..."
-        return 1
-    fi
+    [[ ${_canUseSandbox-} ]]
+}
 
-    return 0
+requireSandboxSupport () {
+    canUseSandbox || skipTest "Sandboxing not supported"
+}
+
+requireGit() {
+    [[ $(type -p git) ]] || skipTest "Git not installed"
 }
 
 fail() {
-    echo "$1"
+    echo "$1" >&2
     exit 1
 }
 
@@ -209,8 +217,7 @@ expectStderr() {
 
 needLocalStore() {
   if [[ "$NIX_REMOTE" == "daemon" ]]; then
-    echo "Can’t run through the daemon ($1), skipping this test..."
-    return 99
+    skipTest "Can’t run through the daemon ($1)"
   fi
 }
 
diff --git a/tests/db-migration.sh b/tests/db-migration.sh
index 7d243eefb..44cd16bc0 100644
--- a/tests/db-migration.sh
+++ b/tests/db-migration.sh
@@ -1,13 +1,13 @@
 # Test that we can successfully migrate from an older db schema
 
+source common.sh
+
 # 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
-    exit 99
+    skipTest "not using the Nix daemon"
 fi
 
-source common.sh
-
 killDaemon
 
 # Fill the db using the older Nix
diff --git a/tests/fetchGit.sh b/tests/fetchGit.sh
index a7a8df186..e2ccb0e97 100644
--- a/tests/fetchGit.sh
+++ b/tests/fetchGit.sh
@@ -1,9 +1,6 @@
 source common.sh
 
-if [[ -z $(type -p git) ]]; then
-    echo "Git not installed; skipping Git tests"
-    exit 99
-fi
+requireGit
 
 clearStore
 
diff --git a/tests/fetchGitRefs.sh b/tests/fetchGitRefs.sh
index a0d86ca5e..d643fea04 100644
--- a/tests/fetchGitRefs.sh
+++ b/tests/fetchGitRefs.sh
@@ -1,9 +1,6 @@
 source common.sh
 
-if [[ -z $(type -p git) ]]; then
-    echo "Git not installed; skipping Git tests"
-    exit 99
-fi
+requireGit
 
 clearStore
 
diff --git a/tests/fetchGitSubmodules.sh b/tests/fetchGitSubmodules.sh
index 08ccaa3cd..df81232e5 100644
--- a/tests/fetchGitSubmodules.sh
+++ b/tests/fetchGitSubmodules.sh
@@ -2,10 +2,7 @@ source common.sh
 
 set -u
 
-if [[ -z $(type -p git) ]]; then
-    echo "Git not installed; skipping Git submodule tests"
-    exit 99
-fi
+requireGit
 
 clearStore
 
diff --git a/tests/fetchMercurial.sh b/tests/fetchMercurial.sh
index 5c64ffd26..e6f8525c6 100644
--- a/tests/fetchMercurial.sh
+++ b/tests/fetchMercurial.sh
@@ -1,9 +1,6 @@
 source common.sh
 
-if [[ -z $(type -p hg) ]]; then
-    echo "Mercurial not installed; skipping Mercurial tests"
-    exit 99
-fi
+[[ $(type -p hq) ]] || skipTest "Mercurial not installed"
 
 clearStore
 
diff --git a/tests/flakes/common.sh b/tests/flakes/common.sh
index 58616ac4c..427abcdde 100644
--- a/tests/flakes/common.sh
+++ b/tests/flakes/common.sh
@@ -2,13 +2,6 @@ source ../common.sh
 
 registry=$TEST_ROOT/registry.json
 
-requireGit() {
-    if [[ -z $(type -p git) ]]; then
-        echo "Git not installed; skipping flake tests"
-        exit 99
-    fi
-}
-
 writeSimpleFlake() {
     local flakeDir="$1"
     cat > $flakeDir/flake.nix <<EOF
diff --git a/tests/flakes/mercurial.sh b/tests/flakes/mercurial.sh
index 2614006c8..0622c79b7 100644
--- a/tests/flakes/mercurial.sh
+++ b/tests/flakes/mercurial.sh
@@ -1,9 +1,6 @@
 source ./common.sh
 
-if [[ -z $(type -p hg) ]]; then
-    echo "Mercurial not installed; skipping"
-    exit 99
-fi
+[[ $(type -p hq) ]] || skipTest "Mercurial not installed"
 
 flake1Dir=$TEST_ROOT/flake-hg1
 mkdir -p $flake1Dir
diff --git a/tests/gc-runtime.sh b/tests/gc-runtime.sh
index 6094959cb..dc1826a55 100644
--- a/tests/gc-runtime.sh
+++ b/tests/gc-runtime.sh
@@ -4,7 +4,7 @@ case $system in
     *linux*)
         ;;
     *)
-        exit 99;
+        skipTest "Not running Linux";
 esac
 
 set -m # enable job control, needed for kill
diff --git a/tests/linux-sandbox.sh b/tests/linux-sandbox.sh
index 5667000d9..5a2cf7abd 100644
--- a/tests/linux-sandbox.sh
+++ b/tests/linux-sandbox.sh
@@ -4,13 +4,13 @@ needLocalStore "the sandbox only runs on the builder side, so it makes no sense
 
 clearStore
 
-if ! canUseSandbox; then exit 99; fi
+requireSandboxSupport
 
 # Note: we need to bind-mount $SHELL into the chroot. Currently we
 # only support the case where $SHELL is in the Nix store, because
 # otherwise things get complicated (e.g. if it's in /bin, do we need
 # /lib as well?).
-if [[ ! $SHELL =~ /nix/store ]]; then exit 99; fi
+if [[ ! $SHELL =~ /nix/store ]]; then skipTest "Shell is not from Nix store"; fi
 
 chmod -R u+w $TEST_ROOT/store0 || true
 rm -rf $TEST_ROOT/store0
diff --git a/tests/plugins.sh b/tests/plugins.sh
index 805f49f01..baf71a362 100644
--- a/tests/plugins.sh
+++ b/tests/plugins.sh
@@ -1,8 +1,7 @@
 source common.sh
 
 if [[ $BUILD_SHARED_LIBS != 1 ]]; then
-    echo "plugins are not supported"
-    exit 99
+    skipTest "Plugins are not supported"
 fi
 
 res=$(nix --option setting-set true --option plugin-files $PWD/plugins/libplugintest* eval --expr builtins.anotherNull)
diff --git a/tests/recursive.sh b/tests/recursive.sh
index 91518d67d..6335d44a5 100644
--- a/tests/recursive.sh
+++ b/tests/recursive.sh
@@ -4,7 +4,7 @@ sed -i 's/experimental-features .*/& recursive-nix/' "$NIX_CONF_DIR"/nix.conf
 restartDaemon
 
 # FIXME
-if [[ $(uname) != Linux ]]; then exit 99; fi
+if [[ $(uname) != Linux ]]; then skipTest "Not running Linux"; fi
 
 clearStore
 
diff --git a/tests/shell.sh b/tests/shell.sh
index 6a80e8385..d2f7cf14e 100644
--- a/tests/shell.sh
+++ b/tests/shell.sh
@@ -10,7 +10,7 @@ nix shell -f shell-hello.nix hello -c hello NixOS | grep 'Hello NixOS'
 nix shell -f shell-hello.nix hello^dev -c hello2 | grep 'Hello2'
 nix shell -f shell-hello.nix 'hello^*' -c hello2 | grep 'Hello2'
 
-if ! canUseSandbox; then exit 99; fi
+requireSandboxSupport
 
 chmod -R u+w $TEST_ROOT/store0 || true
 rm -rf $TEST_ROOT/store0
diff --git a/tests/user-envs-migration.sh b/tests/user-envs-migration.sh
index 467c28fbb..187372b16 100644
--- a/tests/user-envs-migration.sh
+++ b/tests/user-envs-migration.sh
@@ -4,7 +4,7 @@
 source common.sh
 
 if isDaemonNewer "2.4pre20211005"; then
-    exit 99
+    skipTest "Daemon is too new"
 fi