From b1a74bd47a577bc844eda74864335981c33e9956 Mon Sep 17 00:00:00 2001
From: Brennan Kinney <5098581+polarathene@users.noreply.github.com>
Date: Fri, 5 Nov 2021 09:35:01 +1300
Subject: [PATCH] tests(feat): Refactor `test_helper/common.bash`
 common_container methods (#2275)

These are improvements for better supporting the requirements of other tests.

- Opted for passing an array reference instead of an ENV file. This seems to be a better approach and supports more than just ENV changes.
- Likewise, shifted to a `create` + `start` approach, instead of `docker run` for added flexibility.
- Using `TEST_TMP_CONFIG` instead of `PRIVATE_CONFIG` to make the difference in usage with config volume in tests more clear.
- Changed the config volume from read-only volume mount to be read-write instead, which seems required for other tests.
- Added notes about logged failures from a read-only config volume during container startup.
- Added `TEST_CA_CERT` as a default CA cert path for the test files volume. This can be used by default by openssl methods.
---
 test/mail_spam_bounced.bats  | 25 +++++++++-----
 test/test_helper/common.bash | 66 ++++++++++++++++++++++++++++--------
 2 files changed, 68 insertions(+), 23 deletions(-)

diff --git a/test/mail_spam_bounced.bats b/test/mail_spam_bounced.bats
index 17db7dd8..d1bbaeb4 100644
--- a/test/mail_spam_bounced.bats
+++ b/test/mail_spam_bounced.bats
@@ -1,4 +1,9 @@
 load 'test_helper/common'
+# Globals referenced from `test_helper/common`:
+# TEST_NAME
+
+# Can run tests in parallel?: No
+# Shared static container name: TEST_NAME
 
 # Test case
 # ---------
@@ -6,8 +11,6 @@ load 'test_helper/common'
 # SPAMASSASSIN_SPAM_TO_INBOX=1 is covered in `mail_spam_junk_folder.bats`.
 # Original test PR: https://github.com/docker-mailserver/docker-mailserver/pull/1485
 
-# TODO: ENV setup will move to actual ENV files in future.
-
 function setup() {
   run_setup_file_if_necessary
 }
@@ -30,11 +33,12 @@ function setup_file() {
 }
 
 @test "checking amavis: spam message is bounced (rejected)" {
-  local TEST_ENV_FILE="${PRIVATE_CONFIG}/defined.env"
-  echo 'ENABLE_SPAMASSASSIN=1' > "${TEST_ENV_FILE}"
-  echo 'SPAMASSASSIN_SPAM_TO_INBOX=0' >> "${TEST_ENV_FILE}"
+  local TEST_DOCKER_ARGS=(
+    --env ENABLE_SPAMASSASSIN=1
+    --env SPAMASSASSIN_SPAM_TO_INBOX=0
+  )
 
-  common_container_setup "${TEST_ENV_FILE}"
+  common_container_setup 'TEST_DOCKER_ARGS'
 
   run _should_emit_warning
   assert_failure
@@ -44,10 +48,13 @@ function setup_file() {
 
 @test "checking amavis: spam message is bounced (rejected), undefined SPAMASSASSIN_SPAM_TO_INBOX should raise a warning" {
   # SPAMASSASSIN_SPAM_TO_INBOX=0 is the default. If no explicit ENV value is set, it should log a warning at startup.
-  local TEST_ENV_FILE="${PRIVATE_CONFIG}/undefined.env"
-  echo 'ENABLE_SPAMASSASSIN=1' > "${TEST_ENV_FILE}"
 
-  common_container_setup "${TEST_ENV_FILE}"
+  # shellcheck disable=SC2034
+  local TEST_DOCKER_ARGS=(
+    --env ENABLE_SPAMASSASSIN=1
+  )
+
+  common_container_setup 'TEST_DOCKER_ARGS'
 
   run _should_emit_warning
   assert_success
diff --git a/test/test_helper/common.bash b/test/test_helper/common.bash
index 4b0ff38d..cafa158c 100644
--- a/test/test_helper/common.bash
+++ b/test/test_helper/common.bash
@@ -208,32 +208,70 @@ function wait_for_empty_mail_queue_in_container() {
 }
 
 # Common defaults appropriate for most tests, override vars in each test when necessary.
-# TODO: Check how many tests need write access. Consider using `docker create` + `docker cp` for easier cleanup.
+# For all tests override in `setup_file()` via an `export` var.
+# For individual test override the var via `local` var instead.
+#
+# For example, if you need an immutable config volume that can't be affected by other tests
+# in the file, then use `local TEST_TMP_CONFIG="$(duplicate_config_for_container . "${UNIQUE_ID_HERE}")"`
 function init_with_defaults() {
-  # Ignore absolute dir path and file extension, only extract filename:
-  export TEST_NAME
+  export TEST_NAME TEST_TMP_CONFIG
+
+  # In `setup_file()` the default name to use for the currently tested docker container
+  # is `${TEST_NAME}` global defined here. It derives the name from the test filename:
+  # `basename` to ignore absolute dir path and file extension, only extract filename.
   TEST_NAME="$(basename "${BATS_TEST_FILENAME}" '.bats')"
+  # In `setup_file()` creates a single copy of the test config folder to use for an entire test file:
+  TEST_TMP_CONFIG="$(duplicate_config_for_container . "${TEST_NAME}")"
 
-  export PRIVATE_CONFIG
-  PRIVATE_CONFIG="$(duplicate_config_for_container . "${TEST_NAME}")"
-  export TEST_FILES_VOLUME="${PWD}/test/test-files:/tmp/docker-mailserver-test:ro"
-  export TEST_CONFIG_VOLUME="${PRIVATE_CONFIG}:/tmp/docker-mailserver:ro"
+  # Common complimentary test files, read-only safe to share across containers:
+  export TEST_FILES_CONTAINER_PATH='/tmp/docker-mailserver-test'
+  export TEST_FILES_VOLUME="${PWD}/test/test-files:${TEST_FILES_CONTAINER_PATH}:ro"
 
+  # The config volume cannot be read-only as some data needs to be written at container startup
+  # - two sed failures (unknown lines)
+  # - dovecot-quotas.cf (setup-stack.sh:_setup_dovecot_quotas)
+  # - postfix-aliases.cf (setup-stack.sh:_setup_postfix_aliases)
+  # TODO: Check how many tests need write access. Consider using `docker create` + `docker cp` for easier cleanup.
+  export TEST_CONFIG_VOLUME="${TEST_TMP_CONFIG}:/tmp/docker-mailserver"
+
+  # The common default FQDN assigned to the container `--hostname` option:
   export TEST_FQDN='mail.my-domain.com'
+
+  # Default Root CA cert used in TLS tests with `openssl` commands:
+  export TEST_CA_CERT="${TEST_FILES_CONTAINER_PATH}/ssl/example.test/with_ca/ecdsa/ca-cert.ecdsa.pem"
 }
 
-# Common docker run command that should satisfy most tests which only modify ENV.
+# Using `create` and `start` instead of only `run` allows to modify
+# the container prior to starting it. Otherwise use this combined method.
+# NOTE: Forwards all args to the create method at present.
 function common_container_setup() {
-  local TEST_ENV_FILE=$1
+  common_container_create "$@"
+  common_container_start
+}
 
-  run docker run -d --name "${TEST_NAME}" \
+# Common docker setup is centralized here.
+#
+# `X_EXTRA_ARGS` - Optional: Pass an array by it's variable name as a string, it will
+# be used as a reference for appending extra config into the `docker create` below:
+#
+# NOTE: Using array reference for a single input parameter, as this method is still
+# under development while adapting tests to it and requirements it must serve (eg: support base config matrix in CI)
+function common_container_create() {
+  local -n X_EXTRA_ARGS=${1}
+
+  run docker create --name "${TEST_NAME}" \
+    --hostname "${TEST_FQDN}" \
+    --tty \
     --volume "${TEST_FILES_VOLUME}" \
     --volume "${TEST_CONFIG_VOLUME}" \
-    --hostname "${TEST_FQDN}" \
-    --env-file "${TEST_ENV_FILE}" \
-    --tty \
+    "${X_EXTRA_ARGS[@]}" \
     "${NAME}"
   assert_success
+}
+
+function common_container_start() {
+  run docker start "${TEST_NAME}"
+  assert_success
 
   wait_for_finished_setup_in_container "${TEST_NAME}"
-}
\ No newline at end of file
+}