eth/downloader: Updated downloader tests to improve perf and reliability#15337
Conversation
|
Thank you for your contribution! Your commits seem to not adhere to the repository coding standards
Please check the contribution guidelines for more details. This message was auto-generated by https://gitcop.com |
Updated use of Parallel and added some subtests to help isolate them. Increased timeout in floodingTestPeer.RequestHeadersByNumber, so that it doesn't accidently timeout and cause other tests to break. Extra sleep for the first failure in TestFastCriticalRestarts.
rojotek
force-pushed
the
downloader-tests-fixes
branch
from
b464648 to
35d587b
Compare
|
GitCop message has been adressed by squashing commits together. Current failing test is far away from fixes included in this commit. |
| // We use data driven subtests to manage this so that it will be parallel on its own | ||
| // and not with the other tests, avoiding intermittent failures. | ||
| func TestDeliverHeadersHang(t *testing.T) { | ||
| testCases :=[]struct { |
There was a problem hiding this comment.
I think you forgot a space after :=
There was a problem hiding this comment.
You should use gofmt to avoid those kind of issues.
| // We use data driven subtests to manage this so that it will be parallel on its own | ||
| // and not with the other tests, avoiding intermittent failures. | ||
| func TestFastCriticalRestarts(t *testing.T) { | ||
| testCases :=[]struct { |
There was a problem hiding this comment.
Another space missing here.
| func TestDeliverHeadersHang64Fast(t *testing.T) { testDeliverHeadersHang(t, 64, FastSync) } | ||
| func TestDeliverHeadersHang64Light(t *testing.T) { testDeliverHeadersHang(t, 64, LightSync) } | ||
| // We use data driven subtests to manage this so that it will be parallel on its own | ||
| // and not with the other tests, avoiding intermittent failures. |
There was a problem hiding this comment.
Why does running these tests synchronously cause intermittent failures?
There was a problem hiding this comment.
I don't have a deep understanding, but suspect that a timeout in a different test was interacting badly with these. There is still parallel tests being run, with all the header tests running in parallel, but not in parallel with the other tests.
|
@armaniferrante @mexskican I've done the formatting fixes, and added a comment about the parallel, please let me know if you have any additional feedback. |
|
Looks good to me, though I'm uncertain about the two time related changes. Someone more familiar should also take a look as well. |
|
|
||
| // If it's the first failure, pivot should be locked => reenable all others to detect pivot changes | ||
| if i == 0 { | ||
| time.Sleep(150 * time.Millisecond) // Make sure no in-flight requests remain |
There was a problem hiding this comment.
If you look at the lines of code just below, there is a sleep directly before failing. I found that moving the sleep to just before the explicit fail was a better place to have it, and made the test more reliable.
There was a problem hiding this comment.
@fjl Hi, just checking in to see if you saw my reply to your comment?
…TestFastCriticalRestartsFail63 in OSX
6 participants
eth/downloader: Updated use of Parallel and added some subtests to help isolate them.
Increased timeout in floodingTestPeer.RequestHeadersByNumber, so that it
doesn't accidently timeout and cause other tests to break.