tests: Refactor LDAP tests to current conventions#3483
tests: Refactor LDAP tests to current conventions#3483polarathene merged 12 commits intodocker-mailserver:masterfrom
Conversation
- We have two helper methods with implicit `CONTAINER_NAME` reference, which is a bit more DRY and improves readability. - `wc -l` + `assert_output 1` converted to use helper `_should_output_number_of_lines 1` - `DOMAIN` var changed from `my-domain.com` to local testing domain `example.test`.
- Test wide ENV defined at the top - OpenLDAP build and run logic grouped together. Added notes, network alias and tty not required. - Extracted out special LDAP Postfix/Dovecot ENV into separate array. LDAP specific provisioning / auth ENV also included, with comments + linebreak to better group related ENV. - Likewise additional ENV to support test cases has been extracted to a separate array with additional comments for context. - Those two arrays are expanded back into the main `CUSTOM_SETUP_ARGUMENTS` that configure hostname and network for the DMS container.
- Covers 3 accounts to test from LDAP. - 2 are the same query against users/aliases/groups tables in Postfix, only differing by account queried (and expected as returned result). - 1 separate test to ensure a difference in config is supported correctly. - Extracted repeated test logic into a helper method. - Added additional context in comments about the creation source of these LDAP accounts and their related Postfix config / interaction. Direct reference to special case PR (since `git blame` will be less useful).
More DRY approach. With a bit more helpful failure context via `assert_output` (_and only grepping the key_). Simpler to grok what's being covered.
A bit more verbose with the new helper method. `test-email.txt` template is only used by the LDAP test, as is the `sendmail` command. Helper will take two args to support the testcases, but at a later date should be refactored to be consistent with the `_send_email()` helper (_which presently uses `nc` that is required for plain-text mail/auth, otherwise we'd have used `openssl`, bigger refactor required_).
- Dovecot quota plugin testcase revised to check files exist instead of rely on `ls` failure. - Moved Postfix quota plugin testcase into prior dovecot testcase for quota plugin check. Better error output by only querying the `smtpd_recipient_restrictions` setting (_which should be the only one configured for the service_). - Moved the saslauthd and pflogsumm testcases (_no changes beyond revised comments_) above the `ATTENTION` comment, and one testcase below the comment that belonged to that group.
- `--no-cache` was creating a new image on the Docker host each time the test is run. Shouldn't be any need to build without cache. - No need to use `pushd` + `popd`, can just provide the path context directly, and the `./Dockerfile` is an implicit default thus `-f` not required either. Additionally removed the old `checking` prefix from testcase names.
- No changes to any of these config files, just better isolation as not relevant to any other tests. - Section heading in `setup_file()` added to distinguish the remainder of the function is dedicated to the DMS container setup. - Comment providing some context about the `mv` to maintainers, this should be done after defaults are initialized but before starting up the container.
polarathene
added
area/tests
service/ldap
kind/improvement
georglauterbach
left a comment
georglauterbach
left a comment
There was a problem hiding this comment.
Looks good at first glance, but I will give it a more thorough review as soon as I have a few minutes to spare again. I will not be blocking on this though, so earlier merging without my review is fine for me.
georglauterbach
left a comment
georglauterbach
left a comment
There was a problem hiding this comment.
Looks good; just some minor changes.
| # Setup local openldap service: | ||
| # NOTE: Building via Dockerfile is required? Image won't accept read-only if it needs to adjust permissions for bootstrap files. | ||
| # TODO: Upstream image is no longer maintained, may want to migrate? | ||
| docker build -t dms-openldap test/config/ldap/docker-openldap/ |
There was a problem hiding this comment.
Just FYI: I have been wondering for a long time why we build this ourselves.. I do like that we have a TODO item here now saying that we should do something about the issue.
There was a problem hiding this comment.
Yeah, I got a bit familiar with it while working through the refactoring of the test.
I tried a few other openldap images, but ran into problems carrying over the config .ldif + .schema we have. I'll give it a bit more time today but call it quits if I can't figure out a suitable replacement.
| run docker exec mail_with_ldap ls /etc/dovecot/conf.d/90-quota.conf.disab | ||
| assert_success | ||
| } | ||
| _run_in_container_bash 'openssl s_client -quiet -connect 0.0.0.0:465 < /tmp/docker-mailserver-test/auth/sasl-ldap-smtp-auth.txt' |
There was a problem hiding this comment.
Just as a reminder for the future: _send_email should probably get a "ports" option and choose between openssl s_client and nc dynamically. Or, simpler, we provide a new helper _send_mail_openssl().
There was a problem hiding this comment.
We could, but instead of switching between openssl / nc it might also be good to just use a CLI tool that sends mail and can support all ports?
Preferably it'd ingest a different mail template, I'd prefer we handle the auth ones with the account password in plaintext, instead of the encoded (hashed? base64?) format exchanged presently for AUTH LOGIN. We have an issue with that for the skipped test, and the .ldif files have SSHA hashed passwords themselves, so I've no clue what they were originally 😅
There was a problem hiding this comment.
Yeah, that might also be a good idea. But no rush, it works fine for now
Co-authored-by: Georg Lauterbach <[email protected]>
2 participants
Description
Part of the big effort to migrate all tests to use the better helpers and conventions that support much better test maintenance (and a step towards better coverage).
TODOhave been added for future improvements.git blamehistory when changes are also applied), I've held off adjusting some email addresses (.tld/.localdomain) until after this PR is merged.All commit messages for easier reference
tests: Switch to setup helper and conventions
tests: Adapt run command to new conventions
CONTAINER_NAMEreference, which is a bit more DRY and improves readability.wc -l+assert_output 1converted to use helper_should_output_number_of_lines 1DOMAINvar changed frommy-domain.comto local testing domainexample.test.tests: Refactor
setup_file()CUSTOM_SETUP_ARGUMENTSthat configure hostname and network for the DMS container.tests: Refactor the LDAP account table query testcase
git blamewill be less useful).tests: Use iteration for
grepsetting checksassert_output(and only grepping the key). Simpler to grok what's being covered.tests: DRY test email delivery
test-email.txttemplate is only used by the LDAP test, as is thesendmailcommand._send_email()helper (which presently usesncthat is required for plain-text mail/auth, otherwise we'd have usedopenssl, bigger refactor required).tests: Slight revisions and relocating testcases
lsfailure.smtpd_recipient_restrictionssetting (which should be the only one configured for the service).ATTENTIONcomment, and one testcase below the comment that belonged to that group.tests: Simplify openldap
docker buildcommand--no-cachewas creating a new image on the Docker host each time the test is run. Shouldn't be any need to build without cache.pushd+popd, can just provide the path context directly, and the./Dockerfileis an implicit default thus-fnot required either.checkingprefix from testcase names.tests: Move LDAP specific config into
test/config/ldap/setup_file()added to distinguish the remainder of the function is dedicated to the DMS container setup.mvto maintainers, this should be done after defaults are initialized but before starting up the container.Type of change
Checklist: