tests: refactor 4 more tests by georglauterbach · Pull Request #3018 · docker-mailserver/docker-mailserver

georglauterbach · 2023-01-22T00:52:37Z

Description

More rewrites. Note: mail_smtponly.bats seems to be really broken. I added TODOs.

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
I have made corresponding changes to the documentation (README.md or the documentation under docs/)
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

\`\` are interpreted like `$()`, and ShellCheck complains in my IDE. Single quotation marks do the job as well, and my IDE does not complain anymore.

polarathene

Nice work! 😀

I almost started working on mail_smtponly.bats myself, so glad I checked GitHub first 😅

polarathene · 2023-01-22T08:30:46Z

@@ -1,4 +1,4 @@
-HELO mail.localhost
+HELO mail.example.test


Just for this file since I think it's the only test that uses it, feel free to change the domains to use the .test TLD if it helps.

This is something I plan to do after all tests are migrated to the new format / helpers. I've noticed the DNS lookups (and attempts to send bounce mail in the logs) as well.

👍🏼

EDIT: I will leave this open in case we stumble on it again in the future.

polarathene · 2023-01-22T08:36:49Z

+@test "(Dovecot) quota plugin is disabled" {
+  _run_in_container cat /etc/dovecot/conf.d/10-mail.conf
+  assert_success
+  refute_output --partial '$mail_plugins quota'

-  run docker exec mail_no_quotas /bin/sh -c "grep '\$mail_plugins imap_quota' /etc/dovecot/conf.d/20-imap.conf"
-  assert_failure
+  _run_in_container cat /etc/dovecot/conf.d/20-imap.conf
+  assert_success
+  refute_output --partial '$mail_plugins imap_quota'


Would it be better to lookup the relevant line(s) with grep and then refute that output for quota / imap_quota?

We recently had a fix submitted where config for quota support was wrong, but these tests wouldn't have failed as both before and after diffs are matching $mail_plugins when the config was $mail_plugin which would have always failed to match causing a false positive.

I will have a look :)

Took some time and thinking but I think I came up with something quite nice: I made the test more agressive asserting partial output only for "quota" / "imap_quota", but I stripped the comments and blank lines so they don't interfere. I then thought this functionality (running a command and filtering the output) is actually one that we need all the time, I added a helper function.

Corresponding commit: ac2be17

polarathene · 2023-01-22T09:08:47Z

+  # the value `no` is not even valid for `smtp_host_lookup`
+  # `smtp_host_lookup` seems to be deprecated too
+  _run_in_container postconf smtp_host_lookup=no


smtp_host_lookup is not deprecated, but disable_dns_lookups is in favor of smtp_dns_support_level.

smtp_host_lookup is to configure how that lookup should be performed. If we want to opt-out we can disable with smtp_dns_support_level=disabled. That disables MX record lookups, but still defers regular hostname lookups to "native" as the Postfix docs describe it.

Chances are this isn't relevant at all to the test. I'll look through the history to see if there's any context, but it's doubtful.

References for revisiting this test in future

Based on the test name. The general approach in tests is to just send mail to the same DMS instance (or sometimes a 2nd instance), wait for the mail queue to empty and then check the mail directory it should have delivered to for a local account.

This test is only interested in if a mail was sent. smtp-delivery.bats has a bunch of test cases doing that, they look like this:

_run_in_container grep 'to=<[email protected]>, orig_to=<[email protected]>' /var/log/mail/mail.log assert_success assert_output --partial 'status=sent' _should_output_number_of_lines 1

And here's another from mail_lmtp_ip.bats:

@test "delivers mail to existing account" { _wait_for_smtp_port_in_container # Send a test mail: _run_in_container_bash "nc 0.0.0.0 25 < /tmp/docker-mailserver-test/email-templates/existing-user1.txt" assert_success # Verify delivery was successful, log line should look similar to: # postfix/lmtp[1274]: 0EA424ABE7D9: to=<[email protected]>, relay=127.0.0.1[127.0.0.1]:24, delay=0.13, delays=0.07/0.01/0.01/0.05, dsn=2.0.0, status=sent (250 2.0.0 <[email protected]> ixPpB+Zvv2P7BAAAUi6ngw Saved) local MATCH_LOG_LINE='postfix/lmtp.* status=sent .* Saved)' run timeout 60 docker exec "${CONTAINER_NAME}" bash -c "tail -F /var/log/mail/mail.log | grep --max-count 1 '${MATCH_LOG_LINE}'" assert_success }

Since Dovecot is not available and no other mail storage is intended in this mode, a 2nd container to receive the mail probably makes more sense 😅

The fail2ban.bats test seems to handle this for SMTP auth by getting the container IP to use with nc, which would probably work?:

CONTAINER1_IP=$(_get_container_ip "${CONTAINER1_NAME}") _run_in_container_explicit "${CONTAINER2_NAME}" bash -c "nc ${CONTAINER1_IP} 25 < /tmp/docker-mailserver-test/auth/smtp-auth-login-wrong.txt"

I'm fine with the skip you've added for now, and addressing this afterwards with a follow-up PR.

For now these lines at least don't seem to provide any value as you've documented (thanks for pointing that out btw 😁 )

Suggested change

# the value `no` is not even valid for `smtp_host_lookup`

# `smtp_host_lookup` seems to be deprecated too

_run_in_container postconf smtp_host_lookup=no

Keeping this open for reference

polarathene · 2023-01-22T09:18:25Z

+  # it looks as if someone tries to send mail to another domain outside of DMS
+  _run_in_container_bash "nc 0.0.0.0 25 < /tmp/docker-mailserver-test/email-templates/smtp-only.txt"


So test this isn't meant to deliver mail to the DMS container, but relay it externally?

I'm not too familiar with the feature, if accounts are tied to Dovecot for authentication, then it's also trying to submit mail without auth (which I think it is permitted to bypass by being sent internally like this).

Not much we can do about fixing that any time soon then 😅

I don't really know why this is the case, but the RCPT is for @external.tld .. I actually don't know if this makes sense either. Maybe it is useless and we can just deliver mail with Postfix to a local account and test the functionality this way too?

Can DMS receive mail and store it without Dovecot when using SMTP_ONLY=1?

Probably not, but I have not tried it TBH :D I'g go about this way: when all tests are refactored so we can run them in parallel, I'd then (after or before the DNS updates I don't know) tackle the left-over TODOs that we added when refactoring tests. I mean, this test was not properly working all the time, and we have enough to do when it comes to reworking the tests for now :)

Co-authored-by: Brennan Kinney <[email protected]>

see https://github.com/docker-mailserver/docker-mailserver/pull/3018/files#r1083431902

georglauterbach added 3 commits January 22, 2023 01:50

convert 3 more tests

2eb2e9b

convert smtp_only (which is totally broken IMO)

bc7f6a0

fix shellcheck lint

aacaf2f

\`\` are interpreted like `$()`, and ShellCheck complains in my IDE. Single quotation marks do the job as well, and my IDE does not complain anymore.

georglauterbach added area/ci kind/improvement Improve an existing feature, configuration file or the documentation labels Jan 22, 2023

georglauterbach added this to the v12.0.0 milestone Jan 22, 2023

georglauterbach requested review from casperklein and polarathene January 22, 2023 00:52

georglauterbach self-assigned this Jan 22, 2023

georglauterbach changed the title ~~Tests/more rewrites~~ tests: refactor 4 more tests Jan 22, 2023

polarathene requested changes Jan 22, 2023

View reviewed changes

georglauterbach and others added 3 commits January 22, 2023 11:16

apply suggestions from code review (no. 1)

fc9654a

Co-authored-by: Brennan Kinney <[email protected]>

apply more PR feedback & add helper function

ac2be17

see https://github.com/docker-mailserver/docker-mailserver/pull/3018/files#r1083431902

Merge remote-tracking branch 'origin/master' into tests/more-rewrites

100cfd8

georglauterbach force-pushed the tests/more-rewrites branch from a531c62 to 100cfd8 Compare January 22, 2023 11:11

georglauterbach requested a review from polarathene January 22, 2023 11:11

polarathene approved these changes Jan 22, 2023

View reviewed changes

casperklein approved these changes Jan 23, 2023

View reviewed changes

Merge branch 'master' into tests/more-rewrites

7338810

georglauterbach merged commit 0fd7c36 into master Jan 24, 2023

georglauterbach deleted the tests/more-rewrites branch January 24, 2023 08:21

polarathene mentioned this pull request Sep 14, 2025

fix: Postfix: disable_dns_lookups => smtp_dns_support_level #4568

Merged

8 tasks

	# the value `no` is not even valid for `smtp_host_lookup`
	# `smtp_host_lookup` seems to be deprecated too
	_run_in_container postconf smtp_host_lookup=no

		# it looks as if someone tries to send mail to another domain outside of DMS
		_run_in_container_bash "nc 0.0.0.0 25 < /tmp/docker-mailserver-test/email-templates/smtp-only.txt"

Uh oh!

Conversation

georglauterbach commented Jan 22, 2023

Description

Type of change

Checklist:

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georglauterbach Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georglauterbach Jan 22, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

georglauterbach Jan 22, 2023 •

edited

Loading

georglauterbach Jan 22, 2023 •

edited

Loading