From df9c6e87863e9ba277b2f0520778678cf8c8a361 Mon Sep 17 00:00:00 2001 From: David Dworken Date: Sat, 21 Oct 2023 15:41:32 -0700 Subject: [PATCH] Swap to using gotestsum for retrying flaky tests --- .github/workflows/go-test.yml | 3 +- Makefile | 9 ++-- client/client_test.go | 32 +++++++------- client/testutils.go | 83 ----------------------------------- 4 files changed, 21 insertions(+), 106 deletions(-) diff --git a/.github/workflows/go-test.yml b/.github/workflows/go-test.yml index 980dd81..622c67e 100644 --- a/.github/workflows/go-test.yml +++ b/.github/workflows/go-test.yml @@ -67,7 +67,8 @@ jobs: env: DD_API_KEY: ${{ secrets.DD_API_KEY }} run: | - make retrying-test + go install gotest.tools/gotestsum@latest + make test # - name: Setup tmate session # if: ${{ failure() }} # uses: mxschmitt/action-tmate@v3 diff --git a/Makefile b/Makefile index 19e2f25..e0a4998 100644 --- a/Makefile +++ b/Makefile @@ -1,16 +1,13 @@ forcetest: go clean -testcache - TZ='America/Los_Angeles' HISHTORY_TEST=1 HISHTORY_SKIP_INIT_IMPORT=1 go test -p 1 -timeout 60m ./... + make test test: - TZ='America/Los_Angeles' HISHTORY_TEST=1 HISHTORY_SKIP_INIT_IMPORT=1 go test -p 1 -timeout 60m ./... + TZ='America/Los_Angeles' HISHTORY_TEST=1 HISHTORY_SKIP_INIT_IMPORT=1 gotestsum --packages ./... --rerun-fails=10 --format testname -- -p 1 --rerun-fails-max-failures=30 ftest: go clean -testcache - TZ='America/Los_Angeles' HISHTORY_TEST=1 HISHTORY_SKIP_INIT_IMPORT=1 go test -v -p 1 -run "$(FILTER)" ./... - -retrying-test: - for i in `seq 1 3`; do echo "Test attempt number $$i"; rm -rf ~/.hishtory/; make test && break; done + TZ='America/Los_Angeles' HISHTORY_TEST=1 HISHTORY_SKIP_INIT_IMPORT=1 gotestsum --packages ./... --rerun-fails=3 --format testname -- -p 1 -run "$(FILTER)" acttest: act push -j test -e .github/push_event.json --reuse --container-architecture linux/amd64 diff --git a/client/client_test.go b/client/client_test.go index a1d3868..c047a95 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -107,16 +107,16 @@ func TestParam(t *testing.T) { t.Run("testCustomColumns/"+tester.ShellName(), func(t *testing.T) { testCustomColumns(t, tester) }) t.Run("testUninstall/"+tester.ShellName(), func(t *testing.T) { testUninstall(t, tester) }) t.Run("testPresaving/"+tester.ShellName(), func(t *testing.T) { testPresaving(t, tester) }) - runTestsWithRetries(t, "testControlR/"+tester.ShellName(), func(t testing.TB) { testControlR(t, tester, tester.ShellName(), Online) }) + t.Run("testControlR/"+tester.ShellName(), func(t *testing.T) { testControlR(t, tester, tester.ShellName(), Online) }) } - runTestsWithRetries(t, "testControlR/offline/bash", func(t testing.TB) { testControlR(t, bashTester{}, "bash", Offline) }) - runTestsWithRetries(t, "testControlR/fish", func(t testing.TB) { testControlR(t, bashTester{}, "fish", Online) }) - runTestsWithExtraRetries(t, "testTui/search", testTui_search, 10) - runTestsWithRetries(t, "testTui/general", testTui_general) - runTestsWithRetries(t, "testTui/scroll", testTui_scroll) - runTestsWithRetries(t, "testTui/resize", testTui_resize) - runTestsWithExtraRetries(t, "testTui/delete", testTui_delete, 10) - runTestsWithRetries(t, "testTui/color", testTui_color) + t.Run("testControlR/offline/bash", func(t *testing.T) { testControlR(t, bashTester{}, "bash", Offline) }) + t.Run("testControlR/fish", func(t *testing.T) { testControlR(t, bashTester{}, "fish", Online) }) + t.Run("testTui/search", testTui_search) + t.Run("testTui/general", testTui_general) + t.Run("testTui/scroll", testTui_scroll) + t.Run("testTui/resize", testTui_resize) + t.Run("testTui/delete", testTui_delete) + t.Run("testTui/color", testTui_color) // Assert there are no leaked connections assertNoLeakedConnections(t) @@ -1635,7 +1635,7 @@ func setupTestTui(t testing.TB) (shellTester, string, *gorm.DB) { return tester, userSecret, db } -func testTui_resize(t testing.TB) { +func testTui_resize(t *testing.T) { // Setup defer testutils.BackupAndRestore(t)() tester, userSecret, _ := setupTestTui(t) @@ -1675,7 +1675,7 @@ func testTui_resize(t testing.TB) { testutils.CompareGoldens(t, out, "TestTui-LongQuery") } -func testTui_scroll(t testing.TB) { +func testTui_scroll(t *testing.T) { // Setup defer testutils.BackupAndRestore(t)() tester, userSecret, _ := setupTestTui(t) @@ -1713,7 +1713,7 @@ func testTui_scroll(t testing.TB) { assertNoLeakedConnections(t) } -func testTui_color(t testing.TB) { +func testTui_color(t *testing.T) { if runtime.GOOS == "linux" { // For some reason, this test fails on linux. Since this test isn't critical and is expected to be // flaky, we can just skip it on linux. @@ -1744,7 +1744,7 @@ func testTui_color(t testing.TB) { testutils.CompareGoldens(t, out, "TestTui-ColoredOutputWithSearch-BetaMode") } -func testTui_delete(t testing.TB) { +func testTui_delete(t *testing.T) { // Setup defer testutils.BackupAndRestore(t)() tester, userSecret, _ := setupTestTui(t) @@ -1786,7 +1786,7 @@ func testTui_delete(t testing.TB) { assertNoLeakedConnections(t) } -func testTui_search(t testing.TB) { +func testTui_search(t *testing.T) { // Setup defer testutils.BackupAndRestore(t)() tester, _, _ := setupTestTui(t) @@ -1847,7 +1847,7 @@ func testTui_search(t testing.TB) { testutils.CompareGoldens(t, out, "TestTui-InvalidSearchBecomesValid") } -func testTui_general(t testing.TB) { +func testTui_general(t *testing.T) { // Setup defer testutils.BackupAndRestore(t)() tester, _, _ := setupTestTui(t) @@ -1907,7 +1907,7 @@ func testTui_general(t testing.TB) { assertNoLeakedConnections(t) } -func testControlR(t testing.TB, tester shellTester, shellName string, onlineStatus OnlineStatus) { +func testControlR(t *testing.T, tester shellTester, shellName string, onlineStatus OnlineStatus) { // Setup defer testutils.BackupAndRestore(t)() installWithOnlineStatus(t, tester, onlineStatus) diff --git a/client/testutils.go b/client/testutils.go index e967f20..808da1c 100644 --- a/client/testutils.go +++ b/client/testutils.go @@ -114,89 +114,6 @@ func (z zshTester) ShellName() string { return "zsh" } -func runTestsWithRetries(parentT *testing.T, testName string, testFunc func(t testing.TB)) { - numRetries := 3 - if testutils.IsGithubAction() { - numRetries = 5 - } - runTestsWithExtraRetries(parentT, testName, testFunc, numRetries) -} - -func runTestsWithExtraRetries(parentT *testing.T, testName string, testFunc func(t testing.TB), numRetries int) { - for i := 1; i <= numRetries; i++ { - rt := &retryingTester{nil, i == numRetries, true, testName, numRetries} - parentT.Run(fmt.Sprintf("%s/%d", testName, i), func(t *testing.T) { - rt.T = t - testFunc(rt) - }) - if rt.succeeded { - if GLOBAL_STATSD != nil { - GLOBAL_STATSD.Incr("test_status", []string{"result:passed", "test:" + testName, "os:" + runtime.GOOS}, 1.0) - GLOBAL_STATSD.Distribution("test_retry_count", float64(i), []string{"test:" + testName, "os:" + runtime.GOOS}, 1.0) - } - break - } else { - if GLOBAL_STATSD != nil { - GLOBAL_STATSD.Incr("test_status", []string{"result:failed", "test:" + testName, "os:" + runtime.GOOS}, 1.0) - } - } - } -} - -type retryingTester struct { - *testing.T - isFinalRun bool - succeeded bool - testName string - numRetries int -} - -func (t *retryingTester) Fatalf(format string, args ...any) { - t.T.Helper() - t.succeeded = false - if t.isFinalRun { - if GLOBAL_STATSD != nil { - GLOBAL_STATSD.Incr("test_failure", []string{"test:" + t.testName, "os:" + runtime.GOOS}, 1.0) - GLOBAL_STATSD.Distribution("test_retry_count", float64(t.numRetries), []string{"test:" + t.testName, "os:" + runtime.GOOS}, 1.0) - } - t.T.Fatalf(format, args...) - } else { - testutils.TestLog(t.T, fmt.Sprintf("retryingTester: Ignoring fatalf for non-final run: %#v", fmt.Sprintf(format, args...))) - } - t.SkipNow() -} - -func (t *retryingTester) Errorf(format string, args ...any) { - t.T.Helper() - t.succeeded = false - if t.isFinalRun { - t.T.Errorf(format, args...) - } else { - testutils.TestLog(t.T, fmt.Sprintf("retryingTester: Ignoring errorf for non-final run: %#v", fmt.Sprintf(format, args...))) - } - t.SkipNow() -} - -func (t *retryingTester) FailNow() { - t.succeeded = false - if t.isFinalRun { - t.T.FailNow() - } else { - testutils.TestLog(t.T, "retryingTester: Ignoring FailNow for non-final run") - // Still terminate execution via SkipNow() since FailNow() means we should stop the current test - t.T.SkipNow() - } -} - -func (t *retryingTester) Fail() { - t.succeeded = false - if t.isFinalRun { - t.T.Fail() - } else { - testutils.TestLog(t.T, "retryingTester: Ignoring Fail for non-final run") - } -} - type OnlineStatus int64 const (