test: ensure all TestAgent constructions use a constructor (#6443)

ensure all TestAgent constructions use a constructor to get start retries + test logs going to the right place

Fixes #6435

agent/agent_endpoint_test.go

@@ -3919,15 +3919,11 @@ func TestAgent_RegisterCheck_Service(t *testing.T) {
 func TestAgent_Monitor(t *testing.T) {
 	t.Parallel()
 	logWriter := logger.NewLogWriter(512)
-	a := &TestAgent{
+	a := NewTestAgentWithFields(t, true, TestAgent{
-		Name:      t.Name(),
 		LogWriter: logWriter,
 		LogOutput: io.MultiWriter(os.Stderr, logWriter),
 		HCL:       `node_name = "invalid!"`,
-	}
+	})
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 

agent/agent_test.go

@@ -93,7 +93,11 @@ func TestAgent_ConnectClusterIDConfig(t *testing.T) {
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			a := &TestAgent{Name: tt.name, HCL: tt.hcl}
+			// This is a rare case where using a constructor for TestAgent
+			// (NewTestAgent and the likes) won't work, since we expect an error
+			// in one test case, and the constructors have built-in retry logic
+			// that runs automatically upon error.
+			a := &TestAgent{Name: tt.name, HCL: tt.hcl, LogOutput: testutil.TestWriter(t)}
 			err := a.Start()
 			if tt.wantErr {
 				if err == nil {
@@ -1218,11 +1222,7 @@ func TestAgent_RestoreServiceWithAliasCheck(t *testing.T) {
 	    enable_central_service_config = false
 		data_dir = "` + dataDir + `"
 	`
-	a := &TestAgent{Name: t.Name(), HCL: cfg, DataDir: dataDir}
+	a := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir})
-	a.LogOutput = testutil.TestWriter(t)
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer os.RemoveAll(dataDir)
 	defer a.Shutdown()
 
@@ -1306,11 +1306,7 @@ node_name = "` + a.Config.NodeName + `"
 		t.Helper()
 
 		// Reload and retain former NodeID and data directory.
-		a2 := &TestAgent{Name: t.Name(), HCL: futureHCL, DataDir: dataDir}
+		a2 := NewTestAgentWithFields(t, true, TestAgent{HCL: futureHCL, DataDir: dataDir})
-		a2.LogOutput = testutil.TestWriter(t)
-		if err := a2.Start(); err != nil {
-			t.Fatal(err)
-		}
 		defer a2.Shutdown()
 		a = nil
 
@@ -1580,7 +1576,7 @@ func TestAgent_HTTPCheck_EnableAgentTLSForChecks(t *testing.T) {
 	t.Parallel()
 
 	run := func(t *testing.T, ca string) {
-		a := &TestAgent{
+		a := NewTestAgentWithFields(t, true, TestAgent{
 			Name:   t.Name(),
 			UseTLS: true,
 			HCL: `
@@ -1591,10 +1587,7 @@ func TestAgent_HTTPCheck_EnableAgentTLSForChecks(t *testing.T) {
 				key_file = "../test/client_certs/server.key"
 				cert_file = "../test/client_certs/server.crt"
 			` + ca,
-		}
+		})
-		if err := a.Start(); err != nil {
-			t.Fatal(err)
-		}
 		defer a.Shutdown()
 
 		health := &structs.HealthCheck{
@@ -1699,10 +1692,7 @@ func TestAgent_PersistService(t *testing.T) {
 		bootstrap = false
 		data_dir = "` + dataDir + `"
 	`
-	a := &TestAgent{Name: t.Name(), HCL: cfg, DataDir: dataDir}
+	a := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir})
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer os.RemoveAll(dataDir)
 	defer a.Shutdown()
 
@@ -1767,10 +1757,7 @@ func TestAgent_PersistService(t *testing.T) {
 	a.Shutdown()
 
 	// Should load it back during later start
-	a2 := &TestAgent{Name: t.Name(), HCL: cfg, DataDir: dataDir}
+	a2 := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir})
-	if err := a2.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a2.Shutdown()
 
 	restored := a2.State.ServiceState(svc.ID)
@@ -1876,10 +1863,7 @@ func TestAgent_PurgeServiceOnDuplicate(t *testing.T) {
 		server = false
 		bootstrap = false
 	`
-	a := &TestAgent{Name: t.Name(), HCL: cfg, DataDir: dataDir}
+	a := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir})
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 	defer os.RemoveAll(dataDir)
 
@@ -1898,17 +1882,14 @@ func TestAgent_PurgeServiceOnDuplicate(t *testing.T) {
 
 	// Try bringing the agent back up with the service already
 	// existing in the config
-	a2 := &TestAgent{Name: t.Name() + "-a2", HCL: cfg + `
+	a2 := NewTestAgentWithFields(t, true, TestAgent{Name: t.Name() + "-a2", HCL: cfg + `
 		service = {
 			id = "redis"
 			name = "redis"
 			tags = ["bar"]
 			port = 9000
 		}
-	`, DataDir: dataDir}
+	`, DataDir: dataDir})
-	if err := a2.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a2.Shutdown()
 
 	file := filepath.Join(a.Config.DataDir, servicesDir, stringHash(svc1.ID))
@@ -1933,10 +1914,7 @@ func TestAgent_PersistCheck(t *testing.T) {
 		bootstrap = false
 		enable_script_checks = true
 	`
-	a := &TestAgent{Name: t.Name(), HCL: cfg, DataDir: dataDir}
+	a := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir})
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer os.RemoveAll(dataDir)
 	defer a.Shutdown()
 
@@ -2007,10 +1985,7 @@ func TestAgent_PersistCheck(t *testing.T) {
 	a.Shutdown()
 
 	// Should load it back during later start
-	a2 := &TestAgent{Name: t.Name() + "-a2", HCL: cfg, DataDir: dataDir}
+	a2 := NewTestAgentWithFields(t, true, TestAgent{Name: t.Name() + "-a2", HCL: cfg, DataDir: dataDir})
-	if err := a2.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a2.Shutdown()
 
 	result := a2.State.Check(check.CheckID)

agent/http_test.go

@@ -131,18 +131,14 @@ func TestHTTPServer_H2(t *testing.T) {
 	t.Parallel()
 
 	// Fire up an agent with TLS enabled.
-	a := &TestAgent{
+	a := NewTestAgentWithFields(t, true, TestAgent{
-		Name:   t.Name(),
 		UseTLS: true,
 		HCL: `
 			key_file = "../test/client_certs/server.key"
 			cert_file = "../test/client_certs/server.crt"
 			ca_file = "../test/client_certs/rootca.crt"
 		`,
-	}
+	})
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 
 	// Make an HTTP/2-enabled client, using the API helpers to set
@@ -458,10 +454,7 @@ func TestContentTypeIsJSON(t *testing.T) {
 func TestHTTP_wrap_obfuscateLog(t *testing.T) {
 	t.Parallel()
 	buf := new(bytes.Buffer)
-	a := &TestAgent{Name: t.Name(), LogOutput: buf}
+	a := NewTestAgentWithFields(t, true, TestAgent{LogOutput: buf})
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 
 	handler := func(resp http.ResponseWriter, req *http.Request) (interface{}, error) {
@@ -1193,12 +1186,7 @@ func TestAllowedNets(t *testing.T) {
 			nets = append(nets, in)
 		}
 
-		a := &TestAgent{
+		a := NewTestAgent(t, t.Name(), "")
-			Name: t.Name(),
-		}
-		if err := a.Start(); err != nil {
-			t.Fatal(err)
-		}
 		defer a.Shutdown()
 		testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 

agent/keyring_test.go

@@ -54,10 +54,7 @@ func TestAgent_LoadKeyrings(t *testing.T) {
 
 	// Server should auto-load LAN and WAN keyring files
 	t.Run("server with keys", func(t *testing.T) {
-		a2 := &TestAgent{Name: t.Name(), Key: key}
+		a2 := NewTestAgentWithFields(t, true, TestAgent{Key: key})
-		if err := a2.Start(); err != nil {
-			t.Fatal(err)
-		}
 		defer a2.Shutdown()
 
 		c2 := a2.consulConfig()
@@ -83,13 +80,10 @@ func TestAgent_LoadKeyrings(t *testing.T) {
 
 	// Client should auto-load only the LAN keyring file
 	t.Run("client with keys", func(t *testing.T) {
-		a3 := &TestAgent{Name: t.Name(), HCL: `
+		a3 := NewTestAgentWithFields(t, true, TestAgent{HCL: `
 			server = false
 			bootstrap = false
-		`, Key: key}
+		`, Key: key})
-		if err := a3.Start(); err != nil {
-			t.Fatal(err)
-		}
 		defer a3.Shutdown()
 
 		c3 := a3.consulConfig()
@@ -137,13 +131,10 @@ func TestAgent_InmemKeyrings(t *testing.T) {
 
 	// Server should auto-load LAN and WAN keyring
 	t.Run("server with keys", func(t *testing.T) {
-		a2 := &TestAgent{Name: t.Name(), HCL: `
+		a2 := NewTestAgent(t, t.Name(), `
-			encrypt = "` + key + `"
+			encrypt = "`+key+`"
 			disable_keyring_file = true
-		`}
+		`)
-		if err := a2.Start(); err != nil {
-			t.Fatal(err)
-		}
 		defer a2.Shutdown()
 
 		c2 := a2.consulConfig()
@@ -169,15 +160,12 @@ func TestAgent_InmemKeyrings(t *testing.T) {
 
 	// Client should auto-load only the LAN keyring
 	t.Run("client with keys", func(t *testing.T) {
-		a3 := &TestAgent{Name: t.Name(), HCL: `
+		a3 := NewTestAgent(t, t.Name(), `
-			encrypt = "` + key + `"
+			encrypt = "`+key+`"
 			server = false
 			bootstrap = false
 			disable_keyring_file = true
-		`}
+		`)
-		if err := a3.Start(); err != nil {
-			t.Fatal(err)
-		}
 		defer a3.Shutdown()
 
 		c3 := a3.consulConfig()
@@ -211,14 +199,11 @@ func TestAgent_InmemKeyrings(t *testing.T) {
 			t.Fatalf("err: %v", err)
 		}
 
-		a4 := &TestAgent{Name: t.Name(), HCL: `
+		a4 := NewTestAgent(t, t.Name(), `
-			encrypt = "` + key + `"
+			encrypt = "`+key+`"
 			disable_keyring_file = true
-			data_dir = "` + dir + `"
+			data_dir = "`+dir+`"
-		`}
+		`)
-		if err := a4.Start(); err != nil {
-			t.Fatal(err)
-		}
 		defer a4.Shutdown()
 
 		c4 := a4.consulConfig()
@@ -287,14 +272,11 @@ func TestAgentKeyring_ACL(t *testing.T) {
 	key1 := "tbLJg26ZJyJ9pK3qhc9jig=="
 	key2 := "4leC33rgtXKIVUr9Nr0snQ=="
 
-	a := &TestAgent{Name: t.Name(), HCL: TestACLConfig() + `
+	a := NewTestAgentWithFields(t, true, TestAgent{HCL: TestACLConfig() + `
 		acl_datacenter = "dc1"
 		acl_master_token = "root"
 		acl_default_policy = "deny"
-	`, Key: key1}
+	`, Key: key1})
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 
 	// List keys without access fails

agent/local/state_test.go

@@ -25,10 +25,7 @@ import (
 
 func TestAgentAntiEntropy_Services(t *testing.T) {
 	t.Parallel()
-	a := &agent.TestAgent{Name: t.Name()}
+	a := agent.NewTestAgent(t, t.Name(), "")
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
@@ -260,10 +257,7 @@ func TestAgentAntiEntropy_Services_ConnectProxy(t *testing.T) {
 	t.Parallel()
 
 	assert := assert.New(t)
-	a := &agent.TestAgent{Name: t.Name()}
+	a := agent.NewTestAgent(t, t.Name(), "")
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
@@ -420,10 +414,7 @@ func TestAgentAntiEntropy_Services_ConnectProxy(t *testing.T) {
 
 func TestAgent_ServiceWatchCh(t *testing.T) {
 	t.Parallel()
-	a := &agent.TestAgent{Name: t.Name()}
+	a := agent.NewTestAgent(t, t.Name(), "")
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
@@ -507,10 +498,7 @@ func TestAgent_ServiceWatchCh(t *testing.T) {
 
 func TestAgentAntiEntropy_EnableTagOverride(t *testing.T) {
 	t.Parallel()
-	a := &agent.TestAgent{Name: t.Name()}
+	a := agent.NewTestAgent(t, t.Name(), "")
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 	testrpc.WaitForTestAgent(t, a.RPC, "dc1")
 
@@ -770,14 +758,11 @@ var testRegisterRules = `
 
 func TestAgentAntiEntropy_Services_ACLDeny(t *testing.T) {
 	t.Parallel()
-	a := &agent.TestAgent{Name: t.Name(), HCL: `
+	a := agent.NewTestAgent(t, t.Name(), `
 		acl_datacenter = "dc1"
 		acl_master_token = "root"
 		acl_default_policy = "deny"
-		acl_enforce_version_8 = true`}
+		acl_enforce_version_8 = true`)
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 	testrpc.WaitForLeader(t, a.RPC, "dc1")
 
@@ -923,10 +908,7 @@ func TestAgentAntiEntropy_Services_ACLDeny(t *testing.T) {
 
 func TestAgentAntiEntropy_Checks(t *testing.T) {
 	t.Parallel()
-	a := &agent.TestAgent{Name: t.Name()}
+	a := agent.NewTestAgent(t, t.Name(), "")
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 
 	testrpc.WaitForTestAgent(t, a.RPC, "dc1")

agent/testagent.go

@@ -89,7 +89,25 @@ type TestAgent struct {
 // caller should call Shutdown() to stop the agent and remove temporary
 // directories.
 func NewTestAgent(t *testing.T, name string, hcl string) *TestAgent {
-	a := &TestAgent{Name: name, HCL: hcl, LogOutput: testutil.TestWriter(t)}
+	return NewTestAgentWithFields(t, true, TestAgent{Name: name, HCL: hcl})
+}
+
+// NewTestAgentWithFields takes a TestAgent struct with any number of fields set,
+// and a boolean 'start', which indicates whether or not the TestAgent should
+// be started. If no LogOutput is set, it will automatically be set to
+// testutil.TestWriter(t). Name will default to t.Name() if not specified.
+func NewTestAgentWithFields(t *testing.T, start bool, ta TestAgent) *TestAgent {
+	// copy values
+	a := ta
+	if a.Name == "" {
+		a.Name = t.Name()
+	}
+	if a.LogOutput == nil {
+		a.LogOutput = testutil.TestWriter(t)
+	}
+	if !start {
+		return nil
+	}
 
 	retry.RunWith(retry.ThreeTimes(), t, func(r *retry.R) {
 		if err := a.Start(); err != nil {
@@ -97,17 +115,7 @@ func NewTestAgent(t *testing.T, name string, hcl string) *TestAgent {
 		}
 	})
 
-	return a
+	return &a
-}
-
-// TODO: testing.T should be removed as a parameter, as it is not being used.
-func NewUnstartedAgent(t *testing.T, name string, hcl string) (*Agent, error) {
-	c := TestConfig(config.Source{Name: name, Format: "hcl", Data: hcl})
-	a, err := New(c, nil)
-	if err != nil {
-		return nil, err
-	}
-	return a, nil
 }
 
 // Start starts a test agent. It returns an error if the agent could not be started.
@@ -170,8 +178,6 @@ func (a *TestAgent) Start() (err error) {
 
 	logOutput := a.LogOutput
 	if logOutput == nil {
-		// TODO: move this out of Start() and back into NewTestAgent,
-		// and make `logOutput = testutil.TestWriter(t)`
 		logOutput = os.Stderr
 	}
 	agentLogger := log.New(logOutput, a.Name+" - ", log.LstdFlags|log.Lmicroseconds)

command/monitor/monitor_test.go

@@ -15,14 +15,10 @@ import (
 func TestMonitorCommand_exitsOnSignalBeforeLinesArrive(t *testing.T) {
 	t.Parallel()
 	logWriter := logger.NewLogWriter(512)
-	a := &agent.TestAgent{
+	a := agent.NewTestAgentWithFields(t, true, agent.TestAgent{
-		Name:      t.Name(),
 		LogWriter: logWriter,
 		LogOutput: io.MultiWriter(os.Stderr, logWriter),
-	}
+	})
-	if err := a.Start(); err != nil {
-		t.Fatal(err)
-	}
 	defer a.Shutdown()
 
 	shutdownCh := make(chan struct{})