Apply suggestions from code review

- `_mid` / `MID` => `_msgid` / `MSG_ID`
- Revised documentation / tooltip comments

test/helper/log_and_filtering.bash

@@ -7,8 +7,8 @@
 
 # shellcheck disable=SC2034,SC2155
 
-# Assert that the number of lines output by a previous command matches the given
+# Assert that the number of lines output by a previous command matches the given amount (${1}).
-# amount (${1}). `lines` is a special BATS variable updated via `run`.
+# `lines` is a special BATS variable updated via `run`.
 #
 # @param ${1} = number of lines that the output should have
 function _should_output_number_of_lines() {
@@ -16,8 +16,7 @@ function _should_output_number_of_lines() {
   assert_equal "${#lines[@]}" "${1:?Number of lines not provided}"
 }
 
-# Filters a service's logs (under `/var/log/supervisor/<SERVICE>.log`) given
+# Filters a service's logs (under `/var/log/supervisor/<SERVICE>.log`) given a specific string.
-# a specific string.
 #
 # @param ${1} = service name
 # @param ${2} = string to filter by
@@ -71,22 +70,23 @@ function _service_log_should_not_contain_string() {
   assert_failure
 }
 
-# Filters the mail log according to MID (Message-ID) and prints lines
+# Filters the mail log by the given MSG_ID (Message-ID) parameter,
-# of the mail log that fit Postfix's queue ID for the given message ID.
+# printing log lines which include the associated Postfix Queue ID.
 #
-# @param ${1} = message ID part before '@'
+# @param ${1} = The local-part of a Message-ID header value (`<local-part@domain-part>`)
-function _print_mail_log_of_queue_id_from_mid() {
+function _print_mail_log_of_queue_id_from_msgid() {
-  # The unique ID Postfix (and other services) use may be different in length
+  # A unique ID Postfix generates for tracking queued mail as it's processed.
-  # on different systems. Hence, we use a range to safely capture it.
+  # The length can vary (as per the postfix docs). Hence, we use a range to safely capture it.
+  # https://github.com/docker-mailserver/docker-mailserver/pull/3747#discussion_r1446679671
   local QUEUE_ID_REGEX='[A-Z0-9]{9,12}'
 
-  local MID=$(__construct_mid "${1:?Left-hand side of MID missing}")
+  local MSG_ID=$(__construct_msgid "${1:?The local-part for MSG_ID was not provided}")
   shift 1
 
   _wait_for_empty_mail_queue_in_container
 
   QUEUE_ID=$(_exec_in_container tac /var/log/mail.log                    \
-    | grep -E "postfix/cleanup.*: ${QUEUE_ID_REGEX}:.*message-id=${MID}" \
+    | grep -E "postfix/cleanup.*: ${QUEUE_ID_REGEX}:.*message-id=${MSG_ID}" \
     | grep -E --only-matching --max-count 1 "${QUEUE_ID_REGEX}" || :)
 
   # We perform plausibility checks on the IDs.
@@ -97,13 +97,12 @@ function _print_mail_log_of_queue_id_from_mid() {
   _filter_service_log 'mail' "${QUEUE_ID}"
 }
 
-# Filters the mail log according to MID (Message-ID) and prints lines
+# A convenience method that filters for Dovecot specific logs with a `msgid` field that matches the MSG_ID input.
-# of the mail log that fit lines with the pattern `msgid=${1}@dms-test`.
 #
-# @param ${1} = message ID part before '@'
+# @param ${1} = The local-part of a Message-ID header value (`<local-part@domain-part>`)
 function _print_mail_log_for_msgid() {
-  local MID=$(__construct_mid "${1:?Left-hand side of MID missing}")
+  local MSG_ID=$(__construct_msgid "${1:?The local-part for MSG_ID was not provided}")
   shift 1
 
-  _filter_service_log 'mail' "msgid=${MID}"
+  _filter_service_log 'mail' "msgid=${MSG_ID}"
 }

test/helper/sending.bash

@@ -102,19 +102,25 @@ function _send_email() {
   return "${RETURN_VALUE}"
 }
 
-# Contruct the message ID for the 'Message-ID' header.
+# Construct the value for a 'Message-ID' header.
+# For tests we use only the local-part to identify mail activity in logs. The rest of the value is fixed.
 #
-# @param ${1} = message ID part before '@' (later used when filtering logs again)
+# A Message-ID header value should be in the form of: `<local-part@domain-part>`
-function __construct_mid() {
+# https://en.wikipedia.org/wiki/Message-ID
-  echo "<${1:?Message-ID prefix is required}@dms-tests>"
+# https://datatracker.ietf.org/doc/html/rfc5322#section-3.6.4
+#
+# @param ${1} = The local-part of a Message-ID header value (`<local-part@domain-part>`)
+function __construct_msgid() {
+  local MSG_ID_LOCALPART=${1:?The local-part for MSG_ID was not provided}
+  echo "<${MSG_ID_LOCALPART}@dms-tests>"
 }
 
-# Like `_send_email` but adds a "Message-Id: ${1}@dms-tests>" header, which
+# Like `_send_email` but adds a "Message-ID: ${1}@dms-tests>" header,
-# allows for filtering logs later.
+# which allows for filtering logs later.
 #
-# @param ${1} = message ID part before '@' (later used when filtering logs again)
+# @param ${1} = The local-part of a Message-ID header value (`<local-part@domain-part>`)
-function _send_email_with_mid() {
+function _send_email_with_msgid() {
-  local MID=$(__construct_mid "${1:?Left-hand side of MID missing}")
+  local MSG_ID=$(__construct_msgid "${1:?The local-part for MSG_ID was not provided}")
   shift 1
 
   _send_email "${@}" --header "Message-Id: ${MID}"
@@ -122,9 +128,9 @@ function _send_email_with_mid() {
 
 # Send a spam e-mail by utilizing GTUBE.
 #
-# Extra arguments given to this function will be supplied by `_send_email_with_mid` directly.
+# Extra arguments given to this function will be supplied by `_send_email_with_msgid` directly.
 function _send_spam() {
-  _send_email_with_mid 'spam' "${@}" \
+  _send_email_with_msgid 'spam' "${@}" \
     --from 'spam@external.tld' \
     --body 'XJS*C4JDBQADN1.NSBN3*2IDNEN*GTUBE-STANDARD-ANTI-UBE-TEST-EMAIL*C.34X'
 }

test/tests/parallel/set1/spam_virus/postscreen.bats

@@ -57,8 +57,8 @@ function teardown_file() {
   # Configure `send_email()` to send from the mail client container (CONTAINER2_NAME) via ENV override,
   # mail is sent to the DMS server container (CONTAINER1_NAME) via `--server` parameter:
   CONTAINER_NAME=${CONTAINER2_NAME} _send_email --expect-rejection --server "${CONTAINER1_IP}" --port 25 --data 'postscreen.txt'
-  # TODO: Use _send_email_with_mid when proper resolution of domain names is possible:
+  # TODO: Use _send_email_with_msgid when proper resolution of domain names is possible:
-  # CONTAINER_NAME=${CONTAINER2_NAME} _send_email_with_mid 'postscreen' --server "${CONTAINER1_IP}" --data 'postscreen.txt'
+  # CONTAINER_NAME=${CONTAINER2_NAME} _send_email_with_msgid 'postscreen' --server "${CONTAINER1_IP}" --data 'postscreen.txt'
   # _print_mail_log_for_msgid 'postscreen'
   # assert_output --partial "stored mail into mailbox 'INBOX'"
 

test/tests/parallel/set1/spam_virus/rspamd_full.bats

@@ -45,16 +45,16 @@ function setup_file() {
 
   # We will send 4 emails:
   #   1. The first one should pass just fine
-  _send_email_with_mid 'pass'
+  _send_email_with_msgid 'pass'
   #   2. The second one should be rejected (Rspamd-specific GTUBE pattern for rejection)
   _send_spam --expect-rejection
   #   3. The third one should be rejected due to a virus (ClamAV EICAR pattern)
   # shellcheck disable=SC2016
-  _send_email_with_mid 'virus' --expect-rejection \
+  _send_email_with_msgid 'virus' --expect-rejection \
     --body 'X5O!P%@AP[4\PZX54(P^)7CC)7}$EICAR-STANDARD-ANTIVIRUS-TEST-FILE!$H+H*'
   #   4. The fourth one will receive an added header (Rspamd-specific GTUBE pattern for adding a spam header)
   #      ref: https://rspamd.com/doc/gtube_patterns.html
-  _send_email_with_mid 'header' --body "YJS*C4JDBQADN1.NSBN3*2IDNEN*GTUBE-STANDARD-ANTI-UBE-TEST-EMAIL*C.34X"
+  _send_email_with_msgid 'header' --body "YJS*C4JDBQADN1.NSBN3*2IDNEN*GTUBE-STANDARD-ANTI-UBE-TEST-EMAIL*C.34X"
 
   _run_in_container cat /var/log/mail.log
   assert_success
@@ -120,7 +120,7 @@ function teardown_file() { _default_teardown ; }
   _service_log_should_contain_string 'rspamd' 'S \(reject\)'
   _service_log_should_contain_string 'rspamd' 'reject "Gtube pattern"'
 
-  _print_mail_log_of_queue_id_from_mid 'spam'
+  _print_mail_log_of_queue_id_from_msgid 'spam'
   assert_output --partial 'milter-reject'
   assert_output --partial '5.7.1 Gtube pattern'
   refute_output --partial "stored mail into mailbox 'INBOX'"
@@ -135,7 +135,7 @@ function teardown_file() { _default_teardown ; }
   _service_log_should_contain_string 'rspamd' 'T \(reject\)'
   _service_log_should_contain_string 'rspamd' 'reject "ClamAV FOUND VIRUS "Eicar-Signature"'
 
-  _print_mail_log_of_queue_id_from_mid 'virus'
+  _print_mail_log_of_queue_id_from_msgid 'virus'
   assert_output --partial 'milter-reject'
   assert_output --partial '5.7.1 ClamAV FOUND VIRUS "Eicar-Signature"'
   refute_output --partial "stored mail into mailbox 'INBOX'"