Skip to content

[#1574] Resolve "make shell tests and shell scripts more robust"

Closes #1574 (closed).

This MR can be split into multiple MRs such that Danger doesn't complain about 5000+ modified files, but it can't be done easily without breaking tests, so #huge-sorry. I have tried to do as little changes as possible. I know this will be a tiring review.

What this MR does in the order of the diff:

  • tools/shellcheck-all.sh is a convenience script that takes all the scripts would normally be shellchecked in Gitlab CI from the .gitlab-ci.yml and shellchecks them locally.
  • add all shell scripts to .gitlab-ci.yml
  • .gitlab-ci.yml now uses our own Kea image which has label latest because I haven't figured out how to tag it differently, in the command I used to build the image I had to tag the image with the registry URL to make it work I think? Was built from this Dockerfile:
FROM ubuntu:latest AS gitlab-ci

# Install dependencies.
RUN apt-get update
RUN apt-get -y install shellcheck
  • make most shell tests look like Googletests, failures will always be shown unless you kill -SIGKILL or close terminal or something. Was inspired by $437, although I think it wanted something else. The GTEST_OUTPUT xml reporting can be achieved, let's just leave it for another MR.

image

  • all autotools substitutions like @prefix@ should be double quoted like so "@prefix@", not single-quoted because they themselves can contain shell variables that need to be expanded. SC2086: Double quote to prevent globbing and word splitting. Except for places where globbing is required. Word splitting should be done via quoted arrays.
  • Backquotes are replaced with $(...). One important advantage is that the latter allows for nested executions e.g. echo $(echo $(echo abc)) SC2006 Use $(...) notation instead of legacy backticked ....
  • When using test and [, == is just a convenience alias for =. Replaced the first one with the second.
  • add #!/bin/sh shebang to all scripts
  • chmod +x all database scripts, they can now be run without having to specify a shell beforehand since we decide what shell to run them in
  • set -eu in all scripts. Exit with error if commands exit with non-zero and if undefined variables are used.
  • db_* default variables moved from kea-admin.in to admin-utils.sh because they are needed in tests as well. One fun unintended successful test pass is due to mysql --host= expanded from mysql --host=${db_host} defaulting to localhost.
  • Because we have set -e, we can't have commands that return non-zero exit code. For most commands that we expect to have non-zero exit code or for which we want to check the exit code, we have some options:
    • Enclose the command inside the if if we only care about it returning zero or non-zero like so if command; then ...
    • Momentarily set +e to allow it to return non-zero exit code, check exit code, then set -e
    • Wrap the command in a function that does this automatically, this is what run_and_return_output_and_exit_code. It maintains quoting, there are no big dangers, just small ones :)
  • Remove redundant return or exit statements like the following: retcode=$?; return $retcode. The exit code of the last command is returned automatically by default.
  • In some places, functions would let the exit code of inside commands shine through to the outside of the function, in other places in case of error a magical value was used as exit-code. I thinkn we should have a consistent behavior by choosing one of the two situations. I've leaned towards the first option. See check_mysql_version
  • In some places, you will see printf '%s\n' "${OUTPUT}" where OUTPUT is set in run_and_return_output_and_exit_code. This is to allow nested calls using run_and_return_output_and_exit_code.
  • cql_upgrade_hosts_test now fails... The hashes it generates are different from the reference file it checks against. I can't understand why just by looking at the code. It passes on master, but fails on this branch. It's not trivial to check because it hashes each byte and then hashes it all into a bigger hash and there is around 5KB of information being hashes which is why it's the test that takes the longest from all the tests. What I would do is recreate this hashing algorithm in C++ and then run it against the same input and then check outputs. Let me know if you want this fixed.
  • The fill() function used in cql_upgrade_hosts_test was optimized in one line.
  • Use $((...)) instead of expr for arithmetics SC2003: expr is antiquated. Consider rewriting this using $((..)), ${} or [[ ]].
  • echo -n is undefined in POSIX, use printf without a newline
  • When capturing arguments from a shell script, see cmdargs="$@" in src/share/database/scripts/mysql/upgrade_8.1_to_8.2.sh.in, it's important to capture them into a list cmdargs=("${@}") and then to reference them via list ${cmdargs[@]} to preserve quoting. This might not be POSIX-compliant but all the alternatives are messy.
  • # We need to set global RESULT when we exit. This is checked by callers. This comment is not true, maybe it was in the past. ${RESULT} is never checked anywhere.
  • Remove evals. Some are still left, there are ways to get rid of all of them in bash, but not in sh.

I would like to improve the assert_* commands in another MR.

Edited by Andrei Pavel

Merge request reports