* chore: Co-locate process checking and process restart verification
Extract the test cases for checking a process is running and properly restarts from various test files into a single one:
Core (always running):
opendkim, opendmarc, master (postfix)
ENV dependent:
amavi (amavisd-new), clamd, dovecot, fail2ban-server (fail2ban), fetchmail, postgrey, postsrsd, saslauthd
These now run off a single container with the required ENV and call a common function (the revised version in parallel test cases).
* fix(saslauthd): Quote wrap supervisor config vars
`saslauth.conf` calls `-O` option for most commands defined with an ENV that may be empty/null. This would cause the process to silently fail / die.
This doesn't happen if quote wrapping the ENV, which calls `-O` with an empty string.
Not necessary, but since one of `postgrey` ENV were quote wrapped in `supervisor-app.conf`, I've also done the same there.
* fix(postsrsd): Change supervisor `autorestart` policy to `true`
The PR that introduced the config switched from `true` to `unexpected` without any context. That prevents restart working when the process is killed. Setting to `true` instead will correctly restart the service.
* chore: Remove disabled postgrey test file
`mail_with_postgrey_disabled_by_default.bats` only checked the migrated test cases, removed as no longer serving a purpose.
* tests(refactor): Make `_should_restart_when_killed()` more reliable
The previous version did not ensure that the last checks process was actually restarted, only that it was running.
It turns out that `pkill` is only sending the signal, there can be some delay before the original process is actually killed and restarted.
This can be identified with `pgrep --older <seconds>`. First ensure the process is at a specified age, then after killing check that the process is not running that is at least that old, finally check that there is a younger process actually running.. (_could fail if a process doesn't restart, or there is a delay such as imposed by `sleep` in wrapper scripts for postfix and fail2ban_)
The helper method is not used anywhere else now, move it into this test instead. It has been refactored to accomodate the needs for `--older`, and `--list-full` provides some output that can be matched (similar for `pkill --echo`).
* test(docs): Add inline notes about processes
* chore: Compress test cases into single case with loop
Moves the list of processes into array vars to iterate through instead.
If a failure occurs, the process name is visible along with line number in `_should_restart_when_killed()` to identify what went wrong.
* chore: Handle `FETCHMAIL_PARALLEL=1` process checks as well
* tests: Add test case for disabled ENV
Additional coverage to match what other test files were doing before, ensuring that these ENV can prevent their respective service from running.
* chore: Move `clamd` enabled check to it's own test case
Not sure about this.
It reduces the time of CPU activity (sustained full load on a thread) and increase in memory usage (1GB+ loading signatures database), but as a separate test case it also adds 10 seconds without reducing the time of the test case it was extracted from.
* chore: Make `disabled` variant the 1st test case
* fix: Adjust test cases to pass when using slower wrapper scripts
* tests(refactor): `mail_fetchmail.bats` updated to new format
Additionally merges in the parallel test file.
* chore: Move `config/fetchmail.cf` into separate sub-directory
Keep out of the default base config for tests.
* chore: Change `fetchmail.cf` FQDNs to `.test` TLD
Changed the first configs remote and local user values to more clearly document what their values should represent (_and that they don't need to be a full mail address, that's just what our Dovecot is configured with for login_).
Shifted the `here` to the end of the `is` line. It's optional syntax, only intended to contrast with the remote `there` for readability.
Additionally configured imap protocol. Not tested or verified if that's correct configuration for usage with imap protocol instead. The fetchmail feature tests are currently lacking.
Added an inline doc into the fetchmail test to reference a PR about the importance of the trailing `.` in the config. Updated the partial matching to ensure it matches for that in the value as well.
* chore: Finalize `process-check-restart.bats`
Few minor adjustments. The other ENV for clamd doesn't seem to provide any benefit, trim out the noise. Added a note about why it's been split out.
Fetchmail parallel configs are matching the config file path in the process command that is returned. The `.rc` suffix is just to add further clarity to that.
* fix: Workaround `postconf` write settle logic
After updating `main.cf`, to avoid an enforced delay from reading the config by postfix tools, we can ensure the modified time is at least 2 seconds in the past as a workaround. This should be ok with our usage AFAIK.
Shaves off 2+ seconds roughly off each container startup, reduces roughly 2+ minutes off tests.
* chore: Only modify `mtime` if less than 2 seconds ago
- Slight improvement by avoiding unnecessary writes with a conditional check on the util method.
- Can more comfortably call this during `postfix reload` in the change detection cycle now.
- Identified other tests that'd benefit from this, created a helper method to call instead of copy/paste.
- The `setup email restrict` command also did a modification and reload. Added util method here too.
* tests(fix): `mail_smtponly.bats` should wait for Postfix
- `postfix reload` fails if the service is not ready yet.
- `service postfix reload` and `/etc/init.d/postfix reload` presumably wait until it is ready? (as these work regardless)
* chore: Review feedback - Move reload method into utilities
While working on tests, I noticed that some of the configs being mounted were adding a few seconds to the start-up time of each container. Notably `postfix-*` and `dovecot.conf` config files, which have been extracted out into their own tests with those files moved into a separate config folder.
`tests.bats` has been adapted to the common setup helper, and removed ENV no longer required to run those tests. Future PRs will extract out more tests.
Review may be easier via individual commit diffs and their associated commit messages describing relevant changes.
<details>
<summary>Commit message history for reference</summary>
```markdown
tests(chore): `tests.bats` - Remove redundant config
===
- ONEDIR volume support no longer relevant, this should have been dropped.
- ClamAV ENV no longer relevant as related tests have been extracted already.
- Same with the some of the SpamAssassin ENV config.
- `VIRUSMAILS_DELETE_DELAY` is tested in the file, but doesn't use this ENV at all? (runs a separate instance to test the ENV instead)
- Hostname updated in preparation for migrating to new test helpers. Relevant test lines referencing the hostname have likewise been updated.
```
```markdown
tests(chore): `tests.bats` - Convert to common setup
===
ENV remains the same, but required adding `ENABLE_AMAVIS=1` to bring that back, while the following became redundant as they're now defaulting to explicitly disabled in the helper method:
- `ENABLE_CLAMAV=0`
- `LOG_LEVEL=debug`
- `ENABLE_UPDATE_CHECK=0`
- `--hostname` + `--tty` + standard `--volume` lines
- `-e` option expanded to long-name `--env`, and all `\` dropped as no longer necessary.
`wait_for_finished_setup_in_container` is now redundant thanks to `common_container_setup`.
```
```markdown
tests(refactor): `tests.bats` - Extract out Dovecot Sieve tests
===
Sieve test files relocated into `test/config/dovecot-sieve/` for better isolation.
`dovecot.sieve` was not using the `reject` import, and we should not encourage it? (docs still do):
https://support.tigertech.net/sieve#the-sieve-reject-jmp
```
```markdown
tests: `tests.bats` - Extract out `checking smtp` tests
===
Migrated to the standard template and copied over the original test cases with `_run_in_container` adjustment only.
Identified minimum required ENV along with which mail is required for each test case.
```
```markdown
tests(refactor): `smtp-delivery.bats`
===
- Disabled `ENABLE_SRS=1`, not necessary for these tests.
- Added a SpamAssassin related test (X-SPAM headers) which requires `SA_TAG` to properly pass (or `ENABLE_SRS=1` to deliver into inbox).
- Many lines with double quotes changed to single quote wrapping, and moving out `grep` filters into `assert_output --partial` lines instead.
- Instead of `wc -l` making failures less helpful, switch to the helper method `_should_output_number_of_lines`
- x2 `assert_output` with different EOF style of usage was not actually failing on tests when it should. Changed to assert partial output of each expected line, and count the number of lines instead.
- Added additional comments related to the test cases with a `TODO` note about `SPAMASSASSIN_SPAM_TO_INBOX=1`.
- Revised test case names, including using the common prefix var.
- `tests.bats` no longer needs to send all these emails, no other test cases require them. This affects a test checking a `/mail` folder exists which has been corrected, and a quotas test case adjusted to expect an empty quota size output.
```
```markdown
tests: `tests.bats` - Extract out test cases for config overrides
===
Slight improvement by additionally matching `postconf` output to verify the setting is properly applied.
```
```markdown
tests: `tests.bats` - Extract out Amavis SpamAssassin test case
===
Removes the need for SpamAssassin ENV in `tests.bats`.
```
</details>
- `disabled_clamav_spamassassin`:
- Just shuffling the test order around, and removing the restart test at the end which doesn't make sense.
- `postscreen`:
- Now uses common helper for getting container IP
- Does not appear to need the `NET_ADMIN` capability?
- Reduced startup time for the 2nd container + additional context about it's relevance.
- Test cases are largely the same, but refactored the `nc` alternative that properly waits it's turn. This only needs to run once. Added additional commentary and made into a generic method if needed in other tests.
- `fail2ban`:
- Use the common container IP helper method.
- Postscreen isn't affecting this test, it's not required to do the much slower exchange with the mail server when sending a login failure.
- IP being passed into ENV is no longer necessary.
- `sleep 5` in the related test cases doesn't seem necessary, can better rely on polling with timeout.
- `sleep 10` for `setup.sh` also doesn't appear to be necessary.
- `postgrey`:
- Reduced POSTGREY_DELAY to 3, which shaves a fair amount of wasted time while still verifying the delay works.
- One of the checks in `main.cf` doesn't seem to need to know about the earlier spamhaus portion of the line to work, removed.
- Better test case descriptions.
- Improved log matching via standard method that better documents the expected triplet under test.
- Removed a redundant whitelist file and test that didn't seem to have any relevance. Added a TODO with additional notes about a concern with these tests.
- Reduced test time as 8 second timeouts from `-w 8` don't appear to be required, better to poll with grep instead.
- Replaced `wc -l` commands with a new method to assert expected line count, better enabling assertions on the actual output.
- `undef_spam_subject`:
- Split to two separate test cases, and initialize each container in their case instead of `setup_file()`, allowing for using the default `teardown()` method (and slight benefit if running in parallel).
- `permit_docker`:
- Not a parallel test, but I realized that the repeat helper methods don't necessarily play well with `run` as the command (can cause false positive of what was successful).
- The usual serial to parallel test conversion to utilize the `setup.bash` common setup structure, and adding a `TEST_PREFIX` var for each test case to leverage.
- Standardize on parallel test naming conventions for variables / values.
- More consistent use of `bash -c` instead of `/bin/bash -c` or `/bin/sh -c`.
- Using the `_run_in_container` helper instead of `run docker exec ${CONTAINER_NAME}`.
- Updates tests to use the `check_if_process_is_running` helper.
---
chore: Revise inline docs for the `ssl_letsencrypt` test
- Moves the override to be in closer proximity to the `initial_setup` call, and better communicates the intent to override.
- Removes top comment block that is no longer providing value or correct information to maintainers.
- Revised `acme.json` test case inline doc comments.
