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/postgres/unit_test.go | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-) (limited to 'internal/postgres/unit_test.go') diff --git a/internal/postgres/unit_test.go b/internal/postgres/unit_test.go index 9c7813c0..5f83af4c 100644 --- a/internal/postgres/unit_test.go +++ b/internal/postgres/unit_test.go @@ -24,8 +24,7 @@ import ( func TestGetUnitMeta(t *testing.T) { t.Parallel() - ctx, cancel := context.WithTimeout(context.Background(), testTimeout*2) - defer cancel() + ctx := context.Background() testGetUnitMeta(t, ctx) } @@ -207,8 +206,7 @@ func TestGetUnitMetaBypass(t *testing.T) { t.Parallel() testDB, release := acquire(t) defer release() - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() + ctx := context.Background() bypassDB := NewBypassingLicenseCheck(testDB.db) @@ -580,8 +578,7 @@ func TestGetUnit(t *testing.T) { t.Parallel() testDB, release := acquire(t) defer release() - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() + ctx := context.Background() InsertSampleDirectoryTree(ctx, t, testDB) // Add a module that has READMEs in a directory and a package. @@ -773,8 +770,7 @@ func TestGetUnit_SubdirectoriesShowNonRedistPackages(t *testing.T) { t.Parallel() testDB, release := acquire(t) defer release() - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() + ctx := context.Background() m := sample.DefaultModule() m.IsRedistributable = false @@ -786,8 +782,7 @@ func TestGetUnitFieldSet(t *testing.T) { t.Parallel() testDB, release := acquire(t) defer release() - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() + ctx := context.Background() readme := &internal.Readme{ Filepath: "a.com/m/dir/p/README.md", @@ -878,8 +873,7 @@ func TestGetUnitBuildContext(t *testing.T) { t.Parallel() testDB, release := acquire(t) defer release() - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() + ctx := context.Background() // Add a module that has documentation for two Go build contexts. m := sample.Module("a.com/twodoc", "v1.2.3", "p") @@ -971,8 +965,7 @@ func TestGetUnitBypass(t *testing.T) { t.Parallel() testDB, release := acquire(t) defer release() - ctx, cancel := context.WithTimeout(context.Background(), testTimeout) - defer cancel() + ctx := context.Background() bypassDB := NewBypassingLicenseCheck(testDB.db) m := nonRedistributableModule() -- cgit v1.3