Skip to content

tests: improve _send_email#3105

Merged
georglauterbach merged 9 commits intomasterfrom
improve/send-email
Feb 24, 2023
Merged

tests: improve _send_email#3105
georglauterbach merged 9 commits intomasterfrom
improve/send-email

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Feb 22, 2023

Description

The function now handles IP, Port and more path names, as well as extra options you can supply to nc.

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

@georglauterbach
make _send_email more generic (& more powerful)
a47aeb2 
The function now handles IP, Port and more path names, as well as
extra options you can supply to `nc`.
@georglauterbach
adjust more code to use _send_email
bf72fcd
@georglauterbach georglauterbach added area/scripts area/tests kind/improvement Improve an existing feature, configuration file or the documentation labels Feb 22, 2023
@georglauterbach georglauterbach added this to the v12.0.0 milestone Feb 22, 2023
@georglauterbach georglauterbach self-assigned this Feb 22, 2023
polarathene
polarathene requested changes Feb 22, 2023
View reviewed changes
Comment thread test/tests/parallel/set1/spam_virus/fail2ban.bats Outdated
Comment thread test/tests/parallel/set3/mta/smtp_delivery.bats Outdated
Comment thread test/helper/sending.bash Outdated
@georglauterbach
revise initial refactoring
1397786 
Now, the method (invocation) really has become easier.
@georglauterbach georglauterbach added the pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged label Feb 22, 2023
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Feb 22, 2023 •
edited
Loading

This PR is waiting for #3006 :) (to avoid forcing the OP to resolve merge conflicts again).

polarathene
polarathene requested changes Feb 22, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the hassle, but I think _send_email() needs to swap param 2 and 3 around so the empty '' values can be dropped? 🤔

Comment thread test/tests/parallel/set3/mta/smtp_delivery.bats Outdated
@georglauterbach georglauterbach mentioned this pull request Feb 22, 2023
Merged
6 tasks
@georglauterbach
adjust _send_mail for the last time :D
6a3da54 
CONTAINER_NAME is now assumed to be set, which makes invocation even
easier for common cases. Moreover, the helper that additionally
retrieves the mail ID was renamed (_send_mail_* -> _send_email_*) to be
canonical with _send_mail; it also takes the same arguments now, and has
the same assumptions when calling it.
@georglauterbach
corrected Amavis test teardown file
83dbfbc
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Feb 22, 2023

CI really is not feeling well today. I'm seeing the same weird test failures as in #3006, even though both PRs do not touch Amavis at all. @polarathene you mentioned a timing issue, because our tests are now running with less delay. Should we introduce additional short sleep periods?

@polarathene
Copy link
Copy Markdown
Member

polarathene commented Feb 23, 2023

Should we introduce additional short sleep periods?

Yeah, seems that may be necessary for this test. I think sleep 5 should be more than enough here?

Justification

docker-mailserver/test/tests/parallel/set3/mta/smtp_delivery.bats

Lines 50 to 59 in 9724060

_run_in_container setup email add '[email protected]' 'mypassword'
assert_success
_wait_until_change_detection_event_completes
# Even if the Amavis port is reachable at this point, it may still refuse connections?
_wait_for_tcp_port_in_container 10024
_wait_for_smtp_port_in_container_to_respond
# Amavis may still not be ready to receive mail, sleep a little to avoid connection failures:
sleep 1

It's likely that the change detection completes, and reloads Amavis:

docker-mailserver/target/scripts/check-for-changes.sh

Lines 86 to 94 in 9724060

function _reload_amavis
{
if [[ ${CHANGED} =~ ${DMS_DIR}/postfix-accounts.cf ]] || [[ ${CHANGED} =~ ${DMS_DIR}/postfix-virtual.cf ]]
then
# /etc/postfix/vhost was updated, amavis must refresh it's config by
# reading this file again in case of new domains, otherwise they will be ignored.
amavisd-new reload
fi
}

And that possibly isn't a quick process for Amavis to reload / restart, but ports may already be available 🤷‍♂️ bumping up the sleep should help. Optimizations that have been made was to load Postfix without a wrapper (and switch to not using chroot), which would have been 3-5 seconds faster IIRC, and the change detection no longer has a 10 second start-up delay (this PR adjusted the failing test in an attempt to compensate), both would have given Amavis plenty of time originally.

Common failure for future reference as action logs will eventually expire 
not ok 30 [SMTP] (delivery) should succeed at emptying mail queue in 374ms
# (from function `refute_output' in file test/test_helper/bats-assert/src/refute_output.bash, line 189,
#  in test file test/tests/parallel/set3/mta/smtp_delivery.bats, line 99)
#   `refute_output --partial 'Connection refused'' failed
#
# -- output should not contain substring --
# substring (1 lines):
#   Connection refused
# output (50 lines):
#   -Queue ID-  --Size-- ----Arrival Time---- -Sender/Recipient-------
#   9E57A23993E*     493 Wed Feb 22 14:39:50  [email protected]
#                                            [email protected]
#
#   AAA7C23997F*     482 Wed Feb 22 14:39:51  [email protected]
#                                            [email protected]
#
#   2B81C2399[37](https://github.com/docker-mailserver/docker-mailserver/actions/runs/4243784120/jobs/7377011909#step:6:38)*     572 Wed Feb 22 14:39:50  [email protected]
#                                            [email protected]
#
#   1DEC62[39](https://github.com/docker-mailserver/docker-mailserver/actions/runs/4243784120/jobs/7377011909#step:6:40)97A*     505 Wed Feb 22 14:39:51  [email protected]
#                                            [email protected]
#
#   65D6123997E*     580 Wed Feb 22 14:39:51  [email protected]
#                                            [email protected]
#
#   E2A6F239986*     490 Wed Feb 22 14:39:51  [email protected]
#                                            [email protected]
#
#   D1C87239960*     505 Wed Feb 22 14:39:50  [email protected]
#                                            [email protected]
#
#   6855C23993A*     505 Wed Feb 22 14:39:50  [email protected]
#                                            [email protected]
#
#   E3AB2239932     514 Wed Feb 22 14:39:49  [email protected]
#                      (connect to 127.0.0.1[127.0.0.1]:10024: Connection refused)
#                                            [email protected]
#
#   2B653239924     522 Wed Feb 22 14:39:49  [email protected]
#                      (connect to 127.0.0.1[127.0.0.1]:10024: Connection refused)
#                                            [email protected]
#
#   A125B2398AF     553 Wed Feb 22 14:39:48  [email protected]
#                      (connect to 127.0.0.1[127.0.0.1]:10024: Connection refused)
#                                            [email protected]
#
#   DDC6[42](https://github.com/docker-mailserver/docker-mailserver/actions/runs/4243784120/jobs/7377011909#step:6:43)398EA     510 Wed Feb 22 14:39:48  [email protected]
#                      (connect to 127.0.0.1[127.0.0.1]:10024: Connection refused)
#                                            [email protected]
#
#   67C212398A5     516 Wed Feb 22 14:39:[48](https://github.com/docker-mailserver/docker-mailserver/actions/runs/4243784120/jobs/7377011909#step:6:49)  [email protected]
#                      (connect to 127.0.0.1[127.0.0.1]:10024: Connection refused)
#                                            [email protected]
#
#   9ADFC23992E     537 Wed Feb 22 14:39:[49](https://github.com/docker-mailserver/docker-mailserver/actions/runs/4243784120/jobs/7377011909#step:6:50)  [email protected]
#                      (connect to 127.0.0.1[127.0.0.1]:10024: Connection refused)
#                                            [email protected]
#
#   -- 7 Kbytes in 14 Requests.
# --

polarathene
polarathene previously approved these changes Feb 23, 2023
View reviewed changes
polarathene
polarathene requested changes Feb 23, 2023
View reviewed changes
Comment thread test/tests/serial/tests.bats Outdated
@georglauterbach georglauterbach merged commit ae05e6a into master Feb 24, 2023
@georglauterbach georglauterbach deleted the improve/send-email branch February 24, 2023 09:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@polarathene polarathene polarathene approved these changes

@casperklein casperklein Awaiting requested review from casperklein

Assignees

@georglauterbach georglauterbach

Labels

area/scripts area/tests kind/improvement Improve an existing feature, configuration file or the documentation pr/waiting for other PR to get merged This PR is waiting for another / other PR(s) to get merged

Projects

None yet

Milestone

v12.0.0

Development

Successfully merging this pull request may close these issues.

2 participants

@georglauterbach @polarathene