Skip to content

tests(feat): Refactor test_helper/common.bash common_container methods#2275

Merged
polarathene merged 7 commits intodocker-mailserver:masterfrom
polarathene:tests/improve-container-template-helper
Nov 4, 2021
Merged

tests(feat): Refactor test_helper/common.bash common_container methods#2275
polarathene merged 7 commits intodocker-mailserver:masterfrom
polarathene:tests/improve-container-template-helper

Conversation

@polarathene
Copy link
Copy Markdown
Member

Description

This has been extracted from #2245 as it does not need to be part of that PR.


These are improvements for better supporting the requirements of other tests.

  • Opted for passing an array reference instead of an ENV file. This seems to be a better approach and supports more than just ENV changes.
  • Likewise, shifted to a create + start approach, instead of docker run for added flexibility.
  • Using TEST_TMP_CONFIG instead of PRIVATE_CONFIG to make the difference in usage with config volume in tests more clear.
  • Changed the config volume from read-only volume mount to be read-write instead, which seems required for other tests.
  • Added notes about logged failures from a read-only config volume during container startup.
  • Added TEST_CA_CERT as a default CA cert path for the test files volume. This can be used by default by openssl methods.

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

polarathene and others added 2 commits November 1, 2021 12:52
These are improvements for better supporting the requirements of other tests.

- Opted for passing an array reference instead of an ENV file. This seems to be a better approach and supports more than just ENV changes.
- Likewise, shifted to a `create` + `start` approach, instead of `docker run` for added flexibility.
- Using `TEST_TMP_CONFIG` instead of `PRIVATE_CONFIG` to make the difference in usage with config volume in tests more clear.
- Changed the config volume from read-only volume mount to be read-write instead, which seems required for other tests.
- Added notes about logged failures from a read-only config volume during container startup.
- Added `TEST_CA_CERT` as a default CA cert path for the test files volume. This can be used by default by openssl methods.
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.

Just one or two questions; not that good at bats myself TBH.

Comment thread test/test_helper/common.bash Outdated
Comment thread test/test_helper/common.bash
Comment thread test/mail_spam_bounced.bats Outdated
@polarathene polarathene self-assigned this Nov 2, 2021
@polarathene polarathene added area/tests kind/improvement Improve an existing feature, configuration file or the documentation labels Nov 2, 2021
@polarathene polarathene added this to the v10.3.0 milestone Nov 2, 2021
- Quote wrap the array var name being passed to be referenced by helper method.
- Better document common_container helper methods.
@polarathene polarathene force-pushed the tests/improve-container-template-helper branch from 2977ced to d8644fb Compare November 3, 2021 03:55
@polarathene
Copy link
Copy Markdown
Member Author

  • Quoted the array var name being passed to the arg to avoid confusion.
  • Added more verbose documentation to the common helper methods. Hopefully not as confusing now 😅

I'd prefer to keep the current approach for now. Can change to ${TEST_DOCKER_ARGS[@]} and $@ at a later point if worthwhile.

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 merged commit b1a74bd into docker-mailserver:master Nov 4, 2021
@georglauterbach
Copy link
Copy Markdown
Member

I think you @polarathene and I just updated the branch at the same time. My commit does the same, but has no changes 😂

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/tests kind/improvement Improve an existing feature, configuration file or the documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants