Skip to content

tests(refactor): mail_hostname.bats#3027

Merged
polarathene merged 20 commits intodocker-mailserver:masterfrom
polarathene:tests/migrate-hostname
Jan 29, 2023
Merged

tests(refactor): mail_hostname.bats#3027
polarathene merged 20 commits intodocker-mailserver:masterfrom
polarathene:tests/migrate-hostname

Conversation

@polarathene
Copy link
Copy Markdown
Member

Description

mail_hostname.bats has been converted into the new testing format and completely refactored to share common tests by grouping them into functions, improved inline docs and expanded test coverage.

There was some overlap in tests.bats, where coveraage is better in mail_hostname.bats. Additionally the tests for graceful shutdown seems like they were misplaced, but I have now relocated back into the supervisord tests (the check was originally added in Nov 2018, and additionally covers the grace period added in May 2021?).

For reference, I originally did a thorough review of the tested network hostname configs and logic changes, refactoring and extracting out the larger original PR into smaller ones in Nov 2021.

Commit history scopes the refactor progress if that's helpful for tracking changes vs the diff alone.

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
  • 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

Based on changes from an earlier closed hostname PR from Oct 2021 with additional revision to use `assert_output` and more thorough checking of values expected in output.
This was originally a single test case in `tests.bats` intended for `supervisord` testing.

It seems at some point it got reassigned to a hostname override test container, and then migrated to separate test file for hostname override test by accident.

It now belongs in the correct place again, as hostname config should have nothing to do with a graceful shutdown?
Wait for SMTP port is left at the end to avoid additional start-up delays.
- I could do multiple container grep calls instead, but opted to match by lines in file. This better ensures values are being matched to the correct lines.
- Renamed the test case descriptions.
- Expanded test coverage of the 4th container as it represents another DNS config, while the 3rd is just the 4th container with the `SRS_DOMAINNAME` env added, no value in more coverage there.
These checks are performed in `mail_hostname.bats` with better coverage.
The original `fqdn-with-subdomain` is now `with-nis-domain` which is more accurate. A new test case will properly cover the default `--hostname` only config that is not a bare domain.
This commit just shifts the test cases, no new changes to any content beyond that.
@polarathene polarathene added this to the v12.0.0 milestone Jan 26, 2023
@polarathene polarathene self-assigned this Jan 26, 2023
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼 Some minor remarks left.

Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
Comment thread test/tests/serial/mail_hostname.bats Outdated
polarathene and others added 2 commits January 27, 2023 11:02
- Fix a suggested change bug with quote wrapping an interpolated variable.
- Convert two other `_bash` methods that were missed from review.
- Apply the last two suggested changes from review.
@polarathene polarathene force-pushed the tests/migrate-hostname branch from 8ff1bb1 to 07877a6 Compare January 26, 2023 22:17
The `| head -n 1` can be dropped if we know for sure it's only one line, which is what we expect. Quotes can then be dropped too.
@polarathene
Copy link
Copy Markdown
Member Author

All test feedback resolved, including suggestions you wanted verified 👍

Copy link
Copy Markdown
Member

@georglauterbach georglauterbach left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@polarathene polarathene enabled auto-merge (squash) January 29, 2023 12:33
@polarathene polarathene merged commit 14829a8 into docker-mailserver:master Jan 29, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants