Improve shell scripts #193
Conversation
This might be a macOS/linux difference, but this fixes a loud error.
Enable pipefail in ci as well.
| @@ -1,20 +1,20 @@ | |||
| #!/bin/bash | |||
|
|
|||
| set -e | |||
| set -euo pipefail | |||
There was a problem hiding this comment.
-o pipefail fails commands if an intermediate step fails.
-u is equivalent to -o nounset, which fails a script if it references unbound variables.
|
|
||
| CWD=$(pwd) | ||
| CMD="elm-review --no-color" | ||
| TMP="$CWD/temporary" | ||
| ELM_HOME="$TMP/elm-home" | ||
| SNAPSHOTS="$CWD/run-snapshots" | ||
| SUBCOMMAND="$1" | ||
| SUBCOMMAND="${1:-}" |
There was a problem hiding this comment.
This makes the subcommand default to none if there is none passed, for nounset.
| @@ -34,10 +34,12 @@ function runCommandAndCompareToSnapshot { | |||
| exit 1 | |||
| fi | |||
|
|
|||
| eval "$LOCAL_COMMAND$AUTH --FOR-TESTS $ARGS" 2>&1 \ | |||
| (eval "$LOCAL_COMMAND$AUTH --FOR-TESTS $ARGS" || true) 2>&1 \ | |||
There was a problem hiding this comment.
All of these || trues tell bash that the command might fail. They should be here either way, it's a correctness issue, but pipefail enforces it.
The subshell makes it so that it's not the true that gets redirected to /dev/null/, but the whole thing.
| local diffed | ||
| diffed="$(diff "$TMP/$FILE" "$SNAPSHOTS/$FILE" || true)" | ||
| if [ "$diffed" != "" ] |
There was a problem hiding this comment.
This is something OSH's strict_errexit caught. Before, if diff failed, it would be ignored. That's not good, so it has to be extracted into a variable.
However, we do want to do our own handling of it, so we then explicitly ignore the potential failures.
| @@ -46,7 +48,6 @@ function runCommandAndCompareToSnapshot { | |||
| cat "$TMP/$FILE" | |||
| echo -e "\n \x1B[31mHere is the difference:\x1B[0m\n" | |||
| diff -p "$TMP/$FILE" "$SNAPSHOTS/$FILE" | |||
| exit 1 | |||
There was a problem hiding this comment.
diff exits with exit code one, so this was dead code.
| @@ -153,7 +156,7 @@ rm -rf "$TMP" \ | |||
|
|
|||
| mkdir -p "$TMP" | |||
|
|
|||
| if [ "$1" == "record" ] | |||
| if [ "$SUBCOMMAND" == "record" ] | |||
There was a problem hiding this comment.
Instead of using the potentially unbound $1, use the bound-to-be-bound $SUBCOMMAND, which also reads easier.
| @@ -254,7 +260,7 @@ $createTest "$CMD" \ | |||
| "Fixing all errors for an entire rule should remove the suppression file" \ | |||
| "" \ | |||
| "suppressed-errors-after-fixed-errors-for-rule.txt" | |||
| if [ -f review/suppressed/NoUnused.Dependencies.json ]; then | |||
| if [ -f "./review/suppressed/NoUnused.Dependencies.json" ]; then | |||
There was a problem hiding this comment.
Always put paths in strings. Doesn't make a difference here, but doesn't hurt.
| defaults: | ||
| run: | ||
| shell: bash |
There was a problem hiding this comment.
Run commands with pipefail.
See lowlighter/libs#69 (comment) for a longer explaination.
| fi | ||
| if [ "$(diff -yq "$TMP/project to fix/src/Folder/Unused.elm" "$SNAPSHOTS/project to fix/src/Folder/Unused.elm")" != "" ] | ||
| declare diffed | ||
| diffed="$(diff -q "$TMP/project to fix/src/Folder/Unused.elm" "$SNAPSHOTS/project to fix/src/Folder/Unused.elm" || true)" |
There was a problem hiding this comment.
-y and -q are incompatible, at least on macOS.
|
Thank you for the improvements! |
I'm going to run our testing scripts through OSH, see what antipatterns it spots, and fix the warnings.
OSH is a fast, simple, (almost) 100% bash-compatible and POSIX-compliant shell from the Oils project. It supports some extra
shopts, which catch things bash and shellcheck don't/can't (respectively). It also supports someshopts that make breaking changes (ysh), but I don't plan on making our tests non-compatible with bash (as that would break CI, for one reason).