diff --git a/.github/workflows/go-tests.yml b/.github/workflows/go-tests.yml index f883c9edbf8d..a69e871b61c5 100644 --- a/.github/workflows/go-tests.yml +++ b/.github/workflows/go-tests.yml @@ -156,7 +156,7 @@ jobs: - uses: actions/setup-go@fac708d6674e30b6ba41289acaab6d4b75aa0753 # v4.0.1 with: go-version-file: 'go.mod' - - run: go install github.com/hashicorp/lint-consul-retry@v1.3.0 && lint-consul-retry + - run: make lint-consul-retry lint: needs: diff --git a/Makefile b/Makefile index 34aeeba1d853..6f0cc6249cd8 100644 --- a/Makefile +++ b/Makefile @@ -20,7 +20,7 @@ PROTOC_GO_INJECT_TAG_VERSION='v1.3.0' PROTOC_GEN_GO_BINARY_VERSION='v0.1.0' DEEP_COPY_VERSION='bc3f5aa5735d8a54961580a3a24422c308c831c2' COPYWRITE_TOOL_VERSION='v0.16.4' -LINT_CONSUL_RETRY_VERSION='v1.3.0' +LINT_CONSUL_RETRY_VERSION='v1.4.0' # Go imports formatter GCI_VERSION='v0.11.2' @@ -258,6 +258,15 @@ lint/%: @echo "--> Running enumcover ($*)" @cd $* && GOWORK=off enumcover ./... +.PHONY: lint-consul-retry +lint-consul-retry: $(foreach mod,$(GO_MODULES),lint-consul-retry/$(mod)) + +.PHONY: lint-consul-retry/% +lint-consul-retry/%: lint-tools + @echo "--> Running lint-consul-retry ($*)" + @cd $* && GOWORK=off lint-consul-retry + + # check that the test-container module only imports allowlisted packages # from the root consul module. Generally we don't want to allow these imports. # In a few specific instances though it is okay to import test definitions and diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index ab9a44ff9877..51308a7945fa 100644 --- a/agent/agent_endpoint_test.go +++ b/agent/agent_endpoint_test.go @@ -1877,7 +1877,7 @@ func TestAgent_ReloadDoesNotTriggerWatch(t *testing.T) { require.NoError(t, a.updateTTLCheck(checkID, api.HealthPassing, "testing-agent-reload-001")) checkStr := func(r *retry.R, evaluator func(string) error) { - t.Helper() + r.Helper() contentsStr := "" // Wait for watch to be populated for i := 1; i < 7; i++ { @@ -1890,14 +1890,14 @@ func TestAgent_ReloadDoesNotTriggerWatch(t *testing.T) { break } time.Sleep(time.Duration(i) * time.Second) - testutil.Logger(t).Info("Watch not yet populated, retrying") + testutil.Logger(r).Info("Watch not yet populated, retrying") } if err := evaluator(contentsStr); err != nil { r.Errorf("ERROR: Test failing: %s", err) } } ensureNothingCritical := func(r *retry.R, mustContain string) { - t.Helper() + r.Helper() eval := func(contentsStr string) error { if strings.Contains(contentsStr, "critical") { return fmt.Errorf("MUST NOT contain critical:= %s", contentsStr) @@ -1915,7 +1915,7 @@ func TestAgent_ReloadDoesNotTriggerWatch(t *testing.T) { } retry.RunWith(retriesWithDelay(), t, func(r *retry.R) { - testutil.Logger(t).Info("Consul is now ready") + testutil.Logger(r).Info("Consul is now ready") // it should contain the output checkStr(r, func(contentStr string) error { if contentStr == "[]" { @@ -4340,7 +4340,7 @@ func testDefaultSidecar(svc string, port int, fns ...func(*structs.NodeService)) } // testCreateToken creates a Policy for the provided rules and a Token linked to that Policy. -func testCreateToken(t *testing.T, a *TestAgent, rules string) string { +func testCreateToken(t testutil.TestingTB, a *TestAgent, rules string) string { policyName, err := uuid.GenerateUUID() // we just need a unique name for the test and UUIDs are definitely unique require.NoError(t, err) @@ -4369,7 +4369,7 @@ func testCreateToken(t *testing.T, a *TestAgent, rules string) string { return aclResp.SecretID } -func testCreatePolicy(t *testing.T, a *TestAgent, name, rules string) string { +func testCreatePolicy(t testutil.TestingTB, a *TestAgent, name, rules string) string { args := map[string]interface{}{ "Name": name, "Rules": rules, diff --git a/agent/agent_test.go b/agent/agent_test.go index 0b17190dd611..282e397c4b9b 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -90,7 +90,7 @@ func requireServiceMissing(t *testing.T, a *TestAgent, id string) { require.Nil(t, getService(a, id), "have service %q (expected missing)", id) } -func requireCheckExists(t *testing.T, a *TestAgent, id types.CheckID) *structs.HealthCheck { +func requireCheckExists(t testutil.TestingTB, a *TestAgent, id types.CheckID) *structs.HealthCheck { t.Helper() chk := getCheck(a, id) require.NotNil(t, chk, "missing check %q", id) @@ -853,7 +853,7 @@ func TestAgent_CheckAliasRPC(t *testing.T) { assert.NoError(t, err) retry.Run(t, func(r *retry.R) { - t.Helper() + r.Helper() var args structs.NodeSpecificRequest args.Datacenter = "dc1" args.Node = "node1" @@ -1888,7 +1888,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) { // We do this so that the agent logs and the informational messages from // the test itself are interwoven properly. - logf := func(t *testing.T, a *TestAgent, format string, args ...interface{}) { + logf := func(a *TestAgent, format string, args ...interface{}) { a.logger.Info("testharness: " + fmt.Sprintf(format, args...)) } @@ -1947,12 +1947,12 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) { retryUntilCheckState := func(t *testing.T, a *TestAgent, checkID string, expectedStatus string) { t.Helper() retry.Run(t, func(r *retry.R) { - chk := requireCheckExists(t, a, types.CheckID(checkID)) + chk := requireCheckExists(r, a, types.CheckID(checkID)) if chk.Status != expectedStatus { - logf(t, a, "check=%q expected status %q but got %q", checkID, expectedStatus, chk.Status) + logf(a, "check=%q expected status %q but got %q", checkID, expectedStatus, chk.Status) r.Fatalf("check=%q expected status %q but got %q", checkID, expectedStatus, chk.Status) } - logf(t, a, "check %q has reached desired status %q", checkID, expectedStatus) + logf(a, "check %q has reached desired status %q", checkID, expectedStatus) }) } @@ -1963,7 +1963,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) { retryUntilCheckState(t, a, "service:ping", api.HealthPassing) retryUntilCheckState(t, a, "service:ping-sidecar-proxy", api.HealthPassing) - logf(t, a, "==== POWERING DOWN ORIGINAL ====") + logf(a, "==== POWERING DOWN ORIGINAL ====") require.NoError(t, a.Shutdown()) @@ -1985,7 +1985,7 @@ node_name = "` + a.Config.NodeName + `" // reregister during standup; we use an adjustable timing to try and force a race sleepDur := time.Duration(idx+1) * 500 * time.Millisecond time.Sleep(sleepDur) - logf(t, a2, "re-registering checks and services after a delay of %v", sleepDur) + logf(a2, "re-registering checks and services after a delay of %v", sleepDur) for i := 0; i < 20; i++ { // RACE RACE RACE! registerServicesAndChecks(t, a2) time.Sleep(50 * time.Millisecond) @@ -1995,7 +1995,7 @@ node_name = "` + a.Config.NodeName + `" retryUntilCheckState(t, a2, "service:ping", api.HealthPassing) - logf(t, a2, "giving the alias check a chance to notice...") + logf(a2, "giving the alias check a chance to notice...") time.Sleep(5 * time.Second) retryUntilCheckState(t, a2, "service:ping-sidecar-proxy", api.HealthPassing) diff --git a/agent/catalog_endpoint_test.go b/agent/catalog_endpoint_test.go index 19d520220427..10b1c8b887b5 100644 --- a/agent/catalog_endpoint_test.go +++ b/agent/catalog_endpoint_test.go @@ -1167,7 +1167,7 @@ func TestCatalogServiceNodes_DistanceSort(t *testing.T) { r.Fatalf("err: %v", err) } - assertIndex(t, resp) + assertIndex(r, resp) nodes = obj.(structs.ServiceNodes) if len(nodes) != 2 { r.Fatalf("bad: %v", obj) diff --git a/agent/connect/ca/provider_vault_test.go b/agent/connect/ca/provider_vault_test.go index 24b81abd5470..5b74946cada9 100644 --- a/agent/connect/ca/provider_vault_test.go +++ b/agent/connect/ca/provider_vault_test.go @@ -295,7 +295,7 @@ func TestVaultCAProvider_ConfigureFailureGoroutineLeakCheck(t *testing.T) { profile := pprof.Lookup("goroutine") sb := strings.Builder{} require.NoError(r, profile.WriteTo(&sb, 2)) - t.Log(sb.String()) + r.Log(sb.String()) require.Contains(r, sb.String(), "created by github.com/hashicorp/consul/agent/connect/ca.(*VaultProvider).Configure", "expected renewal goroutine, got none") diff --git a/agent/connect/ca/testing.go b/agent/connect/ca/testing.go index 7386deea9494..8fcf8a3c6533 100644 --- a/agent/connect/ca/testing.go +++ b/agent/connect/ca/testing.go @@ -126,7 +126,7 @@ func SkipIfVaultNotPresent(t testing.T, reqs ...vaultRequirements) { } } -func NewTestVaultServer(t testing.T) *TestVaultServer { +func NewTestVaultServer(t retry.TestingTB) *TestVaultServer { vaultBinaryName := os.Getenv("VAULT_BINARY_NAME") if vaultBinaryName == "" { vaultBinaryName = "vault" @@ -204,7 +204,7 @@ func (v *TestVaultServer) Client() *vaultapi.Client { return v.client } -func (v *TestVaultServer) WaitUntilReady(t testing.T) { +func (v *TestVaultServer) WaitUntilReady(t retry.TestingTB) { var version string retry.Run(t, func(r *retry.R) { resp, err := v.client.Sys().Health() diff --git a/agent/connect/testing_spiffe.go b/agent/connect/testing_spiffe.go index fdbff5eda67d..e50b2b3b90cd 100644 --- a/agent/connect/testing_spiffe.go +++ b/agent/connect/testing_spiffe.go @@ -3,24 +3,22 @@ package connect -import ( - "github.com/mitchellh/go-testing-interface" -) +import "github.com/hashicorp/consul/sdk/testutil" // TestSpiffeIDService returns a SPIFFE ID representing a service. -func TestSpiffeIDService(t testing.T, service string) *SpiffeIDService { +func TestSpiffeIDService(t testutil.TestingTB, service string) *SpiffeIDService { return TestSpiffeIDServiceWithHost(t, service, TestClusterID+".consul") } // TestSpiffeIDServiceWithHost returns a SPIFFE ID representing a service with // the specified trust domain. -func TestSpiffeIDServiceWithHost(t testing.T, service, host string) *SpiffeIDService { +func TestSpiffeIDServiceWithHost(t testutil.TestingTB, service, host string) *SpiffeIDService { return TestSpiffeIDServiceWithHostDC(t, service, host, "dc1") } // TestSpiffeIDServiceWithHostDC returns a SPIFFE ID representing a service with // the specified trust domain for the given datacenter. -func TestSpiffeIDServiceWithHostDC(t testing.T, service, host, datacenter string) *SpiffeIDService { +func TestSpiffeIDServiceWithHostDC(t testutil.TestingTB, service, host, datacenter string) *SpiffeIDService { return &SpiffeIDService{ Host: host, Namespace: "default", diff --git a/agent/consul/client_test.go b/agent/consul/client_test.go index 1d6fa28b526c..a3967e8c63cd 100644 --- a/agent/consul/client_test.go +++ b/agent/consul/client_test.go @@ -509,7 +509,7 @@ func newClient(t *testing.T, config *Config) *Client { return client } -func newTestResolverConfig(t *testing.T, suffix string, dc, agentType string) resolver.Config { +func newTestResolverConfig(t testutil.TestingTB, suffix string, dc, agentType string) resolver.Config { n := t.Name() s := strings.Replace(n, "/", "", -1) s = strings.Replace(s, "_", "", -1) @@ -520,7 +520,7 @@ func newTestResolverConfig(t *testing.T, suffix string, dc, agentType string) re } } -func newDefaultDeps(t *testing.T, c *Config) Deps { +func newDefaultDeps(t testutil.TestingTB, c *Config) Deps { t.Helper() logger := hclog.NewInterceptLogger(&hclog.LoggerOptions{ diff --git a/agent/consul/enterprise_server_ce_test.go b/agent/consul/enterprise_server_ce_test.go index 1a3622d5d743..c039997a03f5 100644 --- a/agent/consul/enterprise_server_ce_test.go +++ b/agent/consul/enterprise_server_ce_test.go @@ -6,12 +6,11 @@ package consul import ( - "testing" - + "github.com/hashicorp/consul/sdk/testutil" hclog "github.com/hashicorp/go-hclog" ) -func newDefaultDepsEnterprise(t *testing.T, _ hclog.Logger, _ *Config) EnterpriseDeps { +func newDefaultDepsEnterprise(t testutil.TestingTB, _ hclog.Logger, _ *Config) EnterpriseDeps { t.Helper() return EnterpriseDeps{} } diff --git a/agent/consul/server_test.go b/agent/consul/server_test.go index cc53d904e25e..620df6bfa6c4 100644 --- a/agent/consul/server_test.go +++ b/agent/consul/server_test.go @@ -121,7 +121,7 @@ func waitForLeaderEstablishment(t *testing.T, servers ...*Server) { }) } -func testServerConfig(t *testing.T) (string, *Config) { +func testServerConfig(t testutil.TestingTB) (string, *Config) { dir := testutil.TempDir(t, "consul") config := DefaultConfig() @@ -237,7 +237,7 @@ func testServerWithConfig(t *testing.T, configOpts ...func(*Config)) (string, *S var deps Deps // Retry added to avoid cases where bind addr is already in use retry.RunWith(retry.ThreeTimes(), t, func(r *retry.R) { - dir, config = testServerConfig(t) + dir, config = testServerConfig(r) for _, fn := range configOpts { fn(config) } @@ -250,8 +250,8 @@ func testServerWithConfig(t *testing.T, configOpts ...func(*Config)) (string, *S config.ACLResolverSettings.EnterpriseMeta = *config.AgentEnterpriseMeta() var err error - deps = newDefaultDeps(t, config) - srv, err = newServerWithDeps(t, config, deps) + deps = newDefaultDeps(r, config) + srv, err = newServerWithDeps(r, config, deps) if err != nil { r.Fatalf("err: %v", err) } @@ -331,7 +331,7 @@ func newServer(t *testing.T, c *Config) (*Server, error) { return newServerWithDeps(t, c, newDefaultDeps(t, c)) } -func newServerWithDeps(t *testing.T, c *Config, deps Deps) (*Server, error) { +func newServerWithDeps(t testutil.TestingTB, c *Config, deps Deps) (*Server, error) { // chain server up notification oldNotify := c.NotifyListen up := make(chan struct{}) diff --git a/agent/event_endpoint_test.go b/agent/event_endpoint_test.go index a041088c448c..f28b913cfd00 100644 --- a/agent/event_endpoint_test.go +++ b/agent/event_endpoint_test.go @@ -234,7 +234,7 @@ func TestEventList_ACLFilter(t *testing.T) { t.Run("token with access to one event type", func(t *testing.T) { retry.Run(t, func(r *retry.R) { - token := testCreateToken(t, a, ` + token := testCreateToken(r, a, ` event "foo" { policy = "read" } diff --git a/agent/grpc-external/services/peerstream/stream_test.go b/agent/grpc-external/services/peerstream/stream_test.go index ad3399b79f5a..d5a0abd6b444 100644 --- a/agent/grpc-external/services/peerstream/stream_test.go +++ b/agent/grpc-external/services/peerstream/stream_test.go @@ -690,7 +690,7 @@ func TestStreamResources_Server_StreamTracker(t *testing.T) { req := msg.GetRequest() require.NotNil(r, req) require.Equal(r, pbpeerstream.TypeURLExportedService, req.ResourceURL) - prototest.AssertDeepEqual(t, expectAck, msg) + prototest.AssertDeepEqual(r, expectAck, msg) }) expect := Status{ diff --git a/agent/health_endpoint_test.go b/agent/health_endpoint_test.go index 4c491e9cb276..86dd7be70f78 100644 --- a/agent/health_endpoint_test.go +++ b/agent/health_endpoint_test.go @@ -258,7 +258,7 @@ func TestHealthChecksInState_DistanceSort(t *testing.T) { if err != nil { r.Fatalf("err: %v", err) } - assertIndex(t, resp) + assertIndex(r, resp) nodes = obj.(structs.HealthChecks) if len(nodes) != 2 { r.Fatalf("bad: %v", nodes) @@ -613,7 +613,7 @@ func TestHealthServiceChecks_DistanceSort(t *testing.T) { if err != nil { r.Fatalf("err: %v", err) } - assertIndex(t, resp) + assertIndex(r, resp) nodes = obj.(structs.HealthChecks) if len(nodes) != 2 { r.Fatalf("bad: %v", obj) @@ -1371,7 +1371,7 @@ func TestHealthServiceNodes_DistanceSort(t *testing.T) { if err != nil { r.Fatalf("err: %v", err) } - assertIndex(t, resp) + assertIndex(r, resp) nodes = obj.(structs.CheckServiceNodes) if len(nodes) != 2 { r.Fatalf("bad: %v", obj) diff --git a/agent/http_test.go b/agent/http_test.go index f2469acc8352..607061d8681f 100644 --- a/agent/http_test.go +++ b/agent/http_test.go @@ -1628,10 +1628,8 @@ func TestAllowedNets(t *testing.T) { } // assertIndex tests that X-Consul-Index is set and non-zero -func assertIndex(t require.TestingT, resp *httptest.ResponseRecorder) { - if tt, ok := t.(*testing.T); ok { - tt.Helper() - } +func assertIndex(t testutil.TestingTB, resp *httptest.ResponseRecorder) { + t.Helper() require.NoError(t, checkIndex(resp)) } diff --git a/agent/remote_exec_test.go b/agent/remote_exec_test.go index f077de895a39..2488f6b4f2e0 100644 --- a/agent/remote_exec_test.go +++ b/agent/remote_exec_test.go @@ -15,6 +15,7 @@ import ( "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/go-uuid" @@ -358,9 +359,9 @@ func testHandleRemoteExec(t *testing.T, command string, expectedSubstring string retry.Run(t, func(r *retry.R) { event := &remoteExecEvent{ Prefix: "_rexec", - Session: makeRexecSession(t, a.Agent, ""), + Session: makeRexecSession(r, a.Agent, ""), } - defer destroySession(t, a.Agent, event.Session, "") + defer destroySession(r, a.Agent, event.Session, "") spec := &remoteExecSpec{ Command: command, @@ -429,7 +430,7 @@ func TestHandleRemoteExecFailed(t *testing.T) { testHandleRemoteExec(t, "echo failing;exit 2", "failing", "2") } -func makeRexecSession(t *testing.T, a *Agent, token string) string { +func makeRexecSession(t testutil.TestingTB, a *Agent, token string) string { args := structs.SessionRequest{ Datacenter: a.config.Datacenter, Op: structs.SessionCreate, @@ -448,7 +449,7 @@ func makeRexecSession(t *testing.T, a *Agent, token string) string { return out } -func destroySession(t *testing.T, a *Agent, session string, token string) { +func destroySession(t testutil.TestingTB, a *Agent, session string, token string) { args := structs.SessionRequest{ Datacenter: a.config.Datacenter, Op: structs.SessionDestroy, diff --git a/agent/session_endpoint_test.go b/agent/session_endpoint_test.go index ae5a492808d7..d44285f29bb7 100644 --- a/agent/session_endpoint_test.go +++ b/agent/session_endpoint_test.go @@ -15,13 +15,14 @@ import ( "github.com/hashicorp/consul/agent/structs" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/hashicorp/consul/testrpc" "github.com/hashicorp/consul/types" "github.com/stretchr/testify/require" ) -func verifySession(t *testing.T, r *retry.R, a *TestAgent, want structs.Session) { +func verifySession(t testutil.TestingTB, a *TestAgent, want structs.Session) { t.Helper() args := &structs.SessionSpecificRequest{ @@ -30,10 +31,10 @@ func verifySession(t *testing.T, r *retry.R, a *TestAgent, want structs.Session) } var out structs.IndexedSessions if err := a.RPC(context.Background(), "Session.Get", args, &out); err != nil { - r.Fatalf("err: %v", err) + t.Fatalf("err: %v", err) } if len(out.Sessions) != 1 { - r.Fatalf("bad: %#v", out.Sessions) + t.Fatalf("bad: %#v", out.Sessions) } // Make a copy so we don't modify the state store copy for an in-mem @@ -123,7 +124,7 @@ func TestSessionCreate(t *testing.T) { LockDelay: 20 * time.Second, Behavior: structs.SessionKeysRelease, } - verifySession(t, r, a, want) + verifySession(r, a, want) }) } @@ -188,7 +189,7 @@ func TestSessionCreate_NodeChecks(t *testing.T) { LockDelay: 20 * time.Second, Behavior: structs.SessionKeysRelease, } - verifySession(t, r, a, want) + verifySession(r, a, want) }) } @@ -250,7 +251,7 @@ func TestSessionCreate_Delete(t *testing.T) { LockDelay: 20 * time.Second, Behavior: structs.SessionKeysDelete, } - verifySession(t, r, a, want) + verifySession(r, a, want) }) } @@ -288,7 +289,7 @@ func TestSessionCreate_DefaultCheck(t *testing.T) { LockDelay: 20 * time.Second, Behavior: structs.SessionKeysRelease, } - verifySession(t, r, a, want) + verifySession(r, a, want) }) } @@ -329,7 +330,7 @@ func TestSessionCreate_NoCheck(t *testing.T) { LockDelay: 20 * time.Second, Behavior: structs.SessionKeysRelease, } - verifySession(t, r, a, want) + verifySession(r, a, want) }) }) @@ -359,7 +360,7 @@ func TestSessionCreate_NoCheck(t *testing.T) { LockDelay: 20 * time.Second, Behavior: structs.SessionKeysRelease, } - verifySession(t, r, a, want) + verifySession(r, a, want) }) }) @@ -391,7 +392,7 @@ func TestSessionCreate_NoCheck(t *testing.T) { LockDelay: 20 * time.Second, Behavior: structs.SessionKeysRelease, } - verifySession(t, r, a, want) + verifySession(r, a, want) }) }) } @@ -430,7 +431,7 @@ func makeTestSessionDelete(t *testing.T, srv *HTTPHandlers) string { return sessResp.ID } -func makeTestSessionTTL(t *testing.T, srv *HTTPHandlers, ttl string) string { +func makeTestSessionTTL(t testutil.TestingTB, srv *HTTPHandlers, ttl string) string { t.Helper() // Create Session with TTL body := bytes.NewBuffer(nil) @@ -488,7 +489,7 @@ func TestSessionCustomTTL(t *testing.T) { testrpc.WaitForTestAgent(t, a.RPC, "dc1") retry.Run(t, func(r *retry.R) { - id := makeTestSessionTTL(t, a.srv, ttl.String()) + id := makeTestSessionTTL(r, a.srv, ttl.String()) req, _ := http.NewRequest("GET", "/v1/session/info/"+id, nil) resp := httptest.NewRecorder() diff --git a/agent/testagent.go b/agent/testagent.go index 037bdc76da68..110c2f180435 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -117,8 +117,8 @@ func NewTestAgentWithConfigFile(t *testing.T, hcl string, configFiles []string) func StartTestAgent(t *testing.T, a TestAgent) *TestAgent { t.Helper() retry.RunWith(retry.ThreeTimes(), t, func(r *retry.R) { - t.Helper() - if err := a.Start(t); err != nil { + r.Helper() + if err := a.Start(r); err != nil { r.Fatal(err) } }) @@ -152,7 +152,7 @@ func TestConfigHCL(nodeID string) string { // Start starts a test agent. It returns an error if the agent could not be started. // If no error is returned, the caller must call Shutdown() when finished. -func (a *TestAgent) Start(t *testing.T) error { +func (a *TestAgent) Start(t testutil.TestingTB) error { t.Helper() if a.Agent != nil { return fmt.Errorf("TestAgent already started") @@ -442,10 +442,10 @@ func (r *retryShim) Name() string { // chance of port conflicts for concurrently executed test binaries. // Instead of relying on one set of ports to be sufficient we retry // starting the agent with different ports on port conflict. -func randomPortsSource(t *testing.T, useHTTPS bool) string { +func randomPortsSource(t testutil.TestingTB, useHTTPS bool) string { var ports []int retry.RunWith(retry.TwoSeconds(), t, func(r *retry.R) { - ports = freeport.GetN(&retryShim{r, t.Name()}, 7) + ports = freeport.GetN(r, 7) }) var http, https int diff --git a/api/api_test.go b/api/api_test.go index 4d5dd1fda830..d1af29fbddf7 100644 --- a/api/api_test.go +++ b/api/api_test.go @@ -119,7 +119,7 @@ func makeClientWithConfig( var server *testutil.TestServer var err error retry.RunWith(retry.ThreeTimes(), t, func(r *retry.R) { - server, err = testutil.NewTestServerConfigT(t, cb2) + server, err = testutil.NewTestServerConfigT(r, cb2) if err != nil { r.Fatalf("Failed to start server: %v", err.Error()) } diff --git a/api/lock_test.go b/api/lock_test.go index 4003a94632f8..ee59a01abcef 100644 --- a/api/lock_test.go +++ b/api/lock_test.go @@ -13,10 +13,11 @@ import ( "testing" "time" + "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" ) -func createTestLock(t *testing.T, c *Client, key string) (*Lock, *Session) { +func createTestLock(t testutil.TestingTB, c *Client, key string) (*Lock, *Session) { t.Helper() session := c.Session() @@ -106,7 +107,7 @@ func TestAPI_LockForceInvalidate(t *testing.T) { defer s.Stop() retry.Run(t, func(r *retry.R) { - lock, session := createTestLock(t, c, "test/lock") + lock, session := createTestLock(r, c, "test/lock") defer session.Destroy(lock.opts.Session, nil) // Should work diff --git a/connect/proxy/proxy_test.go b/connect/proxy/proxy_test.go index 20b4cdd55efe..73d65b82c6c2 100644 --- a/connect/proxy/proxy_test.go +++ b/connect/proxy/proxy_test.go @@ -98,7 +98,7 @@ func TestProxy_public(t *testing.T) { retry.Run(t, func(r *retry.R) { conn, err = svc.Dial(context.Background(), &connect.StaticResolver{ Addr: TestLocalAddr(ports[0]), - CertURI: agConnect.TestSpiffeIDService(t, "echo"), + CertURI: agConnect.TestSpiffeIDService(r, "echo"), }) if err != nil { r.Fatalf("err: %s", err) diff --git a/connect/service_test.go b/connect/service_test.go index 0f6ed51a8b45..88543a4145da 100644 --- a/connect/service_test.go +++ b/connect/service_test.go @@ -246,7 +246,7 @@ func TestService_HTTPClient(t *testing.T) { //require.Equal(t,"https://backend.service.consul:443", addr) return &StaticResolver{ Addr: testSvr.Addr, - CertURI: connect.TestSpiffeIDService(t, "backend"), + CertURI: connect.TestSpiffeIDService(r, "backend"), }, nil } diff --git a/internal/mesh/internal/controllers/explicitdestinations/controller_test.go b/internal/mesh/internal/controllers/explicitdestinations/controller_test.go index d23f884fd5f0..e21041e818dd 100644 --- a/internal/mesh/internal/controllers/explicitdestinations/controller_test.go +++ b/internal/mesh/internal/controllers/explicitdestinations/controller_test.go @@ -911,7 +911,7 @@ func (suite *controllerTestSuite) TestController() { expDest := &pbmesh.ComputedExplicitDestinations{ Destinations: suite.dest1.Destinations, } - dec := resourcetest.MustDecode[*pbmesh.ComputedExplicitDestinations](t, res) + dec := resourcetest.MustDecode[*pbmesh.ComputedExplicitDestinations](r, res) prototest.AssertDeepEqual(r, expDest.GetDestinations(), dec.GetData().GetDestinations()) matchingWorkloadCD := suite.client.RequireResourceExists(r, matchingWorkloadCDID) diff --git a/internal/mesh/internal/controllers/proxyconfiguration/controller_test.go b/internal/mesh/internal/controllers/proxyconfiguration/controller_test.go index fe5360756829..425a23dd987a 100644 --- a/internal/mesh/internal/controllers/proxyconfiguration/controller_test.go +++ b/internal/mesh/internal/controllers/proxyconfiguration/controller_test.go @@ -245,7 +245,7 @@ func (suite *controllerTestSuite) TestController() { PrometheusBindAddr: "0.0.0.0:9000", }, } - dec := resourcetest.MustDecode[*pbmesh.ComputedProxyConfiguration](t, res) + dec := resourcetest.MustDecode[*pbmesh.ComputedProxyConfiguration](r, res) prototest.AssertDeepEqual(r, expProxyCfg.GetDynamicConfig(), dec.GetData().GetDynamicConfig()) prototest.AssertDeepEqual(r, expProxyCfg.GetBootstrapConfig(), dec.GetData().GetBootstrapConfig()) diff --git a/internal/resource/resourcetest/testing.go b/internal/resource/resourcetest/testing.go index 1be9947226bd..6ea5ce910365 100644 --- a/internal/resource/resourcetest/testing.go +++ b/internal/resource/resourcetest/testing.go @@ -3,14 +3,10 @@ package resourcetest +import "github.com/hashicorp/consul/sdk/testutil" + // T represents the subset of testing.T methods that will be used // by the various functionality in this package type T interface { - Helper() - Log(args ...interface{}) - Logf(format string, args ...interface{}) - Errorf(format string, args ...interface{}) - Fatalf(format string, args ...interface{}) - FailNow() - Cleanup(func()) + testutil.TestingTB } diff --git a/sdk/testutil/io.go b/sdk/testutil/io.go index 7b446339728b..38722cc98159 100644 --- a/sdk/testutil/io.go +++ b/sdk/testutil/io.go @@ -16,7 +16,7 @@ var saveSnapshot = strings.ToLower(os.Getenv("TEST_SAVE_SNAPSHOT")) == "true" // If the directory cannot be created t.Fatal is called. // The directory will be removed when the test ends. Set TEST_NOCLEANUP env var // to prevent the directory from being removed. -func TempDir(t testing.TB, name string) string { +func TempDir(t TestingTB, name string) string { if t == nil { panic("argument t must be non-nil") } diff --git a/sdk/testutil/retry/doc.go b/sdk/testutil/retry/doc.go new file mode 100644 index 000000000000..4afaac062bb2 --- /dev/null +++ b/sdk/testutil/retry/doc.go @@ -0,0 +1,22 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +// Package retry provides support for repeating operations in tests. +// +// A sample retry operation looks like this: +// +// func TestX(t *testing.T) { +// retry.Run(t, func(r *retry.R) { +// if err := foo(); err != nil { +// r.Errorf("foo: %s", err) +// return +// } +// }) +// } +// +// Run uses the DefaultFailer, which is a Timer with a Timeout of 7s, +// and a Wait of 25ms. To customize, use RunWith. +// +// WARNING: unlike *testing.T, *retry.R#Fatal and FailNow *do not* +// fail the test function entirely, only the current run the retry func +package retry diff --git a/sdk/testutil/retry/interface.go b/sdk/testutil/retry/interface.go new file mode 100644 index 000000000000..78d2d948ffe1 --- /dev/null +++ b/sdk/testutil/retry/interface.go @@ -0,0 +1,35 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package retry + +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var nilInf TestingTB = nil + +// Assertion that our TestingTB can be passed to +var _ require.TestingT = nilInf +var _ assert.TestingT = nilInf + +// TestingTB is an interface that describes the implementation of the testing object. +// Using an interface that describes testing.TB instead of the actual implementation +// makes testutil usable in a wider variety of contexts (e.g. use with ginkgo : https://godoc.org/github.com/onsi/ginkgo#GinkgoT) +type TestingTB interface { + Cleanup(func()) + Error(args ...any) + Errorf(format string, args ...any) + Fail() + FailNow() + Failed() bool + Fatal(args ...any) + Fatalf(format string, args ...any) + Helper() + Log(args ...any) + Logf(format string, args ...any) + Name() string + Setenv(key, value string) + TempDir() string +} diff --git a/sdk/testutil/retry/output.go b/sdk/testutil/retry/output.go new file mode 100644 index 000000000000..6d4006f42d02 --- /dev/null +++ b/sdk/testutil/retry/output.go @@ -0,0 +1,42 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package retry + +import ( + "bytes" + "fmt" + "runtime" + "strings" +) + +func dedup(a []string) string { + if len(a) == 0 { + return "" + } + seen := map[string]struct{}{} + var b bytes.Buffer + for _, s := range a { + if _, ok := seen[s]; ok { + continue + } + seen[s] = struct{}{} + b.WriteString(s) + b.WriteRune('\n') + } + return b.String() +} + +func decorate(s string) string { + _, file, line, ok := runtime.Caller(3) + if ok { + n := strings.LastIndex(file, "/") + if n >= 0 { + file = file[n+1:] + } + } else { + file = "???" + line = 1 + } + return fmt.Sprintf("%s:%d: %s", file, line, s) +} diff --git a/sdk/testutil/retry/retry.go b/sdk/testutil/retry/retry.go index af468460d592..a8a8ac5d6fe4 100644 --- a/sdk/testutil/retry/retry.go +++ b/sdk/testutil/retry/retry.go @@ -1,263 +1,232 @@ // Copyright (c) HashiCorp, Inc. // SPDX-License-Identifier: MPL-2.0 -// Package retry provides support for repeating operations in tests. -// -// A sample retry operation looks like this: -// -// func TestX(t *testing.T) { -// retry.Run(t, func(r *retry.R) { -// if err := foo(); err != nil { -// r.Errorf("foo: %s", err) -// return -// } -// }) -// } -// -// Run uses the DefaultFailer, which is a Timer with a Timeout of 7s, -// and a Wait of 25ms. To customize, use RunWith. -// -// WARNING: unlike *testing.T, *retry.R#Fatal and FailNow *do not* -// fail the test function entirely, only the current run the retry func package retry import ( - "bytes" "fmt" - "runtime" - "strings" - "time" + "os" ) -// Failer is an interface compatible with testing.T. -type Failer interface { - Helper() +var _ TestingTB = &R{} - // Log is called for the final test output - Log(args ...interface{}) +type R struct { + wrapped TestingTB + retryer Retryer - // FailNow is called when the retrying is abandoned. - FailNow() -} + done bool + fullOutput bool + immediateCleanup bool -// R provides context for the retryer. -// -// Logs from Logf, (Error|Fatal)(f) are gathered in an internal buffer -// and printed only if the retryer fails. Printed logs are deduped and -// prefixed with source code line numbers -type R struct { - // fail is set by FailNow and (Fatal|Error)(f). It indicates the pass - // did not succeed, and should be retried - fail bool - // done is set by Stop. It indicates the entire run was a failure, - // and triggers t.FailNow() - done bool - output []string + attempts []*attempt +} - cleanups []func() +func (r *R) Cleanup(clean func()) { + if r.immediateCleanup { + a := r.getCurrentAttempt() + a.cleanups = append(a.cleanups, clean) + } else { + r.wrapped.Cleanup(clean) + } } -func (r *R) Logf(format string, args ...interface{}) { - r.log(fmt.Sprintf(format, args...)) +func (r *R) Error(args ...any) { + r.Log(args...) + r.Fail() } -func (r *R) Log(args ...interface{}) { - r.log(fmt.Sprintln(args...)) +func (r *R) Errorf(format string, args ...any) { + r.Logf(format, args...) + r.Fail() } -func (r *R) Helper() {} +func (r *R) Fail() { + r.getCurrentAttempt().failed = true +} -// Cleanup register a function to be run to cleanup resources that -// were allocated during the retry attempt. These functions are executed -// after a retry attempt. If they panic, it will not stop further retry -// attempts but will be cause for the overall test failure. -func (r *R) Cleanup(fn func()) { - r.cleanups = append(r.cleanups, fn) +func (r *R) FailNow() { + r.Fail() + panic(attemptFailed{}) } -func (r *R) runCleanup() { +func (r *R) Failed() bool { + return r.getCurrentAttempt().failed +} - // Make sure that if a cleanup function panics, - // we still run the remaining cleanup functions. - defer func() { - err := recover() - if err != nil { - r.Stop(fmt.Errorf("error when performing test cleanup: %v", err)) - } - if len(r.cleanups) > 0 { - r.runCleanup() - } - }() +func (r *R) Fatal(args ...any) { + r.Log(args...) + r.FailNow() +} - for len(r.cleanups) > 0 { - var cleanup func() - if len(r.cleanups) > 0 { - last := len(r.cleanups) - 1 - cleanup = r.cleanups[last] - r.cleanups = r.cleanups[:last] - } - if cleanup != nil { - cleanup() - } - } +func (r *R) Fatalf(format string, args ...any) { + r.Logf(format, args...) + r.FailNow() } -// runFailed is a sentinel value to indicate that the func itself -// didn't panic, rather that `FailNow` was called. -type runFailed struct{} - -// FailNow stops run execution. It is roughly equivalent to: -// -// r.Error("") -// return -// -// inside the function being run. -func (r *R) FailNow() { - r.fail = true - panic(runFailed{}) +func (r *R) Helper() { + // *testing.T will just record which functions are helpers by their addresses and + // it doesn't much matter where where we record that they are helpers + r.wrapped.Helper() } -// Fatal is equivalent to r.Logf(args) followed by r.FailNow(), i.e. the run -// function should be exited. Retries on the next run are allowed. Fatal is -// equivalent to -// -// r.Error(args) -// return -// -// inside the function being run. -func (r *R) Fatal(args ...interface{}) { - r.log(fmt.Sprint(args...)) - r.FailNow() +func (r *R) Log(args ...any) { + r.log(fmt.Sprintln(args...)) } -// Fatalf is like Fatal but allows a format string -func (r *R) Fatalf(format string, args ...interface{}) { +func (r *R) Logf(format string, args ...any) { r.log(fmt.Sprintf(format, args...)) - r.FailNow() } -// Error indicates the current run encountered an error and should be retried. -// It *does not* stop execution of the rest of the run function. -func (r *R) Error(args ...interface{}) { - r.log(fmt.Sprint(args...)) - r.fail = true +// Name will return the name of the underlying TestingT. +func (r *R) Name() string { + return r.wrapped.Name() } -// Errorf is like Error but allows a format string -func (r *R) Errorf(format string, args ...interface{}) { - r.log(fmt.Sprintf(format, args...)) - r.fail = true +// Setenv will save the current value of the specified env var, set it to the +// specified value and then restore it to the original value in a cleanup function +// once the retry attempt has finished. +func (r *R) Setenv(key, value string) { + prevValue, ok := os.LookupEnv(key) + + if err := os.Setenv(key, value); err != nil { + r.wrapped.Fatalf("cannot set environment variable: %v", err) + } + + if ok { + r.Cleanup(func() { + os.Setenv(key, prevValue) + }) + } else { + r.Cleanup(func() { + os.Unsetenv(key) + }) + } } -// If err is non-nil, equivalent to r.Fatal(err.Error()) followed by -// r.FailNow(). Otherwise a no-op. +// TempDir will use the wrapped TestingT to create a temporary directory +// that will be cleaned up when ALL RETRYING has finished. +func (r *R) TempDir() string { + return r.wrapped.TempDir() +} + +// Check will call r.Fatal(err) if err is not nil func (r *R) Check(err error) { if err != nil { - r.log(err.Error()) - r.FailNow() + r.Fatal(err) } } -func (r *R) log(s string) { - r.output = append(r.output, decorate(s)) -} - -// Stop retrying, and fail the test, logging the specified error. -// Does not stop execution, so return should be called after. func (r *R) Stop(err error) { r.log(err.Error()) r.done = true } -func decorate(s string) string { - _, file, line, ok := runtime.Caller(3) - if ok { - n := strings.LastIndex(file, "/") - if n >= 0 { - file = file[n+1:] - } - } else { - file = "???" - line = 1 - } - return fmt.Sprintf("%s:%d: %s", file, line, s) +func (r *R) failCurrentAttempt() { + r.getCurrentAttempt().failed = true } -func Run(t Failer, f func(r *R)) { - t.Helper() - run(DefaultFailer(), t, f) +func (r *R) log(s string) { + a := r.getCurrentAttempt() + a.output = append(a.output, decorate(s)) } -func RunWith(r Retryer, t Failer, f func(r *R)) { - t.Helper() - run(r, t, f) +func (r *R) getCurrentAttempt() *attempt { + if len(r.attempts) == 0 { + panic("no retry attempts have been started yet") + } + + return r.attempts[len(r.attempts)-1] } -func dedup(a []string) string { - if len(a) == 0 { - return "" - } - seen := map[string]struct{}{} - var b bytes.Buffer - for _, s := range a { - if _, ok := seen[s]; ok { - continue +// cleanupAttempt will perform all the register cleanup operations recorded +// during execution of the single round of the test function. +func (r *R) cleanupAttempt(a *attempt) { + // Make sure that if a cleanup function panics, + // we still run the remaining cleanup functions. + defer func() { + err := recover() + if err != nil { + r.Stop(fmt.Errorf("error when performing test cleanup: %v", err)) + } + if len(a.cleanups) > 0 { + r.cleanupAttempt(a) + } + }() + + for len(a.cleanups) > 0 { + var cleanup func() + if len(a.cleanups) > 0 { + last := len(a.cleanups) - 1 + cleanup = a.cleanups[last] + a.cleanups = a.cleanups[:last] + } + if cleanup != nil { + cleanup() } - seen[s] = struct{}{} - b.WriteString(s) - b.WriteRune('\n') } - return b.String() } -func run(r Retryer, t Failer, f func(r *R)) { - t.Helper() - rr := &R{} +// runAttempt will execute one round of the test function and handle cleanups and panic recovery +// of a failed attempt that should not stop retrying. +func (r *R) runAttempt(f func(r *R)) { + r.Helper() + + a := &attempt{} + r.attempts = append(r.attempts, a) - fail := func() { - t.Helper() - out := dedup(rr.output) - if out != "" { - t.Log(out) + defer r.cleanupAttempt(a) + defer func() { + if p := recover(); p != nil && p != (attemptFailed{}) { + panic(p) } - t.FailNow() - } + }() + f(r) +} + +func (r *R) run(f func(r *R)) { + r.Helper() - for r.Continue() { - // run f(rr), but if recover yields a runFailed value, we know - // FailNow was called. - func() { - defer rr.runCleanup() - defer func() { - if p := recover(); p != nil && p != (runFailed{}) { - panic(p) - } - }() - f(rr) - }() + for r.retryer.Continue() { + r.runAttempt(f) switch { - case rr.done: - fail() + case r.done: + r.recordRetryFailure() return - case !rr.fail: + case !r.Failed(): + // the current attempt did not fail so we can go ahead and return return } - rr.fail = false } - fail() + + // We cannot retry any more and no attempt has succeeded yet. + r.recordRetryFailure() } -// DefaultFailer provides default retry.Run() behavior for unit tests, namely -// 7s timeout with a wait of 25ms -func DefaultFailer() *Timer { - return &Timer{Timeout: 7 * time.Second, Wait: 25 * time.Millisecond} +func (r *R) recordRetryFailure() { + r.Helper() + output := r.getCurrentAttempt().output + if r.fullOutput { + var combined []string + for _, attempt := range r.attempts { + combined = append(combined, attempt.output...) + } + output = combined + } + + out := dedup(output) + if out != "" { + r.wrapped.Log(out) + } + r.wrapped.FailNow() } -// Retryer provides an interface for repeating operations -// until they succeed or an exit condition is met. -type Retryer interface { - // Continue returns true if the operation should be repeated, otherwise it - // returns false to indicate retrying should stop. - Continue() bool +type attempt struct { + failed bool + output []string + cleanups []func() } + +// attemptFailed is a sentinel value to indicate that the func itself +// didn't panic, rather that `FailNow` was called. +type attemptFailed struct{} diff --git a/sdk/testutil/retry/retry_test.go b/sdk/testutil/retry/retry_test.go index 77bc2d4d9f96..ecc8e5608232 100644 --- a/sdk/testutil/retry/retry_test.go +++ b/sdk/testutil/retry/retry_test.go @@ -62,7 +62,7 @@ func TestBasics(t *testing.T) { t.Run("Fatal returns from func, but does not fail test", func(t *testing.T) { i := 0 gotHere := false - ft := &fakeT{} + ft := &fakeT{T: t} Run(ft, func(r *R) { i++ t.Logf("i: %d; r: %#v", i, r) @@ -97,7 +97,7 @@ func TestBasics(t *testing.T) { func TestRunWith(t *testing.T) { t.Run("calls FailNow after exceeding retries", func(t *testing.T) { - ft := &fakeT{} + ft := &fakeT{T: t} iter := 0 RunWith(&Counter{Count: 3, Wait: time.Millisecond}, ft, func(r *R) { iter++ @@ -109,7 +109,7 @@ func TestRunWith(t *testing.T) { }) t.Run("Stop ends the retrying", func(t *testing.T) { - ft := &fakeT{} + ft := &fakeT{T: t} iter := 0 RunWith(&Counter{Count: 5, Wait: time.Millisecond}, ft, func(r *R) { iter++ @@ -128,38 +128,54 @@ func TestRunWith(t *testing.T) { }) } +func TestCleanup_Passthrough(t *testing.T) { + +} + func TestCleanup(t *testing.T) { + t.Run("basic", func(t *testing.T) { - ft := &fakeT{} + ft := &fakeT{T: t} cleanupsExecuted := 0 - RunWith(&Counter{Count: 2, Wait: time.Millisecond}, ft, func(r *R) { - r.Cleanup(func() { - cleanupsExecuted += 1 - }) - }) + + Run( + ft, + func(r *R) { + r.Cleanup(func() { + cleanupsExecuted += 1 + }) + }, + WithImmediateCleanup(), + WithRetryer(&Counter{Count: 2, Wait: time.Millisecond}), + ) require.Equal(t, 0, ft.fails) require.Equal(t, 1, cleanupsExecuted) }) t.Run("cleanup-panic-recovery", func(t *testing.T) { - ft := &fakeT{} + ft := &fakeT{T: t} cleanupsExecuted := 0 - RunWith(&Counter{Count: 2, Wait: time.Millisecond}, ft, func(r *R) { - r.Cleanup(func() { - cleanupsExecuted += 1 - }) + Run( + ft, + func(r *R) { + r.Cleanup(func() { + cleanupsExecuted += 1 + }) - r.Cleanup(func() { - cleanupsExecuted += 1 - panic(fmt.Errorf("fake test error")) - }) + r.Cleanup(func() { + cleanupsExecuted += 1 + panic(fmt.Errorf("fake test error")) + }) - r.Cleanup(func() { - cleanupsExecuted += 1 - }) + r.Cleanup(func() { + cleanupsExecuted += 1 + }) - // test is successful but should fail due to the cleanup panicing - }) + // test is successful but should fail due to the cleanup panicing + }, + WithRetryer(&Counter{Count: 2, Wait: time.Millisecond}), + WithImmediateCleanup(), + ) require.Equal(t, 3, cleanupsExecuted) require.Equal(t, 1, ft.fails) @@ -167,33 +183,71 @@ func TestCleanup(t *testing.T) { }) t.Run("cleanup-per-retry", func(t *testing.T) { - ft := &fakeT{} + ft := &fakeT{T: t} iter := 0 cleanupsExecuted := 0 - RunWith(&Counter{Count: 3, Wait: time.Millisecond}, ft, func(r *R) { - if cleanupsExecuted != iter { - r.Stop(fmt.Errorf("cleanups not executed between retries")) - return - } - iter += 1 + Run( + ft, + func(r *R) { + if cleanupsExecuted != iter { + r.Stop(fmt.Errorf("cleanups not executed between retries")) + return + } + iter += 1 - r.Cleanup(func() { - cleanupsExecuted += 1 - }) + r.Cleanup(func() { + cleanupsExecuted += 1 + }) - r.FailNow() - }) + r.FailNow() + }, + WithRetryer(&Counter{Count: 3, Wait: time.Millisecond}), + WithImmediateCleanup(), + ) require.Equal(t, 3, cleanupsExecuted) // ensure that r.Stop hadn't been called. If it was then we would // have log output require.Len(t, ft.out, 0) }) + + t.Run("passthrough-to-t", func(t *testing.T) { + cleanupsExecuted := 0 + + require.True(t, t.Run("internal", func(t *testing.T) { + iter := 0 + Run( + t, + func(r *R) { + iter++ + + r.Cleanup(func() { + cleanupsExecuted += 1 + }) + + // fail all but the last one to ensure the right number of cleanups + // are eventually executed + if iter < 3 { + r.FailNow() + } + }, + WithRetryer(&Counter{Count: 3, Wait: time.Millisecond}), + ) + + // at this point nothing should be cleaned up + require.Equal(t, 0, cleanupsExecuted) + })) + + // now since the subtest finished the test cleanup funcs + // should have been executed. + require.Equal(t, 3, cleanupsExecuted) + }) } type fakeT struct { fails int out []string + *testing.T } func (f *fakeT) Helper() {} @@ -206,4 +260,4 @@ func (f *fakeT) FailNow() { f.fails++ } -var _ Failer = &fakeT{} +var _ TestingTB = &fakeT{} diff --git a/sdk/testutil/retry/retryer.go b/sdk/testutil/retry/retryer.go new file mode 100644 index 000000000000..45f41988f21e --- /dev/null +++ b/sdk/testutil/retry/retryer.go @@ -0,0 +1,36 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package retry + +import "time" + +// Retryer provides an interface for repeating operations +// until they succeed or an exit condition is met. +type Retryer interface { + // Continue returns true if the operation should be repeated, otherwise it + // returns false to indicate retrying should stop. + Continue() bool +} + +// DefaultRetryer provides default retry.Run() behavior for unit tests, namely +// 7s timeout with a wait of 25ms +func DefaultRetryer() Retryer { + return &Timer{Timeout: 7 * time.Second, Wait: 25 * time.Millisecond} +} + +// ThirtySeconds repeats an operation for thirty seconds and waits 500ms in between. +// Best for known slower operations like waiting on eventually consistent state. +func ThirtySeconds() *Timer { + return &Timer{Timeout: 30 * time.Second, Wait: 500 * time.Millisecond} +} + +// TwoSeconds repeats an operation for two seconds and waits 25ms in between. +func TwoSeconds() *Timer { + return &Timer{Timeout: 2 * time.Second, Wait: 25 * time.Millisecond} +} + +// ThreeTimes repeats an operation three times and waits 25ms in between. +func ThreeTimes() *Counter { + return &Counter{Count: 3, Wait: 25 * time.Millisecond} +} diff --git a/sdk/testutil/retry/run.go b/sdk/testutil/retry/run.go new file mode 100644 index 000000000000..51dad9f4e4d3 --- /dev/null +++ b/sdk/testutil/retry/run.go @@ -0,0 +1,48 @@ +// Copyright (c) HashiCorp, Inc. +// SPDX-License-Identifier: MPL-2.0 + +package retry + +type Option func(r *R) + +func WithRetryer(retryer Retryer) Option { + return func(r *R) { + r.retryer = retryer + } +} + +func WithFullOutput() Option { + return func(r *R) { + r.fullOutput = true + } +} + +// WithImmediateCleanup will cause all cleanup operations added +// by calling the Cleanup method on *R to be performed after +// the retry attempt completes (regardless of pass/fail status) +// Use this only if all resources created during the retry loop should +// not persist after the retry has finished. +func WithImmediateCleanup() Option { + return func(r *R) { + r.immediateCleanup = true + } +} + +func Run(t TestingTB, f func(r *R), opts ...Option) { + t.Helper() + r := &R{ + wrapped: t, + retryer: DefaultRetryer(), + } + + for _, opt := range opts { + opt(r) + } + + r.run(f) +} + +func RunWith(r Retryer, t TestingTB, f func(r *R)) { + t.Helper() + Run(t, f, WithRetryer(r)) +} diff --git a/sdk/testutil/retry/timer.go b/sdk/testutil/retry/timer.go index be4f5e92f407..6c65d38ba0e6 100644 --- a/sdk/testutil/retry/timer.go +++ b/sdk/testutil/retry/timer.go @@ -5,22 +5,6 @@ package retry import "time" -// ThirtySeconds repeats an operation for thirty seconds and waits 500ms in between. -// Best for known slower operations like waiting on eventually consistent state. -func ThirtySeconds() *Timer { - return &Timer{Timeout: 30 * time.Second, Wait: 500 * time.Millisecond} -} - -// TwoSeconds repeats an operation for two seconds and waits 25ms in between. -func TwoSeconds() *Timer { - return &Timer{Timeout: 2 * time.Second, Wait: 25 * time.Millisecond} -} - -// ThreeTimes repeats an operation three times and waits 25ms in between. -func ThreeTimes() *Counter { - return &Counter{Count: 3, Wait: 25 * time.Millisecond} -} - // Timer repeats an operation for a given amount // of time and waits between subsequent operations. type Timer struct { diff --git a/sdk/testutil/types.go b/sdk/testutil/types.go index e5721d3c9647..33cc3002997d 100644 --- a/sdk/testutil/types.go +++ b/sdk/testutil/types.go @@ -3,14 +3,33 @@ package testutil +import ( + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var nilInf TestingTB = nil + +// Assertion that our TestingTB can be passed to +var _ require.TestingT = nilInf +var _ assert.TestingT = nilInf + // TestingTB is an interface that describes the implementation of the testing object. // Using an interface that describes testing.TB instead of the actual implementation // makes testutil usable in a wider variety of contexts (e.g. use with ginkgo : https://godoc.org/github.com/onsi/ginkgo#GinkgoT) type TestingTB interface { Cleanup(func()) + Error(args ...any) + Errorf(format string, args ...any) + Fail() + FailNow() Failed() bool - Logf(format string, args ...interface{}) - Name() string - Fatalf(fmt string, args ...interface{}) + Fatal(args ...any) + Fatalf(format string, args ...any) Helper() + Log(args ...any) + Logf(format string, args ...any) + Name() string + Setenv(key, value string) + TempDir() string } diff --git a/test-integ/tenancy/client.go b/test-integ/tenancy/client.go index 910e7f76045e..a3152b762587 100644 --- a/test-integ/tenancy/client.go +++ b/test-integ/tenancy/client.go @@ -23,18 +23,6 @@ import ( // // TODO: Move to a general package if used more widely. -// T represents the subset of testing.T methods that will be used -// by the various functionality in this package -type T interface { - Helper() - Log(args ...interface{}) - Logf(format string, args ...interface{}) - Errorf(format string, args ...interface{}) - Fatalf(format string, args ...interface{}) - FailNow() - Cleanup(func()) -} - type ClientOption func(*Client) func WithACLToken(token string) ClientOption { @@ -76,13 +64,13 @@ func (client *Client) SetRetryerConfig(timeout time.Duration, wait time.Duration client.wait = wait } -func (client *Client) retry(t T, fn func(r *retry.R)) { +func (client *Client) retry(t testutil.TestingTB, fn func(r *retry.R)) { t.Helper() retryer := &retry.Timer{Timeout: client.timeout, Wait: client.wait} retry.RunWith(retryer, t, fn) } -func (client *Client) Context(t T) context.Context { +func (client *Client) Context(t testutil.TestingTB) context.Context { ctx := testutil.TestContext(t) if client.token != "" { @@ -95,7 +83,7 @@ func (client *Client) Context(t T) context.Context { return ctx } -func (client *Client) RequireResourceNotFound(t T, id *pbresource.ID) { +func (client *Client) RequireResourceNotFound(t testutil.TestingTB, id *pbresource.ID) { t.Helper() rsp, err := client.Read(client.Context(t), &pbresource.ReadRequest{Id: id}) @@ -104,7 +92,7 @@ func (client *Client) RequireResourceNotFound(t T, id *pbresource.ID) { require.Nil(t, rsp) } -func (client *Client) RequireResourceExists(t T, id *pbresource.ID) *pbresource.Resource { +func (client *Client) RequireResourceExists(t testutil.TestingTB, id *pbresource.ID) *pbresource.Resource { t.Helper() rsp, err := client.Read(client.Context(t), &pbresource.ReadRequest{Id: id}) @@ -117,7 +105,7 @@ func ToGVK(resourceType *pbresource.Type) string { return fmt.Sprintf("%s.%s.%s", resourceType.Group, resourceType.GroupVersion, resourceType.Kind) } -func (client *Client) WaitForResourceExists(t T, id *pbresource.ID) *pbresource.Resource { +func (client *Client) WaitForResourceExists(t testutil.TestingTB, id *pbresource.ID) *pbresource.Resource { t.Helper() var res *pbresource.Resource @@ -128,7 +116,7 @@ func (client *Client) WaitForResourceExists(t T, id *pbresource.ID) *pbresource. return res } -func (client *Client) WaitForDeletion(t T, id *pbresource.ID) { +func (client *Client) WaitForDeletion(t testutil.TestingTB, id *pbresource.ID) { t.Helper() client.retry(t, func(r *retry.R) { @@ -139,12 +127,12 @@ func (client *Client) WaitForDeletion(t T, id *pbresource.ID) { // MustDelete will delete a resource by its id, retrying if necessary and fail the test // if it cannot delete it within the timeout. The clients request delay settings are // taken into account with this operation. -func (client *Client) MustDelete(t T, id *pbresource.ID) { +func (client *Client) MustDelete(t testutil.TestingTB, id *pbresource.ID) { t.Helper() client.retryDelete(t, id) } -func (client *Client) retryDelete(t T, id *pbresource.ID) { +func (client *Client) retryDelete(t testutil.TestingTB, id *pbresource.ID) { t.Helper() ctx := client.Context(t) diff --git a/test-integ/topoutil/asserter.go b/test-integ/topoutil/asserter.go index 1cc7243dc3ca..091ba2379705 100644 --- a/test-integ/topoutil/asserter.go +++ b/test-integ/topoutil/asserter.go @@ -14,6 +14,7 @@ import ( "github.com/hashicorp/consul/api" "github.com/hashicorp/consul/proto-public/pbresource" + "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" libassert "github.com/hashicorp/consul/test/integration/consul-container/libs/assert" "github.com/hashicorp/consul/test/integration/consul-container/libs/utils" @@ -52,13 +53,13 @@ func NewAsserter(sp SprawlLite) *Asserter { } } -func (a *Asserter) mustGetHTTPClient(t *testing.T, cluster string) *http.Client { +func (a *Asserter) mustGetHTTPClient(t testutil.TestingTB, cluster string) *http.Client { client, err := a.httpClientFor(cluster) require.NoError(t, err) return client } -func (a *Asserter) mustGetAPIClient(t *testing.T, cluster string) *api.Client { +func (a *Asserter) mustGetAPIClient(t testutil.TestingTB, cluster string) *api.Client { clu := a.sp.Topology().Clusters[cluster] cl, err := a.sp.APIClientForCluster(clu.Name, "") require.NoError(t, err) @@ -208,7 +209,7 @@ type testingT interface { // // We treat 400, 503, and 504s as retryable errors func (a *Asserter) fortioFetch2Destination( - t testingT, + t testutil.TestingTB, client *http.Client, addr string, dest *topology.Destination, diff --git a/test/integration/consul-container/libs/assert/grpc.go b/test/integration/consul-container/libs/assert/grpc.go index a41ef65af620..101d1c14109e 100644 --- a/test/integration/consul-container/libs/assert/grpc.go +++ b/test/integration/consul-container/libs/assert/grpc.go @@ -26,7 +26,7 @@ func GRPCPing(t *testing.T, addr string) { var msg *fgrpc.PingMessage retries := 0 retry.RunWith(&retry.Timer{Timeout: time.Minute, Wait: 25 * time.Millisecond}, t, func(r *retry.R) { - t.Logf("making grpc call to %s", addr) + r.Logf("making grpc call to %s", addr) retries += 1 msg, err = pingCl.Ping(context.Background(), &fgrpc.PingMessage{ // use addr as payload so we have something variable to check against diff --git a/test/integration/consul-container/libs/assert/service.go b/test/integration/consul-container/libs/assert/service.go index 7434a1d5e36f..726731dcc0ed 100644 --- a/test/integration/consul-container/libs/assert/service.go +++ b/test/integration/consul-container/libs/assert/service.go @@ -221,7 +221,7 @@ func doHTTPServiceEchoesWithClient( } retry.RunWith(failer(), t, func(r *retry.R) { - t.Logf("making call to %s", url) + r.Logf("making call to %s", url) reader := strings.NewReader(phrase) req, err := http.NewRequest("POST", url, reader) @@ -242,7 +242,7 @@ func doHTTPServiceEchoesWithClient( defer res.Body.Close() statusCode := res.StatusCode - t.Logf("...got response code %d", statusCode) + r.Logf("...got response code %d", statusCode) require.Equal(r, 200, statusCode) body, err := io.ReadAll(res.Body) @@ -342,7 +342,7 @@ func WaitForFortioNameWithClient(t *testing.T, r retry.Retryer, urlbase string, // It retries with timeout defaultHTTPTimeout and wait defaultHTTPWait. // // client must be a custom http.Client -func FortioNameWithClient(t retry.Failer, urlbase string, name string, reqHost string, client *http.Client) (string, error) { +func FortioNameWithClient(t retry.TestingTB, urlbase string, name string, reqHost string, client *http.Client) (string, error) { t.Helper() var fortioNameRE = regexp.MustCompile("\nFORTIO_NAME=(.+)\n") var body []byte diff --git a/test/integration/consul-container/test/envoy_extensions/ext_authz_test.go b/test/integration/consul-container/test/envoy_extensions/ext_authz_test.go index cf00105345fa..ea2e874162c4 100644 --- a/test/integration/consul-container/test/envoy_extensions/ext_authz_test.go +++ b/test/integration/consul-container/test/envoy_extensions/ext_authz_test.go @@ -15,6 +15,7 @@ import ( "github.com/testcontainers/testcontainers-go" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" libassert "github.com/hashicorp/consul/test/integration/consul-container/libs/assert" libcluster "github.com/hashicorp/consul/test/integration/consul-container/libs/cluster" @@ -87,8 +88,8 @@ func TestExtAuthzLocal(t *testing.T) { // Make requests to the static-server. We expect that all requests are rejected with 403 Forbidden // unless they are to the /allow path. baseURL := fmt.Sprintf("http://localhost:%d", port) - doRequest(t, baseURL, http.StatusForbidden) - doRequest(t, baseURL+"/allow", http.StatusOK) + retryRequest(t, baseURL, http.StatusForbidden) + retryRequest(t, baseURL+"/allow", http.StatusOK) } func createServices(t *testing.T, cluster *libcluster.Cluster) libservice.Service { @@ -161,10 +162,16 @@ func createLocalAuthzService(t *testing.T, cluster *libcluster.Cluster) { } } -func doRequest(t *testing.T, url string, expStatus int) { +func retryRequest(t *testing.T, url string, expStatus int) { + t.Helper() retry.RunWith(&retry.Timer{Timeout: 5 * time.Second, Wait: time.Second}, t, func(r *retry.R) { - resp, err := cleanhttp.DefaultClient().Get(url) - require.NoError(r, err) - require.Equal(r, expStatus, resp.StatusCode) + doRequest(r, url, expStatus) }) } + +func doRequest(t testutil.TestingTB, url string, expStatus int) { + t.Helper() + resp, err := cleanhttp.DefaultClient().Get(url) + require.NoError(t, err) + require.Equal(t, expStatus, resp.StatusCode) +} diff --git a/test/integration/consul-container/test/envoy_extensions/otel_access_logging_test.go b/test/integration/consul-container/test/envoy_extensions/otel_access_logging_test.go index 87bb7b4d6165..e684bdbcc832 100644 --- a/test/integration/consul-container/test/envoy_extensions/otel_access_logging_test.go +++ b/test/integration/consul-container/test/envoy_extensions/otel_access_logging_test.go @@ -40,8 +40,6 @@ import ( // - Make sure a call to the client sidecar local bind port results in Envoy access logs being sent to the // otel-collector. func TestOTELAccessLogging(t *testing.T) { - t.Parallel() - cluster, _, _ := topology.NewCluster(t, &topology.ClusterConfig{ NumServers: 1, NumClients: 1, @@ -87,8 +85,9 @@ func TestOTELAccessLogging(t *testing.T) { // Make requests from the static-client to the static-server and look for the access logs // to show up in the `otel-collector` container logs. - retry.RunWith(&retry.Timer{Timeout: 60 * time.Second, Wait: time.Second}, t, func(r *retry.R) { - doRequest(t, fmt.Sprintf("http://localhost:%d", port), http.StatusOK) + retry.Run(t, func(r *retry.R) { + doRequest(r, fmt.Sprintf("http://localhost:%d", port), http.StatusOK) + reader, err := launchInfo.Container.Logs(context.Background()) require.NoError(r, err) log, err := io.ReadAll(reader) @@ -96,7 +95,10 @@ func TestOTELAccessLogging(t *testing.T) { require.Contains(r, string(log), `log_name: Str(otel-integration-test)`) require.Contains(r, string(log), `cluster_name: Str(static-server)`) require.Contains(r, string(log), `node_name: Str(static-server-sidecar-proxy)`) - }) + }, + retry.WithFullOutput(), + retry.WithRetryer(&retry.Timer{Timeout: 60 * time.Second, Wait: time.Second}), + ) } func createLocalOTELService(t *testing.T, cluster *libcluster.Cluster) *libcluster.LaunchInfo { diff --git a/test/integration/consul-container/test/gateways/terminating_gateway_test.go b/test/integration/consul-container/test/gateways/terminating_gateway_test.go index ef8f95ab8292..3ac130a546cd 100644 --- a/test/integration/consul-container/test/gateways/terminating_gateway_test.go +++ b/test/integration/consul-container/test/gateways/terminating_gateway_test.go @@ -173,12 +173,12 @@ func assertHTTPRequestToServiceAddress(t *testing.T, client *libservice.ConnectC upstreamURL := fmt.Sprintf("http://localhost:%d/debug?env=dump", port) retry.RunWith(requestRetryTimer, t, func(r *retry.R) { out, err := client.Exec(context.Background(), []string{"curl", "-s", upstreamURL}) - t.Logf("curl request to upstream service address: url=%s\nerr = %v\nout = %s", upstreamURL, err, out) + r.Logf("curl request to upstream service address: url=%s\nerr = %v\nout = %s", upstreamURL, err, out) if expSuccess { require.NoError(r, err) require.Contains(r, out, fmt.Sprintf("FORTIO_NAME=%s", serviceName)) - t.Logf("successfuly messaged %s", serviceName) + r.Logf("successfuly messaged %s", serviceName) } else { require.Error(r, err) require.Contains(r, err.Error(), "exit code 52") diff --git a/test/integration/consul-container/test/ratelimit/ratelimit_test.go b/test/integration/consul-container/test/ratelimit/ratelimit_test.go index 89293da7f8ee..e0b85a17d186 100644 --- a/test/integration/consul-container/test/ratelimit/ratelimit_test.go +++ b/test/integration/consul-container/test/ratelimit/ratelimit_test.go @@ -11,6 +11,7 @@ import ( "time" "github.com/hashicorp/consul/api" + "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" "github.com/stretchr/testify/require" @@ -282,7 +283,7 @@ func setupClusterAndClient(t *testing.T, config *libtopology.ClusterConfig, isSe return cluster, client } -func checkForMetric(t require.TestingT, cluster *libcluster.Cluster, operationName string, expectedLimitType string, expectedMode string, expectMetric bool) { +func checkForMetric(t testutil.TestingTB, cluster *libcluster.Cluster, operationName string, expectedLimitType string, expectedMode string, expectMetric bool) { // validate metrics server, err := cluster.GetClient(nil, true) require.NoError(t, err) @@ -320,7 +321,7 @@ func checkForMetric(t require.TestingT, cluster *libcluster.Cluster, operationNa } } -func checkLogsForMessage(t require.TestingT, logs []string, msg string, operationName string, logType string, logShouldExist bool) { +func checkLogsForMessage(t testutil.TestingTB, logs []string, msg string, operationName string, logType string, logShouldExist bool) { if logShouldExist { found := false for _, log := range logs { diff --git a/test/integration/consul-container/test/tproxy/tproxy_test.go b/test/integration/consul-container/test/tproxy/tproxy_test.go index 15eae0c21090..1f962aeba277 100644 --- a/test/integration/consul-container/test/tproxy/tproxy_test.go +++ b/test/integration/consul-container/test/tproxy/tproxy_test.go @@ -136,7 +136,7 @@ func assertHTTPRequestToVirtualAddress(t *testing.T, clientService libservice.Se `, virtualHostname), }, ) - t.Logf("curl request to upstream virtual address\nerr = %v\nout = %s", err, out) + r.Logf("curl request to upstream virtual address\nerr = %v\nout = %s", err, out) require.NoError(r, err) require.Regexp(r, `Virtual IP: 240.0.0.\d+`, out) require.Contains(r, out, fmt.Sprintf("FORTIO_NAME=%s", serverName)) @@ -155,7 +155,7 @@ func assertHTTPRequestToServiceAddress(t *testing.T, client, server libcluster.A upstreamURL := fmt.Sprintf("http://%s:8080/debug?env=dump", server.GetIP()) retry.RunWith(requestRetryTimer, t, func(r *retry.R) { out, err := client.Exec(context.Background(), []string{"curl", "-s", upstreamURL}) - t.Logf("curl request to upstream service address: url=%s\nerr = %v\nout = %s", upstreamURL, err, out) + r.Logf("curl request to upstream service address: url=%s\nerr = %v\nout = %s", upstreamURL, err, out) if expSuccess { require.NoError(r, err) diff --git a/testrpc/wait.go b/testrpc/wait.go index ca315d28539e..7c845cbf6810 100644 --- a/testrpc/wait.go +++ b/testrpc/wait.go @@ -108,6 +108,7 @@ func WaitForTestAgent(t *testing.T, rpc rpcFn, dc string, options ...waitOption) var checks structs.IndexedHealthChecks retry.Run(t, func(r *retry.R) { + r.Helper() dcReq := &structs.DCSpecificRequest{ Datacenter: dc, QueryOptions: structs.QueryOptions{Token: flat.Token},