-
Notifications
You must be signed in to change notification settings - Fork 135
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
test: add test for default utp concurrency limit #1514
Conversation
70a070b
to
596a813
Compare
596a813
to
b820475
Compare
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.
@@ -29,7 +29,7 @@ where | |||
Fut: Future<Output = Result<O>>, | |||
{ | |||
// If content is absent an error will be returned. | |||
for counter in 0..60 { |
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 think this made some tests flaky, at least when running on circleci. The test that is frequently failing (with "Retried too many times" panic) is peertest_history_offer_propagates_gossip_multiple_large_content_values
.
Test passes fine when I run it locally.
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.
Hmmm, thanks for the heads up. I'll look into it
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.
Well, I made this change since it's a real pain to wait for a minute if the test fails locally. I assumed (incorrectly) that this change wouldn't affect any of our tests since the wait time is fairly long. But, I have no problem with you reverting the 10
-> 60
in #1515 . It's fairly simple to make this change when testing locally if it becomes a pain in the future
What was wrong?
Added a test to make sure that trin will always manage at least the default # of concurrent utp txs. The test/constant can also easily be increased to test trin's real tx capacity (currently somewhere around 100 from what I've seen, but still working to see if this can be improved)
How was it fixed?
Added the test
To-Do