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.

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

Skip clean_removed test #9207

Merged
merged 1 commit into from
Nov 22, 2018
Merged

Skip clean_removed test #9207

merged 1 commit into from
Nov 22, 2018

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Nov 22, 2018

See #7690

@ruflin ruflin added review flaky-test Unstable or unreliable test cases. labels Nov 22, 2018
@@ -851,6 +851,9 @@ def test_clean_removed(self):
"""
Checks that files which were removed, the state is removed
"""

raise SkipTest
Copy link
Member

Choose a reason for hiding this comment

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

Add a comment mentioning the reason why the test is skipped? or with a link to the issue?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done.

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

instead of raising SkipTest, please use the decorator and add link to #7690

import unittest
@unittest.skip("temporarily disabled")

@ruflin
Copy link
Member Author

ruflin commented Nov 22, 2018

@ph Can do, but will it make any difference?

@ruflin
Copy link
Member Author

ruflin commented Nov 22, 2018

@ph Changed.

@ph
Copy link
Contributor

ph commented Nov 22, 2018

@ph Can do, but will it make any difference?

it is just cleaner IMHO :)

Copy link
Contributor

@ph ph left a comment

Choose a reason for hiding this comment

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

LGTM

@ph ph added the needs_backport PR is waiting to be backported to other branches. label Nov 22, 2018
@ph
Copy link
Contributor

ph commented Nov 22, 2018

@ruflin I have added needs_backport label, I think we want to disable that in all the current branches.

@ruflin ruflin merged commit be187b9 into elastic:master Nov 22, 2018
@ruflin ruflin deleted the skip-clean_removed branch November 22, 2018 20:42
ruflin added a commit to ruflin/beats that referenced this pull request Nov 22, 2018
@ruflin ruflin added v6.6.0 and removed needs_backport PR is waiting to be backported to other branches. labels Nov 22, 2018
@ruflin
Copy link
Member Author

ruflin commented Nov 22, 2018

backported to 6.x. Would ignore it for now in 6.5.

@@ -847,10 +848,12 @@ def test_clean_inactive(self):
else:
assert data[0]["offset"] == 2

@unittest.skip("Skipped as flaky: https://github.com/elastic/beats/issues/7690")
Copy link
Member

Choose a reason for hiding this comment

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

👍

def test_clean_removed(self):
"""
Checks that files which were removed, the state is removed
"""

Copy link
Member

Choose a reason for hiding this comment

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

Nit. Unneeded space added.

Copy link
Member Author

Choose a reason for hiding this comment

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

nit ignored as already merged :-)

ph pushed a commit that referenced this pull request Nov 26, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flaky-test Unstable or unreliable test cases. review v6.6.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants