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 <gobot@golang.org>
Auto-Submit: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
kokoro-CI: kokoro <noreply+kokoro@google.com>
Reviewed-by: Michael Matloob <matloob@golang.org>

1 files changed, 6 insertions, 14 deletions

diff --git a/internal/database/database_test.go b/internal/database/database_test.go
index e48caf99..43b02d74 100644
--- a/internal/database/database_test.go
+++ b/internal/database/database_test.go
@@ -24,8 +24,6 @@ import (
 	"golang.org/x/pkgsite/internal/derrors"
 )
 
-const testTimeout = 5 * time.Second
-
 const testDBName = "discovery_postgres_test"
 
 var testDB *DB
@@ -136,8 +134,7 @@ func TestBulkInsert(t *testing.T) {
 		},
 	} {
 		t.Run(test.name, func(t *testing.T) {
-			ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
-			defer cancel()
+			ctx := context.Background()
 
 			createQuery := fmt.Sprintf(`CREATE TABLE %s (
 					colA TEXT NOT NULL,
@@ -198,8 +195,7 @@ func TestBulkInsert(t *testing.T) {
 }
 
 func TestLargeBulkInsert(t *testing.T) {
-	ctx, cancel := context.WithTimeout(context.Background(), testTimeout*3)
-	defer cancel()
+	ctx := context.Background()
 	if _, err := testDB.Exec(ctx, `CREATE TEMPORARY TABLE test_large_bulk (i BIGINT);`); err != nil {
 		t.Fatal(err)
 	}
@@ -235,8 +231,7 @@ func TestLargeBulkInsert(t *testing.T) {
 }
 
 func TestBulkUpsert(t *testing.T) {
-	ctx, cancel := context.WithTimeout(context.Background(), testTimeout*3)
-	defer cancel()
+	ctx := context.Background()
 	if _, err := testDB.Exec(ctx, `CREATE TEMPORARY TABLE test_replace (C1 int PRIMARY KEY, C2 int);`); err != nil {
 		t.Fatal(err)
 	}
@@ -308,8 +303,7 @@ func TestBuildBulkUpdateQuery(t *testing.T) {
 }
 
 func TestBulkUpdate(t *testing.T) {
-	ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
-	defer cancel()
+	ctx := context.Background()
 
 	defer func(old int) { maxBulkUpdateArrayLen = old }(maxBulkUpdateArrayLen)
 	maxBulkUpdateArrayLen = 5
@@ -372,8 +366,7 @@ func TestTransactSerializable(t *testing.T) {
 	// Test that serializable transactions retry until success.
 	// This test was taken from the example at https://www.postgresql.org/docs/11/transaction-iso.html,
 	// section 13.2.3.
-	ctx, cancel := context.WithTimeout(context.Background(), testTimeout*2)
-	defer cancel()
+	ctx := context.Background()
 
 	// Once in while, the test doesn't work. Repeat to de-flake.
 	var msg string
@@ -469,8 +462,7 @@ func testTransactSerializable(ctx context.Context, t *testing.T) string {
 
 func TestCopyDoesNotUpsert(t *testing.T) {
 	// This test verifies that copying rows into a table will not overwrite existing rows.
-	ctx, cancel := context.WithTimeout(context.Background(), testTimeout)
-	defer cancel()
+	ctx := context.Background()
 	conn, err := testDB.db.Conn(ctx)
 	if err != nil {
 		t.Fatal(err)