Skip to content

Skip non-deterministic tests until they've been debugged#2177

Merged
georglauterbach merged 6 commits intomasterfrom
fix-tests
Sep 11, 2021
Merged

Skip non-deterministic tests until they've been debugged#2177
georglauterbach merged 6 commits intomasterfrom
fix-tests

Conversation

@georglauterbach
Copy link
Copy Markdown
Member

@georglauterbach georglauterbach commented Sep 9, 2021 •
edited
Loading

Description

Let's see whether my additions / changes can fix the unreliable tests. I need this PR to not be work in progress for tests to run. Commented the lines that fail so we have a working CI for now.

Fixes #2176

Type of change

  • Bug fix (non-breaking change which fixes an issue)

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

@georglauterbach georglauterbach added area/ci meta/needs triage This issue / PR needs checks and verification from maintainers area/tests kind/improvement Improve an existing feature, configuration file or the documentation labels Sep 9, 2021
@georglauterbach georglauterbach added this to the v10.2.0 milestone Sep 9, 2021
@georglauterbach georglauterbach requested a review from a team September 9, 2021 07:29
@georglauterbach georglauterbach self-assigned this Sep 9, 2021
@georglauterbach georglauterbach changed the title Trying to fix tests Comment sporadically failing tests Sep 9, 2021
@georglauterbach georglauterbach added pr/needs review priority/high and removed meta/needs triage This issue / PR needs checks and verification from maintainers labels Sep 9, 2021
@polarathene
Copy link
Copy Markdown
Member

polarathene commented Sep 9, 2021 •
edited
Loading

Note the tests are failing again with test checking user login: predefined user can login:

not ok 311 checking user login: predefined user can login
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 935)
#   `assert_output "passdb: [email protected] auth succeeded"' failed
# 
# -- output differs --
# expected : passdb: [email protected] auth succeeded
# actual   : passdb: [email protected] auth failed
# --
#

Additional references of failures:

EDIT: This is after merge has happened, just adding reference to this list.

Another failure often cited is: #2148 (comment)

not ok 299 checking quota: warn message received when quota exceeded
# (from function `assert_output' in file test/test_helper/bats-assert/src/assert.bash, line 239,
#  in test file test/tests.bats, line 902)
#   `assert_output "2"' failed
# 
# -- output differs --
# expected : 2
# actual   : 3
# --
#

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Sep 9, 2021

This test is also known to be flakey. Shall I comment it as well?

@georglauterbach georglauterbach mentioned this pull request Sep 9, 2021
Merged
11 tasks
@polarathene
Copy link
Copy Markdown
Member

polarathene commented Sep 9, 2021

Shall I comment it as well?

If we can't rely on it, it's not a particularly useful test?

I don't see much point in having it enabled if it can pass/fail with no relevant code changes.

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Sep 9, 2021

This PR is basically ready to be merged. Waiting for reviews :D

polarathene
polarathene previously approved these changes Sep 9, 2021
View reviewed changes
@NorseGaud
Copy link
Copy Markdown
Member

NorseGaud commented Sep 9, 2021 •
edited
Loading

@georglauterbach , can we use skip? If it works as I expect, I think that would help us from forgetting that these were a problem at some point since it would show up in output still.

@test "wait_for_empty_mail_queue_in_container succeeds within timeout" {
    skip 'disabled as it fails randomly: https://github.com/docker-mailserver/docker-mailserver/issues/2176'

Here is what it looks like on my repo

 ✓ wait_for_changes_to_be_detected_in_container fails when timeout is reached
 ✓ wait_for_changes_to_be_detected_in_container succeeds within timeout
 - wait_for_empty_mail_queue_in_container fails when timeout reached (skipped: disabled as it fails randomly: https://github.com/docker-mailserver/docker-mailserver/issues/2176)
 - wait_for_empty_mail_queue_in_container succeeds within timeout (skipped: disabled as it fails randomly: https://github.com/docker-mailserver/docker-mailserver/issues/2176)

18 tests, 1 failure, 2 skipped

@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Sep 9, 2021 •
edited
Loading

@NorseGaud I like the idea of using skip. Can you pull the branch and implement it? I'm not too familiar with BATS :)

EDIT: Nevermind, was easier than anticipated :D

@NorseGaud NorseGaud self-requested a review September 10, 2021 15:18
@NorseGaud
Copy link
Copy Markdown
Member

NorseGaud commented Sep 10, 2021

@NorseGaud I like the idea of using skip. Can you pull the branch and implement it? I'm not too familiar with BATS :)

EDIT: Nevermind, was easier than anticipated :D

Thanks man, I've been a bit overloaded lately :(

polarathene
polarathene approved these changes Sep 10, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@polarathene polarathene left a comment

Choose a reason for hiding this comment

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

Minor feedback. Approving so I don't block merging if the changes aren't worth bothering about.

Comment thread test/test_helper.bats
Comment thread test/tests.bats
Comment thread test/test_helper.bats
@polarathene polarathene changed the title Comment sporadically failing tests Skip non-deterministic tests until they've been debugged Sep 10, 2021
@georglauterbach
Copy link
Copy Markdown
Member Author

georglauterbach commented Sep 11, 2021

@polarathene I’ll merge this as is and provide a follow-up with your suggestions so CI passes again and we can merge other PRs too.

@georglauterbach georglauterbach merged commit 8877326 into master Sep 11, 2021
@georglauterbach georglauterbach deleted the fix-tests branch September 11, 2021 09:30
georglauterbach added a commit that referenced this pull request Sep 11, 2021
@georglauterbach
follow up on #2177 with @polarathene's suggestions
d0a1787
This was referenced Sep 11, 2021
Merged
Merged
polarathene pushed a commit that referenced this pull request Sep 11, 2021
@georglauterbach
follow up on #2177 with @polarathene's suggestions (#2180)
8e91251
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

@polarathene polarathene polarathene approved these changes

+1 more reviewer

@NorseGaud NorseGaud NorseGaud approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@georglauterbach georglauterbach

Labels

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

Projects

None yet

Milestone

v10.2.0

Development

Successfully merging this pull request may close these issues.

[BUG] Failing helper tests in master

4 participants

@georglauterbach @polarathene @NorseGaud @casperklein