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

fix flaky ssh integration tests #1587

Closed

Conversation

billbsing
Copy link
Contributor

What was wrong?

Related to Issue #1580

How was it fixed?

Checked that each ssh.post is followed by a time.sleep(1)

@kclowes
Copy link
Collaborator

kclowes commented Feb 26, 2020

@billbsing after running the integration tests a few times, I think they're still flaky. I ran into this in the Parity integration tests a while back, see #1447. I think the best option is just to mark it as flaky like I did in #1447. Let me know if you disagree or if you don't want/have time to implement and I can pick it up. Thanks!

@billbsing
Copy link
Contributor Author

billbsing commented Feb 27, 2020

@billbsing after running the integration tests a few times, I think they're still flaky. I ran into this in the Parity integration tests a while back, see #1447. I think the best option is just to mark it as flaky like I did in #1447. Let me know if you disagree or if you don't want/have time to implement and I can pick it up. Thanks!

I have added xfail on a ValueError around each test method using a post.

I'm ok to try and find a better solution, if this does not work, or just cancel this PR and continue skipping the flaky tests for the time being.

@billbsing billbsing changed the title make sure a sleep occurs after each ssh post command fix flaky ssh integration tests Feb 27, 2020
Copy link
Collaborator

@kclowes kclowes left a comment

Choose a reason for hiding this comment

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

@billbsing I don't think we need the time.sleep(1) if we have the xfails. I also think have only seen it fail on the remove_filter, remove_filter_deprecated, shh_post, and shh_post_deprecated, so I'd vote to only keep the xfail decorators on those tests until we start seeing others fail. I could be convinced otherwise, however, if you have a strong opinion.

On a higher level, it's strange that they just started failing, and that I've only seen them fail on geth 1.7. This fix is definitely treating the symptom rather than the cause and I haven't had time to dig into why they are suddenly failing periodically. If you have ideas you want to explore, feel free!

@@ -219,6 +236,10 @@ def test_shh_async_filter_deprecated(self, web3: "Web3") -> None:

watcher.stop()

# Sometimes the post fails because PoW is too low.
# We don't care if an error or a True response comes back,
# we only care that we're interfacing correctly with Parity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# we only care that we're interfacing correctly with Parity
# we only care that we're interfacing correctly with Geth

@@ -246,6 +267,10 @@ def test_shh_remove_filter_deprecated(self, web3: "Web3") -> None:
except BaseException:
assert True

# Sometimes the post fails because PoW is too low.
# We don't care if an error or a True response comes back,
# we only care that we're interfacing correctly with Parity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# we only care that we're interfacing correctly with Parity
# we only care that we're interfacing correctly with Geth

@@ -358,6 +383,10 @@ def test_shh_symmetric_key_pair_from_password(self, web3: "Web3") -> None:
#
# shh_post
#
# Sometimes the post fails because PoW is too low.
# We don't care if an error or a True response comes back,
# we only care that we're interfacing correctly with Parity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# we only care that we're interfacing correctly with Parity
# we only care that we're interfacing correctly with Geth


# Sometimes the post fails because PoW is too low.
# We don't care if an error or a True response comes back,
# we only care that we're interfacing correctly with Parity
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
# we only care that we're interfacing correctly with Parity
# we only care that we're interfacing correctly with Geth

@billbsing
Copy link
Contributor Author

This PR will not resolve the flaky shh module errors.
This is now replaced by PR #1590

@billbsing billbsing closed this Mar 1, 2020
@billbsing billbsing deleted the fix-flaky-ssh-intergation-tests branch March 9, 2020 00:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants