Description

All common test cases that checked if a process was running or that it restarts have been migrated into a new test file that co-locates them. Seems more appropriate to centralize that into one place. Brings serial test time down by about 2 minutes (540 sec run-time was my best result).

Commits have been staged out with additional context in their commit messages to explain certain changes/choices made.

Additionally:

• The fetchmail test files were merged into one and updated to the new format. A postgrey test that only checked the service was disabled due to ENV also removed as redundant.
• postsrsd in supervisord config had it's autorestart policy as unexpected instead of true like other services. I looked up the relevant PR and there is a commit that changes to unexpected but it provides no context as to why, nor any discussion in the PR thread. Tests aren't showing any issues in switching to true but would fail to restart postsrsd with unexpected.
• SASLAuthd config for supervisord has most commands use -O with an ENV, which if unset fails to run the process as the command itself is invalid. Quote wrapping the ENV in the config however treats the arg with an empty string value and runs without configuring the ENV explicitly. I decided to quote wrapped each ENV referenced in supervisord config.

Details

The original test cases were not great and the revised versions were a bit too eager to be reliable. A common test function is called by iterating an array of processes to test against.

• It'll wait until the process has been running for a little bit of time.
• Then kill the process and wait until that process is no longer running (by polling the process age until no result), as some processes take a couple seconds to be killed.
• Finally, the new process should be running, but we poll again as this isn't always the case. Postfix and Fail2Ban are managed via wrapper scripts which have sleep 5 that delay the restart. I've documented local observations of the timings in the test itself.

Earlier commits did have separate test cases calling the same function instead of running through a loop. The loop seems fine and correctly provides errors mentioning the involved process in the event of a failure. A similar loop is used for the disabled ENV test case.

Optional Optimization (Remove up to 30 seconds test time)

I was able to bring the enabled test case time down to approx 10 seconds (it can be 20-30 currently, with another 10 if keeping the separate ClamAV test case). It would be a bit more verbose and add extra complexity for maintenance though, so I opted against it.

Realistically it seems to run in 10 seconds vs 20 seconds of the current approach (with clamd not in a separate test case, and wrapper scripts removed). If useful here is that earlier attempt:

@test "enabled - should restart processes when killed" {
  local MIN_PROCESS_AGE=4

  # Average time: 4 seconds (7 seconds with chroot + wrapper)
  run repeat_until_success_or_timeout 10 _should_have_all_processes_running "${MIN_PROCESS_AGE}"
  assert_success

  # Average time: 2 seconds
  _should_kill_all_processes_successfully

  # Average time: 3 seconds
  local PROCESS_LIST=("${ENABLED_PROCESS_LIST[@]}")
  run repeat_until_success_or_timeout 10 _should_have_restarted_all_processes "${MIN_PROCESS_AGE}"
  assert_success

  # Average time: 1 second (4-6 seconds with chroot + wrapper)
  run repeat_until_success_or_timeout 10 _should_have_all_processes_running
  assert_success
}

function _is_process_running() {
  local PROCESS=${1}
  local MIN_SECS_RUNNING
  [[ -n ${2} ]] && MIN_SECS_RUNNING="--older ${2}"

  docker exec "${CONTAINER_NAME}" pgrep --list-full ${MIN_SECS_RUNNING} "${PROCESS}"
}

function _should_have_all_processes_running() {
  for PROCESS in "${ENABLED_PROCESS_LIST[@]}"
  do
    if [[ ! $(_is_process_running "${PROCESS}" "${1}") =~ "${PROCESS}" ]]
    then
      echo "(fail) '${PROCESS}' is not running"
      return 1
    fi
  done
}

# Kill each process to trigger a restart:
function _should_kill_all_processes_successfully() {
  for PROCESS in "${ENABLED_PROCESS_LIST[@]}"
  do
    _run_in_container pkill --echo "${PROCESS}"
    assert_output --partial "${PROCESS}"
  done
}

# NOTE: Processes that have been restarted are removed from additional iterations,
# so that they don't fail the check while waiting on other processes to restart.
# NOTE: `--older` being too low (eg: 2) will match restarted process age, causing failure.
function _should_have_restarted_all_processes() {
  local STILL_RUNNING=()

  for i in "${!PROCESS_LIST[@]}"
  do
    local PROCESS="${PROCESS_LIST[$i]}"

    if [[ "$(_is_process_running "${PROCESS}" "${1}")" =~ "${PROCESS}" ]]
    then
      # Output helpful for debugging failures:
      echo "(fail) '${PROCESS}' is still running"
      STILL_RUNNING+="${PROCESS}"
    else
      # Output helpful for debugging failures:
      echo "(pass) Excluding '${PROCESS}' from next iteration"
      unset PROCESS_LIST[$i]
    fi
  done

  # Fail if any older processes were still found running:
  [[ ! ${#STILL_RUNNING[@]} -gt 0 ]]
}

Maintenance notes

Some frustrations in helpers to be aware of if someone maintains the test in future:

• run_until_success_or_timeout ... - Cannot assert output if encountering a timeout failure. Only the timedout command message is emitted.
• run repeat_until_success_or_timeout ... - Captures the output of each polled function call. Fine for success status, but if you try to refute output against the success, the previous failures get considered as well.
• repeat_until_success_or_timeout run ... - The first call is always successful even if the function called failed, since run doesn't bubble that error up. Causes false positives. Cannot pair with _run_in_container() as a result, caution needs to be taken there (or anything else with an implicit run call inside the helper).

Type of change

• Improvement (non-breaking change that does improve existing functionality)

Checklist:

• My code follows the style guidelines of this project
• I have performed a self-review of my own code
• I have commented my code, particularly in hard-to-understand areas
• If necessary I have added tests that prove my fix is effective or that my feature works
• New and existing unit tests pass locally with my changes