From 76f4137fe5bb3d97d1ffa28cb490e71a96d5dcb9 Mon Sep 17 00:00:00 2001 From: "Bryan C. Mills" Date: Tue, 22 Aug 2023 15:23:22 -0400 Subject: all: remove arbitrary hard-coded timeouts in tests If a test times out, that implies that it got stuck on something. By default, the Go testing package dumps goroutines when its own timeout is passed, which prints a goroutine dump, helping to reveal what was stuck. Adding an arbitrary timeout on top of the testing package's own timeout is, in my experience, almost always counterproductive. If the arbitrary timeout catches a real hang, it causes the test to fail instead of dumping goroutines, making it much harder to see what was stuck. On the other hand, if the timeouts are set aggressively enough to make the test fail early, they are often too aggressive for CI testing, causing flakes that then have to be triaged on an ongoing basis. On balance, the value of saving a minute or two for developers who have introduced a hang is not worth the cost of suppressing debugging information and causing flakes that have to be triaged. Fixes #61556. For #59347. Change-Id: I0263d0d9b18283470f100e5a0155818b87b5312f Reviewed-on: https://go-review.googlesource.com/c/pkgsite/+/521837 TryBot-Result: Gopher Robot Auto-Submit: Bryan Mills Run-TryBot: Bryan Mills kokoro-CI: kokoro Reviewed-by: Michael Matloob --- internal/source/source_test.go | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) (limited to 'internal/source') diff --git a/internal/source/source_test.go b/internal/source/source_test.go index 99e390fe..451e39f9 100644 --- a/internal/source/source_test.go +++ b/internal/source/source_test.go @@ -22,8 +22,7 @@ import ( ) var ( - testTimeout = 2 * time.Second - record = flag.Bool("record", false, "record interactions with other systems, for replay") + record = flag.Bool("record", false, "record interactions with other systems, for replay") ) func TestModuleInfo(t *testing.T) { @@ -551,7 +550,6 @@ func TestModuleInfoDynamic(t *testing.T) { client := &Client{ httpClient: &http.Client{ Transport: testTransport(testWeb), - Timeout: testTimeout, }, } // The version doesn't figure into the interesting work and we test versions to commits @@ -731,6 +729,12 @@ func TestRemoveVersionSuffix(t *testing.T) { func TestAdjustVersionedModuleDirectory(t *testing.T) { ctx := context.Background() + + testTimeout := 72 * time.Hour // arbitrary + if deadline, ok := t.Deadline(); ok { + testTimeout = time.Until(deadline) * 9 / 10 // Allow 10% for error reporting and cleanup. + } + client := NewClient(testTimeout) client.httpClient.Transport = testTransport(map[string]string{ // Repo "branch" follows the "major branch" convention: versions 2 and higher -- cgit v1.3-6-g1900