From 1618912cf63f3aa35a6cfc48af5faa80777f1922 Mon Sep 17 00:00:00 2001
From: Daniel Nephin <dnephin@hashicorp.com>
Date: Mon, 10 May 2021 13:20:45 -0400
Subject: [PATCH] Fix some test flakes

- return errors in TestAgent.Start so that the retry works correctly
- remove duplicate logging, the error is returned already
- add a missing t.Helper() to retry.Run
- properly set a.Agent to nil so that subsequent retry attempts will actually try to start
---
 agent/testagent.go          | 24 +++++++-----------------
 go.mod                      |  1 -
 sdk/testutil/retry/retry.go |  1 +
 3 files changed, 8 insertions(+), 18 deletions(-)

diff --git a/agent/testagent.go b/agent/testagent.go
index d2b5c8e11b..999ca038b6 100644
--- a/agent/testagent.go
+++ b/agent/testagent.go
@@ -16,10 +16,8 @@ import (
 	"time"
 
 	metrics "github.com/armon/go-metrics"
-	"github.com/hashicorp/errwrap"
 	"github.com/hashicorp/go-hclog"
 	uuid "github.com/hashicorp/go-uuid"
-	"github.com/stretchr/testify/require"
 
 	"github.com/hashicorp/consul/acl"
 	"github.com/hashicorp/consul/agent/config"
@@ -99,6 +97,7 @@ func NewTestAgent(t *testing.T, hcl string) *TestAgent {
 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.Fatal(err)
 		}
@@ -130,7 +129,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) (err error) {
+func (a *TestAgent) Start(t *testing.T) error {
 	t.Helper()
 	if a.Agent != nil {
 		return fmt.Errorf("TestAgent already started")
@@ -187,7 +186,9 @@ func (a *TestAgent) Start(t *testing.T) (err error) {
 		return result, err
 	}
 	bd, err := NewBaseDeps(loader, logOutput)
-	require.NoError(t, err)
+	if err != nil {
+		return fmt.Errorf("failed to create base deps: %w", err)
+	}
 
 	bd.Logger = logger
 	bd.MetricsHandler = metrics.NewInmemSink(1*time.Second, time.Minute)
@@ -215,8 +216,8 @@ func (a *TestAgent) Start(t *testing.T) (err error) {
 
 	if err := a.waitForUp(); err != nil {
 		a.Shutdown()
-		t.Logf("Error while waiting for test agent to start: %v", err)
-		return errwrap.Wrapf(name+": {{err}}", err)
+		a.Agent = nil
+		return fmt.Errorf("error waiting for test agent to start: %w", err)
 	}
 
 	a.dns = a.dnsServers[0]
@@ -280,17 +281,6 @@ func (a *TestAgent) waitForUp() error {
 // Shutdown stops the agent and removes the data directory if it is
 // managed by the test agent.
 func (a *TestAgent) Shutdown() error {
-	/* Removed this because it was breaking persistence tests where we would
-	persist a service and load it through a new agent with the same data-dir.
-	Not sure if we still need this for other things, everywhere we manually make
-	a data dir we already do 'defer os.RemoveAll()'
-	defer func() {
-		if a.DataDir != "" {
-			os.RemoveAll(a.DataDir)
-		}
-	}()*/
-
-	// already shut down
 	if a.Agent == nil {
 		return nil
 	}
diff --git a/go.mod b/go.mod
index cec935d8c3..824f105df0 100644
--- a/go.mod
+++ b/go.mod
@@ -30,7 +30,6 @@ require (
 	github.com/google/tcpproxy v0.0.0-20180808230851-dfa16c61dad2
 	github.com/hashicorp/consul/api v1.8.0
 	github.com/hashicorp/consul/sdk v0.7.0
-	github.com/hashicorp/errwrap v1.0.0
 	github.com/hashicorp/go-bexpr v0.1.2
 	github.com/hashicorp/go-checkpoint v0.5.0
 	github.com/hashicorp/go-cleanhttp v0.5.1
diff --git a/sdk/testutil/retry/retry.go b/sdk/testutil/retry/retry.go
index 0b6e1d7071..8be5c0f0e4 100644
--- a/sdk/testutil/retry/retry.go
+++ b/sdk/testutil/retry/retry.go
@@ -99,6 +99,7 @@ func decorate(s string) string {
 }
 
 func Run(t Failer, f func(r *R)) {
+	t.Helper()
 	run(DefaultFailer(), t, f)
 }