tests: Refactored bounced spam test + Introduce common container setup template #2198
Conversation
Looks like when the 2nd (B) test case was added, they originally shared the same PRIVATE_CONFIG mount (before PRIVATE_CONFIG was a thing), and a copy/paste error was made with the undefined container name. Fixed the container name to be distinct, and made the two PRIVATE_CONFIG distinct (A and B).
Convert indentations of 4 spaces per level to 2 spaces for consistency in the file.
Should assist maintainers like myself that are not yet familiar with this functionality, saving some time :)
DRY'd up the test and extracted a common init pattern for other tests to adopt in future.
The test does not need to run distinct containers at once, so a common name is fine, although the `init_with_defaults()` method could be given an arg to add a suffix: `init_with_defaults "_${BATS_TEST_NUMBER}"` which could be called in `setup()` for tests that can benefit from being run in parallel.
Often it seems the containers only need the bare minimum config such as accounts provided to actually make the container happy to perform a test, so sharing a `:ro` config mount is fine, or in future this could be better addressed.
---
The test would fail if the test cases requiring smtp access ran before postfix was ready (_only a few seconds after setup scripts announce being done_). Added the wait condition for smtp, took a while to track that failure down.
georglauterbach
added
area/tests
kind/improvement
| assert_success | ||
|
|
||
| wait_for_finished_setup_in_container "${TEST_NAME}" | ||
| } |
There was a problem hiding this comment.
GitHub says here is no newline character at the end of this file. Intended?
There was a problem hiding this comment.
It does that to me too randomly. Very odd when it happens.
There was a problem hiding this comment.
Possibly an auto-formatting issue?
There was a problem hiding this comment.
Nope not intended, usually my editor adds them on save. I noticed similar was happening with a PR from @NorseGaud , although I'm not sure if I was overly fussed about it, we may have some existing files that are missing new line at end of file, is there an easy way to check all files for it?
EDIT: Oh, I should have refreshed, I see there was more than one comment since, whoops 😅
There was a problem hiding this comment.
I know that I copy/pasted this portion to the file from the bats test that I originally had it in. It's not a Github thing as my local commit has it missing too.
Looks like I need to tell VSCode to make sure it adds new lines on save, or get it to load the .editorconfig file, which I recall being a hassle with VSCode to get working properly in the past :\
Do either of you have .editorconfig working with VSCode? Otherwise another option is to have a local git commit hook or github workflow on merge that ensures compliance with .editorconfig, actually I thought we had a lint for this...?
There was a problem hiding this comment.
but I can test it locally again.
Sure but lets not enforce the linting on that just yet (assuming many lint issues are raised as a result).
There was a problem hiding this comment.
Do either of you have
.editorconfigworking with VSCode?
I'm using the same extension @georglauterbach mentioned and never had an issue before 💁🏻
Name: EditorConfig for VS Code
Id: editorconfig.editorconfig
Description: EditorConfig Support for Visual Studio Code
Version: 0.16.4
Publisher: EditorConfig
VS Marketplace Link: https://marketplace.visualstudio.com/items?itemName=EditorConfig.EditorConfig
or github workflow on merge that ensures compliance with
.editorconfig, actually I thought we had a lint for this...
Should be covered by
We need to check if this lint ever worked and which config it is using ;)
There was a problem hiding this comment.
and never had an issue before 💁🏻
I haven't tried in a while, but I recall it not working properly with VSCode when I last tried, while my previou editor Atom had no issue. I'm on Linux, if one of you are also, then it's probably an issue with my system. I'll try that extension again at some point :)
There was a problem hiding this comment.
Linux swift 5.11.0-34-generic #36-Ubuntu SMP Thu Aug 26 19:22:09 UTC 2021 x86_64 x86_64 x86_64 GNU/Linux :D
georglauterbach
removed
the
pr/waiting for other PR to get merged
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Description
Originally I spotted a typo in this test when it failed on me the other day. Earlier commit resolved that, but I've since refactored due to the 2nd change (adding new test helper methods).
I have also been wanting to better consolidate container setup for most tests which tend to just add some env and copy/paste other common options, this introduces an initial approach to handle that.
I also noticed that tests were starting a few seconds too fast with the original wait condition, resulting in failure when trying to connect to an unintialized postfix (becomes available a few seconds after the ready state in the test environment). Someone put together some awesome test helpers, so that was an easy fix :)
In a future update, tests will get grouped for delegating to multiple job runners in parallel. Tests will use actual
.envfiles unless there is better suggestion (not really a fan of the heredoc syntax).Original bounced spam test PR: #1485
Type of change
The new test helper might need some docs, but it's not in use by any other tests as of yet and still needs to be iterated on a bit, so beyond this PR, not much value in documenting it further?
Checklist:
docs/)