-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
tests(refactor): Adjust mail_changedetector
+ change detection helpers
#2997
Merged
polarathene
merged 18 commits into
docker-mailserver:master
from
polarathene:tests/migrate-changedetector
Jan 16, 2023
Merged
Changes from all commits
Commits
Show all changes
18 commits
Select commit
Hold shift + click to select a range
abafd43
tests(refactor): `mail_changedetector.bats` - Leverage DRY methods
polarathene 5b28815
tests(chore): Remove sleep and redundant change event
polarathene 4530f25
tests(refactor): Add more DRY methods
polarathene 4729d10
tests(chore): Revise the change detection helper method
polarathene 4378031
tests(chore): Migrate to current test conventions
polarathene 3596a29
tests(chore): Remove legacy change detection
polarathene c3dbe4a
chore: Lock removal should not incur `sleep 5` afterwards
polarathene 5a4bd9b
tests(chore): `tls_letsencrypt.bats` leverage improved change detection
polarathene c9a8d5c
tests(chore): Convert `setup-cli.bats` to new test conventions
polarathene ae3dc4c
tests(chore): Extract out helpers related to change-detection
polarathene fa4ec9c
tests(refactor): `tls_letsencrypt.bats` remove `_get_service_logs()`
polarathene a04a147
tests(chore): Remove common setup methods from `test_helper/common.bash`
polarathene a28d19d
chore: Minor style changes
polarathene 91610dc
chore: Relocate inline docs
polarathene 32aea55
chore: Simplify setting `EXPECTED_COUNT`
polarathene 655bb50
Revert "chore: Simplify setting `EXPECTED_COUNT`"
polarathene 0893c98
chore: Use `|| true` to simplify setting `EXPECTED_COUNT` correctly
polarathene cbf58ef
Merge branch 'master' into tests/migrate-changedetector
georglauterbach File filter
Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
#!/bin/bash | ||
|
||
load "${REPOSITORY_ROOT}/test/helper/common" | ||
|
||
function wait_until_change_detection_event_begins() { | ||
local MATCH_CONTENT='Change detected' | ||
local MATCH_IN_LOG='/var/log/supervisor/changedetector.log' | ||
|
||
_wait_until_expected_count_is_matched "${@}" | ||
} | ||
|
||
# NOTE: Change events can start and finish all within < 1 sec, | ||
# Reliably track the completion of a change event by counting events: | ||
function wait_until_change_detection_event_completes() { | ||
local MATCH_CONTENT='Completed handling of detected change' | ||
local MATCH_IN_LOG='/var/log/supervisor/changedetector.log' | ||
|
||
_wait_until_expected_count_is_matched "${@}" | ||
} | ||
|
||
function _get_logs_since_last_change_detection() { | ||
local CONTAINER_NAME=${1} | ||
local MATCH_IN_FILE='/var/log/supervisor/changedetector.log' | ||
local MATCH_STRING='Change detected' | ||
|
||
# Read file in reverse, collect lines until match with sed is found, | ||
# then stop and return these lines back in original order (flipped again through tac): | ||
docker exec "${CONTAINER_NAME}" bash -c "tac ${MATCH_IN_FILE} | sed '/${MATCH_STRING}/q' | tac" | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I haven't compared, but casper had suggested a different approach than polling like I'm doing here.
That will take a stream of the log and exit once the result count is matched. It needed to use
bash -c
, even if usingdocker logs --follow
for the input as I found it'd hang otherwise (presumably something to do with stdin not closing?)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot reproduce the hanging. I tried this:
Then in another session:
docker exec mail supervisorctl restart update-check
PS: The container must be running with
SUPERVISOR_LOGLEVEL=info
I think, that this example works.This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was able to reproduce on current master the same issue. I tested with replacing this:
docker-mailserver/test/tests/parallel/set1/spam_virus/spam_junk_folder.bats
Lines 33 to 35 in 1650cdf
Reproduction
Perhaps it's something different with our environments or how
docker logs --follow
works on our machines? For me, given the above observations, it doesn't seem like the stream is closed?I had also verified by running the same command in another terminal session directly while the test was running. It would find the match and output it, but not exit until the timeout was reached.
I've had a similar experience with
-q
(not as nice as--max-count
), where either option was working at least fordecoder
as the value to match when the container was starting up (I could not reproduce withPassed SPAM
for some reason..). I could repeat the command in the terminal several times and it'll exit quickly with either option successful. However a few seconds after that and it would fail, even though the logs command would clearly show the lines 🤷♂️Clearly something is iffy there, at least on my end :\
UPDATE: My
grep
command is the problem.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I swapped
grep
forrg
(ripgrep), and now it works as you'd expect...So something wrong with my
grep
command I guess? 🤷♂️I'd rather avoid the
-q
or--max-count
approach, at least with a local grep call, as it seems that's where the problem is. The containers grep was working fine withtail
, which would be ok as it avoids false positives when I run tests locally.We could add
ripgrep
as a dependency for testing like we do withjq
I suppose? (but then CI may need to grab/install it each time, unless it already provides that like it doesjq
)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes 😄
docker logs ${CONTAINER_NAME} | grep 'Passed SPAM {RelayedTaggedInbound,Quarantined}'"
When you don't pass
-q
or--max-count 1
, grep runs infinitly. Why should it stop?If you only need to now, if a keyword was found, go with
-q
. If you need the output to parse further or make assertions, go with--max-count 1
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're referring to the current version in master I referenced? That is not using
--follow
, so it does not run infinitely. The attempts I tried can be found in the collapsed "> Reproduction" section.I collapsed it after responding with an update about my local
grep
being the problem. I understand the reason for-q
or--max-count 1
, but only if we're usinggrep
within the container (doesn't make sense fordocker logs --follow
due to that), as for whatever reason my localgrep
is misbehaving which would be problematic for me running tests locally 😅 (something I've been doing quite frequently lately)If we're to use either option with
docker logs --follow
, then I'd need to know why mygrep
is broken and how to fix it, or addripgrep
as a dependency for tests.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I overlooked that.
BTW: Are you running the tests on debian/ubuntu?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
EndeavourOS (ArchLinux based)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Depending on how much more time you want to invest, you may try the "grep" binary from debian:
I am wondering, that such a basic tool behaves differently between linux distros.
PS: If we are ever in doubt, we should stick with debian based distros supported for testing (because CI is also running on ubuntu).