From 001137e5e5218ac6ec1791d66025410792e0f798 Mon Sep 17 00:00:00 2001 From: Sarah Adams Date: Thu, 5 Sep 2019 10:24:36 -0700 Subject: [PATCH] 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 | 8 ++--- agent/agent_test.go | 57 +++++++++------------------------ agent/http_test.go | 20 +++--------- agent/keyring_test.go | 48 +++++++++------------------ agent/local/state_test.go | 32 ++++-------------- agent/testagent.go | 34 ++++++++++++-------- command/monitor/monitor_test.go | 8 ++--- 7 files changed, 66 insertions(+), 141 deletions(-) diff --git a/agent/agent_endpoint_test.go b/agent/agent_endpoint_test.go index 500994ffe4..cb8ceec536 100644 --- a/agent/agent_endpoint_test.go +++ b/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{ - Name: t.Name(), + a := NewTestAgentWithFields(t, true, TestAgent{ 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") diff --git a/agent/agent_test.go b/agent/agent_test.go index 3b1bee23fc..7b5d77e455 100644 --- a/agent/agent_test.go +++ b/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.LogOutput = testutil.TestWriter(t) - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir}) 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.LogOutput = testutil.TestWriter(t) - if err := a2.Start(); err != nil { - t.Fatal(err) - } + a2 := NewTestAgentWithFields(t, true, TestAgent{HCL: futureHCL, DataDir: dataDir}) 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} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir}) 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} - if err := a2.Start(); err != nil { - t.Fatal(err) - } + a2 := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir}) 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} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir}) 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} - if err := a2.Start(); err != nil { - t.Fatal(err) - } + `, DataDir: dataDir}) 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} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := NewTestAgentWithFields(t, true, TestAgent{HCL: cfg, DataDir: dataDir}) 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} - if err := a2.Start(); err != nil { - t.Fatal(err) - } + a2 := NewTestAgentWithFields(t, true, TestAgent{Name: t.Name() + "-a2", HCL: cfg, DataDir: dataDir}) defer a2.Shutdown() result := a2.State.Check(check.CheckID) diff --git a/agent/http_test.go b/agent/http_test.go index 541ff142f4..08f7fa60cd 100644 --- a/agent/http_test.go +++ b/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{ - Name: t.Name(), + a := NewTestAgentWithFields(t, true, TestAgent{ 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} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := NewTestAgentWithFields(t, true, TestAgent{LogOutput: buf}) 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{ - Name: t.Name(), - } - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := NewTestAgent(t, t.Name(), "") defer a.Shutdown() testrpc.WaitForTestAgent(t, a.RPC, "dc1") diff --git a/agent/keyring_test.go b/agent/keyring_test.go index 4647db68c8..c67db8d831 100644 --- a/agent/keyring_test.go +++ b/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} - if err := a2.Start(); err != nil { - t.Fatal(err) - } + a2 := NewTestAgentWithFields(t, true, TestAgent{Key: key}) 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} - if err := a3.Start(); err != nil { - t.Fatal(err) - } + `, Key: key}) 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: ` - encrypt = "` + key + `" + a2 := NewTestAgent(t, t.Name(), ` + 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: ` - encrypt = "` + key + `" + a3 := NewTestAgent(t, t.Name(), ` + 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: ` - encrypt = "` + key + `" + a4 := NewTestAgent(t, t.Name(), ` + encrypt = "`+key+`" disable_keyring_file = true - data_dir = "` + dir + `" - `} - if err := a4.Start(); err != nil { - t.Fatal(err) - } + data_dir = "`+dir+`" + `) 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} - if err := a.Start(); err != nil { - t.Fatal(err) - } + `, Key: key1}) defer a.Shutdown() // List keys without access fails diff --git a/agent/local/state_test.go b/agent/local/state_test.go index 97f228271c..210f802128 100644 --- a/agent/local/state_test.go +++ b/agent/local/state_test.go @@ -25,10 +25,7 @@ import ( func TestAgentAntiEntropy_Services(t *testing.T) { t.Parallel() - a := &agent.TestAgent{Name: t.Name()} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := agent.NewTestAgent(t, t.Name(), "") 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()} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := agent.NewTestAgent(t, t.Name(), "") 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()} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := agent.NewTestAgent(t, t.Name(), "") 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()} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := agent.NewTestAgent(t, t.Name(), "") 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`} - if err := a.Start(); err != nil { - t.Fatal(err) - } + acl_enforce_version_8 = true`) 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()} - if err := a.Start(); err != nil { - t.Fatal(err) - } + a := agent.NewTestAgent(t, t.Name(), "") defer a.Shutdown() testrpc.WaitForTestAgent(t, a.RPC, "dc1") diff --git a/agent/testagent.go b/agent/testagent.go index a7536b5036..5649d3378d 100644 --- a/agent/testagent.go +++ b/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 -} - -// 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 + return &a } // 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) diff --git a/command/monitor/monitor_test.go b/command/monitor/monitor_test.go index b55d7c16dc..dec199ce1e 100644 --- a/command/monitor/monitor_test.go +++ b/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{ - Name: t.Name(), + a := agent.NewTestAgentWithFields(t, true, agent.TestAgent{ LogWriter: logWriter, LogOutput: io.MultiWriter(os.Stderr, logWriter), - } - if err := a.Start(); err != nil { - t.Fatal(err) - } + }) defer a.Shutdown() shutdownCh := make(chan struct{})