diff --git a/agent/agent_test.go b/agent/agent_test.go index 06f82d6c8a..3b1bee23fc 100644 --- a/agent/agent_test.go +++ b/agent/agent_test.go @@ -66,7 +66,7 @@ func TestAgent_ConnectClusterIDConfig(t *testing.T) { name string hcl string wantClusterID string - wantPanic bool + wantErr bool }{ { name: "default TestAgent has fixed cluster id", @@ -87,30 +87,27 @@ func TestAgent_ConnectClusterIDConfig(t *testing.T) { } }`, wantClusterID: "", - wantPanic: true, + wantErr: true, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - // Indirection to support panic recovery cleanly - testFn := func() { - a := &TestAgent{Name: "test", HCL: tt.hcl} - a.ExpectConfigError = tt.wantPanic - if err := a.Start(); err != nil { - t.Fatal(err) + a := &TestAgent{Name: tt.name, HCL: tt.hcl} + err := a.Start() + if tt.wantErr { + if err == nil { + t.Fatal("expected error, got nil") } - defer a.Shutdown() - - cfg := a.consulConfig() - assert.Equal(t, tt.wantClusterID, cfg.CAConfig.ClusterID) + return // don't run the rest of the test } - - if tt.wantPanic { - require.Panics(t, testFn) - } else { - testFn() + if !tt.wantErr && err != nil { + t.Fatal(err) } + defer a.Shutdown() + + cfg := a.consulConfig() + assert.Equal(t, tt.wantClusterID, cfg.CAConfig.ClusterID) }) } } diff --git a/agent/testagent.go b/agent/testagent.go index fbbcb2c712..a7536b5036 100644 --- a/agent/testagent.go +++ b/agent/testagent.go @@ -45,12 +45,6 @@ type TestAgent struct { HCL string - // ExpectConfigError can be set to prevent the agent retrying Start on errors - // and eventually blowing up with runtime.Goexit. This enables tests to assert - // that some specific bit of config actually does prevent startup entirely in - // a reasonable way without reproducing a lot of the boilerplate here. - ExpectConfigError bool - // Config is the agent configuration. If Config is nil then // TestConfig() is used. If Config.DataDir is set then it is // the callers responsibility to clean up the data directory. @@ -199,19 +193,6 @@ func (a *TestAgent) Start() (err error) { agent.ShutdownAgent() agent.ShutdownEndpoints() - if a.ExpectConfigError { - // Panic the error since this can be caught if needed. Pretty gross way to - // detect errors but enough for now and this is a tiny edge case that I'd - // otherwise not have a way to test at all... - // - // TODO(sadams): This can be refactored away by returning an - // error here instead of panicing, removing the `ExpectConfigError` - // field from `TestAgent`, and having the test that uses this - // (TestAgent_ConnectClusterIDConfig) check for an error instead of - // catching a panic. - panic(err) - } - return fmt.Errorf("%s %s Error starting agent: %s", id, a.Name, err) } @@ -294,6 +275,11 @@ func (a *TestAgent) Shutdown() error { } }()*/ + // already shut down + if a.Agent == nil { + return nil + } + // shutdown agent before endpoints defer a.Agent.ShutdownEndpoints() return a.Agent.ShutdownAgent()