From 35d587bf6c26a3d9ac20b677396d07988dc7522c Mon Sep 17 00:00:00 2001 From: Rob Dawson Date: Wed, 18 Oct 2017 15:18:28 +1000 Subject: [PATCH 1/2] eth/downloader: Updated downloader tests to improve perf and reliability 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. --- eth/downloader/downloader_test.go | 61 +++++++++++++++++++++++++------ 1 file changed, 49 insertions(+), 12 deletions(-) diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index 58f6e9a62435..ef68f00e8968 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -704,6 +704,7 @@ func TestThrottling64Full(t *testing.T) { testThrottling(t, 64, FullSync) } func TestThrottling64Fast(t *testing.T) { testThrottling(t, 64, FastSync) } func testThrottling(t *testing.T, protocol int, mode SyncMode) { + t.Parallel() tester := newTester() defer tester.terminate() @@ -1166,6 +1167,8 @@ func TestShiftedHeaderAttack64Fast(t *testing.T) { testShiftedHeaderAttack(t, 6 func TestShiftedHeaderAttack64Light(t *testing.T) { testShiftedHeaderAttack(t, 64, LightSync) } func testShiftedHeaderAttack(t *testing.T, protocol int, mode SyncMode) { + t.Parallel() + tester := newTester() defer tester.terminate() @@ -1198,6 +1201,8 @@ func TestInvalidHeaderRollback64Fast(t *testing.T) { testInvalidHeaderRollback( func TestInvalidHeaderRollback64Light(t *testing.T) { testInvalidHeaderRollback(t, 64, LightSync) } func testInvalidHeaderRollback(t *testing.T, protocol int, mode SyncMode) { + t.Parallel() + tester := newTester() defer tester.terminate() @@ -1310,6 +1315,8 @@ func TestBlockHeaderAttackerDropping63(t *testing.T) { testBlockHeaderAttackerDr func TestBlockHeaderAttackerDropping64(t *testing.T) { testBlockHeaderAttackerDropping(t, 64) } func testBlockHeaderAttackerDropping(t *testing.T, protocol int) { + t.Parallel() + // Define the disconnection requirement for individual hash fetch errors tests := []struct { result error @@ -1665,12 +1672,26 @@ func testFakedSyncProgress(t *testing.T, protocol int, mode SyncMode) { // This test reproduces an issue where unexpected deliveries would // block indefinitely if they arrived at the right time. -func TestDeliverHeadersHang62(t *testing.T) { testDeliverHeadersHang(t, 62, FullSync) } -func TestDeliverHeadersHang63Full(t *testing.T) { testDeliverHeadersHang(t, 63, FullSync) } -func TestDeliverHeadersHang63Fast(t *testing.T) { testDeliverHeadersHang(t, 63, FastSync) } -func TestDeliverHeadersHang64Full(t *testing.T) { testDeliverHeadersHang(t, 64, FullSync) } -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. +func TestDeliverHeadersHang(t *testing.T) { + testCases :=[]struct { + protocol int + syncMode SyncMode + }{ + {62, FullSync}, + {63, FullSync}, + {63, FastSync}, + {64, FullSync}, + {64, FastSync}, + {64, LightSync}, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("protocol %d mode %v", tc.protocol, tc.syncMode), func(t *testing.T) { + testDeliverHeadersHang(t, tc.protocol, tc.syncMode) + }) + } +} type floodingTestPeer struct { peer Peer @@ -1703,7 +1724,7 @@ func (ftp *floodingTestPeer) RequestHeadersByNumber(from uint64, count, skip int // Deliver the actual requested headers. go ftp.peer.RequestHeadersByNumber(from, count, skip, reverse) // None of the extra deliveries should block. - timeout := time.After(15 * time.Second) + timeout := time.After(60 * time.Second) for i := 0; i < cap(deliveriesDone); i++ { select { case <-deliveriesDone: @@ -1732,7 +1753,6 @@ func testDeliverHeadersHang(t *testing.T, protocol int, mode SyncMode) { tester.downloader.peers.peers["peer"].peer, tester, } - if err := tester.sync("peer", nil, mode); err != nil { t.Errorf("sync failed: %v", err) } @@ -1742,12 +1762,28 @@ func testDeliverHeadersHang(t *testing.T, protocol int, mode SyncMode) { // Tests that if fast sync aborts in the critical section, it can restart a few // times before giving up. -func TestFastCriticalRestartsFail63(t *testing.T) { testFastCriticalRestarts(t, 63, false) } -func TestFastCriticalRestartsFail64(t *testing.T) { testFastCriticalRestarts(t, 64, false) } -func TestFastCriticalRestartsCont63(t *testing.T) { testFastCriticalRestarts(t, 63, true) } -func TestFastCriticalRestartsCont64(t *testing.T) { testFastCriticalRestarts(t, 64, true) } +// 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 { + protocol int + progress bool + }{ + {63, false}, + {64, false}, + {63, true}, + {64, true}, + } + for _, tc := range testCases { + t.Run(fmt.Sprintf("protocol %d progress %v", tc.protocol, tc.progress), func(t *testing.T) { + testFastCriticalRestarts(t, tc.protocol, tc.progress) + }) + } +} func testFastCriticalRestarts(t *testing.T, protocol int, progress bool) { + t.Parallel() + tester := newTester() defer tester.terminate() @@ -1776,6 +1812,7 @@ func testFastCriticalRestarts(t *testing.T, protocol int, progress bool) { // 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 if tester.downloader.fsPivotLock == nil { time.Sleep(400 * time.Millisecond) // Make sure the first huge timeout expires too t.Fatalf("pivot block not locked in after critical section failure") From b5fd6ec880587991ae1e46008ba7489d03b70327 Mon Sep 17 00:00:00 2001 From: Rob Dawson Date: Fri, 3 Nov 2017 14:23:23 +1000 Subject: [PATCH 2/2] eth/downloader: fixed formatting. --- eth/downloader/downloader_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/eth/downloader/downloader_test.go b/eth/downloader/downloader_test.go index ef68f00e8968..7d1cc8c340a8 100644 --- a/eth/downloader/downloader_test.go +++ b/eth/downloader/downloader_test.go @@ -1675,7 +1675,7 @@ func testFakedSyncProgress(t *testing.T, protocol int, mode SyncMode) { // 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 { + testCases := []struct { protocol int syncMode SyncMode }{ @@ -1765,7 +1765,7 @@ func testDeliverHeadersHang(t *testing.T, protocol int, mode SyncMode) { // 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 { + testCases := []struct { protocol int progress bool }{