tests: new sending and filtering functions by georglauterbach · Pull Request #3786 · docker-mailserver/docker-mailserver · GitHub

Description

This PR is intended to solve concerns raised in #3747 (comment) by @polarathene.

A new file for log filter helpers was created. Thereafter, _send_email_and_get_id was reworked to become _send_email_with_mid. I paid attention to using sub-shells only where necessary. This new way of sending + filtering allows for greater flexibility and the implementation has become easier. Last but not least, I updated the service log filters and applied them throughout all our tests to improve code quality.

Review commit by commit.

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
I have added information about changes made in this PR to CHANGELOG.md

The new function, called `_send_email_with_mid`, aligns with suggestions from @polarethene and is heavily simplified compared to its predecessor `_send_email_and_get_id`. New helpers will be introduced to filter logs according to the MID constructed in this function.

I'll review this after v13.3 is out, trying to wrap up some tasks in time for that 😅

I'll review this after v13.3 is out, trying to wrap up some tasks in time for that 😅

Nice! This is basically just me implementing what you proposed in #3747 (comment).

I'll apply these.

Bulk of the changes are:

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

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

I'd personally prefer being more explicit about those that are using regex, potentially with a _regex variant of such methods 🤷‍♂️

Probably unlikely to be an issue but technically all those logs being checked for addresses will treat the . as any character rather than an actual .? Fixed string matching by default seems better suited?

Feel free to delay that to a separate PR.

Great work! 🥳

Moreover, I added a function to print the whole mail log. Appropriate comments were added to this function to indicate that one should only use this function when necessary.

I'll apply some of these, and leave some others unresolved for better visibility to you 👍

Just the prior review feedback and some additional feedback here to address and this looks good to go! 😁 🚀

Similar to the dms-test-email-spam, it might be nice to have the equivalent for virus. Not required to be addressed in this PR though since there's plenty of noise here already 😅

Huzzah! A failure like I expected 😛

not ok 86 [Rspamd] (full) RSPAMD_LEARN works in 3193ms
# (from function `assert_success' in file test/test_helper/bats-assert/src/assert_success.bash, line 42,
#  from function `_service_log_should_contain_string' in file test/helper/log_and_filtering.bash, line 50,
#  in test file test/tests/parallel/set1/spam_virus/rspamd_full.bats, line 290)
#   `_service_log_should_contain_string 'mail' 'imapsieve: Matched static mailbox rule [2]'' failed
#
# -- command failed --
# status : 1
# output :
# --
#

Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>

LGTM 👍

May want to merge this after the base image upgrade PR?

Yes, this is fine with me :)

I resolved merge conflicts and enabled auto-merge. When you approve @polarathene, this will automatically be merged into master.

🚀

georglauterbach added 6 commits January 16, 2024 11:58

move log/filter functions into own file

87b92f6

add ShellCheck global directives

3a64a7a

new filters for searching logs with MID

74d63af

use new filters (and sending) functions

745e59d

add new helper for asserting non-existence of log message

51a3b3f

georglauterbach requested a review from polarathene January 16, 2024 19:52

georglauterbach self-assigned this Jan 16, 2024

georglauterbach added area/tests kind/improvement Improve an existing feature, configuration file or the documentation labels Jan 16, 2024

georglauterbach added this to the v14.0.0 milestone Jan 16, 2024

georglauterbach commented Jan 16, 2024

View reviewed changes

Comment thread test/helper/common.bash

use new filters in tests

6503f24

georglauterbach force-pushed the tests/new_filters branch from 065298e to 6503f24 Compare January 16, 2024 20:02

georglauterbach mentioned this pull request Jan 19, 2024

tests: Revise OAuth2 tests #3795

Merged

7 tasks

polarathene requested changes Jan 20, 2024

View reviewed changes

Apply suggestions from code review

7c39c15

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

polarathene requested changes Jan 20, 2024

View reviewed changes

polarathene reviewed Jan 20, 2024

View reviewed changes

Comment thread test/helper/sending.bash Outdated

Apply suggestions from code review

7f82583

georglauterbach commented Jan 20, 2024

View reviewed changes

Comment thread test/tests/parallel/set3/mta/smtp_delivery.bats

georglauterbach added 7 commits January 20, 2024 12:01

fix tests

5622761

use more distinct names for MSG_ID headers

143ca0c

update _filter_service_log to not use -i -E

aaeea8a

Moreover, I added a function to print the whole mail log. Appropriate comments were added to this function to indicate that one should only use this function when necessary.

adjust helpers to new helper filter

b3a6a7b

follow-up of previous commit

ca0127e

Merge branch 'master' into tests/new_filters

9ae8336

add CHANGELOG entry

709b56b

georglauterbach requested a review from polarathene January 20, 2024 11:45

polarathene requested changes Jan 21, 2024

View reviewed changes

Apply suggestions from code review

1b98de4

polarathene requested changes Jan 21, 2024

View reviewed changes

polarathene and others added 5 commits January 21, 2024 13:51

chore: Update OAuth2 to use new log helper

5da61f9

Apply suggestions from code review

f954982

Co-authored-by: Brennan Kinney <5098581+polarathene@users.noreply.github.com>

Merge branch 'master' into tests/new_filters

be828dd

Merge branch 'master' into tests/new_filters

b64eb75

added explicit _regexp filters for logs

fa1f3b0

polarathene requested changes Jan 24, 2024

View reviewed changes

Comment thread test/helper/log_and_filtering.bash Outdated

Comment thread test/helper/log_and_filtering.bash Outdated

Comment thread test/helper/log_and_filtering.bash Outdated

Comment thread CHANGELOG.md

Apply suggestions from code review

a01aa66

polarathene previously approved these changes Jan 24, 2024

View reviewed changes

Merge branch 'master' into tests/new_filters

f077f38

georglauterbach dismissed polarathene’s stale review via f077f38 January 24, 2024 16:16

georglauterbach requested a review from polarathene January 24, 2024 16:16

georglauterbach enabled auto-merge (squash) January 24, 2024 16:16

correct small mistake during conflict resolution

7900e34

polarathene approved these changes Jan 24, 2024

View reviewed changes

georglauterbach merged commit ed1e1eb into master Jan 24, 2024

georglauterbach deleted the tests/new_filters branch January 24, 2024 22:06

                 _service_log_should_contain_string 'rspamd' 'reject "ClamAV FOUND VIRUS "Eicar-Signature"'
-                _print_mail_log_for_id "${MAIL_ID_VIRUS}"
+                _print_mail_log_of_queue_id_from_msgid 'rspamd-test-email-virus'

tests: new sending and filtering functions by georglauterbach · Pull Request #3786 · docker-mailserver/docker-mailserver · GitHub

Uh oh!

Uh oh!

tests: new sending and filtering functions - #3786

tests: new sending and filtering functions#3786georglauterbach merged 25 commits intomasterdocker-mailserver/docker-mailserver:masterfrom tests/new_filtersdocker-mailserver/docker-mailserver:tests/new_filtersCopy head branch name to clipboard

Conversation

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

Uh oh!

Description

Type of change

Checklist:

Uh oh!

Uh oh!

polarathene commented Jan 18, 2024

Uh oh!

georglauterbach commented Jan 19, 2024

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

polarathene Jan 21, 2024 • 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!

polarathene commented Jan 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

polarathene left a comment

Choose a reason for hiding this comment

Uh oh!

georglauterbach commented Jan 24, 2024

Uh oh!

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

tests: new sending and filtering functions#3786
georglauterbach merged 25 commits into
masterfrom
tests/new_filters

georglauterbach commented Jan 16, 2024 •
edited
Loading

polarathene Jan 21, 2024 •
edited
Loading

polarathene commented Jan 21, 2024 •
edited
Loading

georglauterbach commented Jan 24, 2024 •
edited
Loading