tests(refactor): open_dkim.bats
#3060
Conversation
- These all do roughly the same logic that can be split into two separate methods. - `_should_generate_dkim_key()` covers a bit more logic as it can be leveraged to handle other test cases that also perform the same logic. - The `config/opendkim/` doesn't seem necessary for tests. Only the first few test cases here are testing against it, so we can conditionally make that available. `process_check_restart.bats` also depended on it to run OpenDKIM successfully, but this was due to the `setup-stack.sh` config defaults failing to find an "empty" file forcing `supervisord` to constantly restart the process.. - With this, there we inverse the default opendkim config, so we don't have to mount unique / empty subfolders for each test case, followed by copying over the two extra configs.
All the remaining test cases but the last one were refactored here for a clean commit diff. The last test case will be refactored in the following commit. Plenty of repeated logic spread across these test cases, now condensed into shared methods.
- As the majority of test cases are only running `open-dkim` helper, we don't actually have to wait for a full container setup. So an alternative container start is called. - Also improves assertions a bit more instead of just counting lines. - Some test cases don't bind mount all of `/tmp/docker-mailserver` contents, thus don't raise permission errors on subsequent test runs. - Instead of `rm -f` on some config files, have opted to mount them read-only instead, or alternatively mount an anonymous empty volume instead. - Collapsed the first three test cases into one, thus no `setup_file()` necessary. - Shift the `_wait_for_finished_setup_in_container()` method into `_common_container_setup()` instead since nothing else is using `_common_container_start()` yet, this allows for avoiding the wait.
This makes these tests a bit more DRY, and enhances the raised quality issue with these tests. Now not only is the domain checked in the generated DNS dkim record, but we also verify the key size is corrected in the public and private keys via openssl.
- `__should_have_tables_trustedhosts_for_domain` shifted in each test case to just after generating the domains keys. - Asserts `open-dkim` logs instead of just counting them. - Added checks for domains that should not be present in a test case. - Additional coverage and notes about the alias from vhost `@localdomain.com` - Single assert statement with switch statement as all are using common args.
polarathene
added
area/tests
kind/improvement
The order printed from local system vs CI differed causing the CI to fail. The order of lines is irrelevant so `--index` is not required. Additionally correct the prefix of the called method to be only one `_` now that it's a `common.bash` helper method.
These cover the same test logic for the most part, the first domain could also be testing the custom selector. `special_use_folders.bats` + `mailbox_format_dbox` can assert lines instead, removing the need for `--partial`.
polarathene
force-pushed
the
tests/refactor-opendkim
branch
from
a5578c7
to
78d874b
Compare
test/tests/serial/open_dkim.bats
Outdated
| local EXPECTED_DOMAIN=${2} | ||
| local EXPECTED_SELECTOR=${3:-'mail'} | ||
|
|
||
| case "${ASSERT_FOR}" in |
There was a problem hiding this comment.
Do we want / need a default ( * ) for this switch-case statement? Usually good practice to catch issues.
There was a problem hiding this comment.
I can add one, but it's only a method used by the direct 3 wrapping methods preceding it. And only this test would use this method.
There was a problem hiding this comment.
Should I just remove the switch statement method, and copy/paste the 3 parameters into the wrapper ones with their relevant assertion? 🤔
There was a problem hiding this comment.
Should I just remove the switch statement method, and copy/paste the 3 parameters into the wrapper ones with their relevant assertion? 🤔
This may actually be the best idea, yes. It removes the need for the default case and eliminates some complexity as well.
Co-authored-by: Georg Lauterbach <44545919+georglauterbach@users.noreply.github.com>
There was a problem hiding this comment.
LGTM 👍🏼
Now you just have to take care of removing the extra slash for tests to pass :D (see https://github.com/docker-mailserver/docker-mailserver/actions/runs/4112519553/jobs/7097549778)
polarathene
force-pushed
the
tests/refactor-opendkim
branch
from
222ef0f
to
e3766e3
Compare
Initial investigation (with CI run logs linked)It seems our CI is buggered: BuildKit in Docker Engine would be Weird since we have explicitly opted out of provenance 🤔 It has the same Docker Engine version, but Buildx was I am going to restart the parallel tests and see if that make a difference. Further investigation
Attempt to resolve by clearing CI cache (no luck)
A CI run for this PR was working yesterday before the I'm wondering if we remove I have noticed that the builds are performed with the buildx Hence the BuildKit version is too low and a safeguard is causing the false-positive. That seems to not be triggered with newer buildx Problem is due to PR raised to resolve it: #3072 |
Description
2 years later, but we've finally done it 😛
This test has been refactored heavily and should address the concerns raised in the comments and past discussions about it. There was a mention of a
checkkeyfeature that could be used, but that requires setting up DNS. In future once we do have a DNS service in the tests, I'd rather just do a test that sends and receives mail, while verifying DKIM was handled correctly.I staged out changes across commits with commit messages in each if anyone wants to compare to the original with easier diffs to follow.
Feature history
KeyTableconfig file to avoid start-up issues withopendkim(the associated issue provides actual details).supervisordrestart theopendkimprocess over 4k times spamming error logs with a related failure once the default opendkim config folder for tests was renamed.open-dkim domainfeature.2048default, but no action has been taken since (I'll probably resolve that after this PR).open-dkim.open-dkim keysizefeature (and associated tests). There was a mention of not being able to use openssl successfully at the time for tests./etc/resolv.confat start-up and associated test. Justified in the associated issue for PR, although the test is only checking that an IP exists, so could be improved there.chrootjail, it would not be in sync if changes are made to/etc/resolve.conf.1.1.1.1.mail.vhostshandling / generation was adjusted. I documented many of the same references above, among some other DKIM related info there.Type of change
Checklist: