Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tests(fix): Adjust for local testing conditions #2606

Merged
merged 4 commits into from
May 30, 2022
Merged

tests(fix): Adjust for local testing conditions #2606

merged 4 commits into from
May 30, 2022

Conversation

polarathene
Copy link
Member

Description

When running some tests locally, there were several failures unrelated to the feature being tested. These were resolved and covered by this PR.

  • Tests were run in a local VM where a few failed due to timeouts but would have been successful otherwise. Needed about 30 seconds (instead of current 20), but have increased to 60 seconds. The affected tests continue as soon as success, only failure would take longer due to increased timeout.
  • --hostname + --domainname with both as multi-label was causing helpers/dns.sh to fail at container startup. --domainname isn't necessary for the affected container under test. Removed to avoid panic. Possible cause is CI running via Ubuntu 20.04 which may run an older Docker daemon, or some other environment condition causing hostname -f to behave this way. Reminder --domainname is for setting NIS domain, not DNS domain, so this should be a non-issue.
  • setup.sh would fail in test/tests.bats when the mail container name was not targeted for the command, if an existing docker-mailserver container was already running separate from the tests (or lingering from a failed test that didn't clean it up).
  • View diffs with white-space ignored, or individual commits as one commit normalizes white-space (I got annoyed by the mixed tab/spaces indentation and finally addressed it). I can split this out into a separate PR if preferred.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • 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
  • New and existing unit tests pass locally with my changes

@polarathene
fix: Increase timeouts
3773b99 
Running tests locally via a VM these tests would fail sometimes due to the time from being queued and Amavis actually processing being roughly around 30 seconds.

There should be no harm in raising this to 60 seconds, other than delaying a failure case which will ripple through other time sensitive tests.

It's better to pass when functionality is actually correct but just needs a bit longer to complete.
@polarathene
fix: Don't setup an invalid hostname
ded4e85 
Containers were not removing themselves after failures either (missing teardown). Which would cause problems when running tests again.

---

During container startup `helpers/dns.sh` would panic with `hostname -f` failing.

Dropping `--domainname` for this container is fine and does not affect the point of it's test.

---

It's unclear why this does not occur in CI. Possibly changes within the docker daemon since as CI runs docker on Ubuntu 20.04? (2020).

For clarity, this may be equivalent to setting a hostname of `domain.com.domain.com`, or `--hostname` value truncated the NIS domain (`--domainname`) of the same value.

IIRC, it would still fail with both options using different values if `--hostname` was multi-label. I believe I've documented how non-deterministic these options can be across different environments.

`--hostname` should be preferred. There doesn't seem to be any reason to actually need `--domainname` (which is NIS domain name, unrelated to the DNS domain name). We still need to properly investigate reworking our ENV support that `dns.sh` manages.
@polarathene
chore: Normalize white-space
14eb68a 
Sets a consistent indent size of 2 spaces. Previously this varied a fair bit, sometimes with tabs or mixed tabs and spaces.

Some formatting with blank lines.

Easier to review with white-space in diff ignored. Some minor edits besides blank lines, but no change in functionality.
@polarathene
fix: setup.sh target container under test
a84285e 
Some of the `setup.sh` commands did not specify the container which was problematic if another `docker-mailserver` container was running, causing test failures.

This probably doesn't help with `test/no_container.bats`, but at least prevents `test/tests.bats` failing at this point.
@polarathene polarathene added priority/low area/tests kind/improvement Improve an existing feature, configuration file or the documentation kind/bugfix labels May 29, 2022
@polarathene polarathene added this to the v11.1.0 milestone May 29, 2022
@polarathene polarathene self-assigned this May 29, 2022
georglauterbach
georglauterbach approved these changes May 29, 2022
View reviewed changes
Copy link
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.

Big diff, a lot of formatting - thank your for doing this, very much appreciated!!!

I "quickly" had a look, if you want me to look into some sections in more detail, tell me where I can find them and I will look again, but LGTM 👍🏼

casperklein
casperklein approved these changes May 29, 2022
View reviewed changes
Copy link
Member

@casperklein casperklein left a comment

Choose a reason for hiding this comment

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

LGTM.

For the whitespace changes: I only had a quick look and did not explicitly check each line.

@polarathene
Copy link
Member Author

if you want me to look into some sections in more detail

Everything worth noting was laid out in the PR description :)

In this case it's easier to review the individual commits to avoid noise of white-space housekeeping. Not really anything needing special attention though AFAIK :)

For the white-space commit, clicking the cog on the diff view you can disable white-space diffs which will ignore displaying the indentation changes, only the blank lines and few minor edits (a comment, collapsing a multi-line, etc) would be visible then.

@polarathene polarathene merged commit 3b4f44e into docker-mailserver:master May 30, 2022
scristian71 pushed a commit to scristian71/docker-mailserver that referenced this pull request May 30, 2022
@polarathene @scristian71
tests(fix): Adjust for local testing conditions (docker-mailserver#2606)
b6d08df 
* tests(fix): Increase some timeouts

Running tests locally via a VM these tests would fail sometimes due to the time from being queued and Amavis actually processing being roughly around 30 seconds.

There should be no harm in raising this to 60 seconds, other than delaying a failure case which will ripple through other time sensitive tests.

It's better to pass when functionality is actually correct but just needs a bit longer to complete.



* tests(fix): Don't setup an invalid hostname

During container startup `helpers/dns.sh` would panic with `hostname -f` failing.

Dropping `--domainname` for this container is fine and does not affect the point of it's test.

---

It's unclear why this does not occur in CI. Possibly changes within the docker daemon since as CI runs docker on Ubuntu 20.04? (2020).

For clarity, this may be equivalent to setting a hostname of `domain.com.domain.com`, or `--hostname` value truncated the NIS domain (`--domainname`) of the same value.

IIRC, it would still fail with both options using different values if `--hostname` was multi-label. I believe I've documented how non-deterministic these options can be across different environments.

`--hostname` should be preferred. There doesn't seem to be any reason to actually need `--domainname` (which is NIS domain name, unrelated to the DNS domain name). We still need to properly investigate reworking our ENV support that `dns.sh` manages.

---

Containers were also not removing themselves after failures either (missing teardown). Which would cause problems when running tests again.



* chore: Normalize white-space

Sets a consistent indent size of 2 spaces. Previously this varied a fair bit, sometimes with tabs or mixed tabs and spaces.

Some formatting with blank lines.

Easier to review with white-space in diff ignored. Some minor edits besides blank lines, but no change in functionality.



* fix: `setup.sh` target container under test

Some of the `setup.sh` commands did not specify the container which was problematic if another `docker-mailserver` container was running, causing test failures.

This probably doesn't help with `test/no_container.bats`, but at least prevents `test/tests.bats` failing at this point.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@casperklein casperklein casperklein approved these changes

@georglauterbach georglauterbach georglauterbach approved these changes

Assignees

@polarathene polarathene

Labels
area/tests kind/improvement Improve an existing feature, configuration file or the documentation priority/low
Projects
None yet
Milestone
v11.1.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@polarathene @casperklein @georglauterbach