From 71d6a2bf4b446d5697c29b7c81be10b7e9568cf1 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 10 May 2021 13:20:45 -0400 Subject: [PATCH 1/3] 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 d2b5c8e11..999ca038b 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 cec935d8c..824f105df 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 0b6e1d707..8be5c0f0e 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) } From fc4100ab4d0f0625920fe82df2a51155ebcd5748 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 10 May 2021 13:25:18 -0400 Subject: [PATCH 2/3] ci: update gotestsum To pickup this fix: https://github.com/gotestyourself/gotestsum/commit/e91cbd912ad379aebd1a4bec7643cc9c078869a1 --- .circleci/config.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 48e3fe32c..0e690f86f 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -36,7 +36,7 @@ steps: install-gotestsum: &install-gotestsum name: install gotestsum environment: - GOTESTSUM_RELEASE: 0.6.0 + GOTESTSUM_RELEASE: 1.6.4 command: | url=https://github.com/gotestyourself/gotestsum/releases/download curl -sSL "${url}/v${GOTESTSUM_RELEASE}/gotestsum_${GOTESTSUM_RELEASE}_linux_amd64.tar.gz" | \ From 3dd951ab1e6372bf45ba1c6b990df2d466e08906 Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 10 May 2021 13:30:10 -0400 Subject: [PATCH 3/3] testing: don't run t.Parallel in a goroutine TestACLEndpoint_Login_with_TokenLocality was reguardly being reported as failed even though it was not failing. I took another look and I suspect it is because t.Parllel was being called in a goroutine. This would lead to strange behaviour which apparently confused the 'go test' runner. --- agent/consul/acl_endpoint_test.go | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/agent/consul/acl_endpoint_test.go b/agent/consul/acl_endpoint_test.go index cf536b0c4..0031ce621 100644 --- a/agent/consul/acl_endpoint_test.go +++ b/agent/consul/acl_endpoint_test.go @@ -10,6 +10,11 @@ import ( "testing" "time" + uuid "github.com/hashicorp/go-uuid" + msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" + "github.com/stretchr/testify/require" + "gopkg.in/square/go-jose.v2/jwt" + "github.com/hashicorp/consul/acl" "github.com/hashicorp/consul/agent/consul/authmethod/kubeauth" "github.com/hashicorp/consul/agent/consul/authmethod/testauth" @@ -18,10 +23,6 @@ import ( "github.com/hashicorp/consul/sdk/freeport" "github.com/hashicorp/consul/sdk/testutil" "github.com/hashicorp/consul/sdk/testutil/retry" - uuid "github.com/hashicorp/go-uuid" - msgpackrpc "github.com/hashicorp/net-rpc-msgpackrpc" - "github.com/stretchr/testify/require" - "gopkg.in/square/go-jose.v2/jwt" ) func TestACLEndpoint_Bootstrap(t *testing.T) { @@ -4981,7 +4982,7 @@ func TestACLEndpoint_Login_with_TokenLocality(t *testing.T) { t.Skip("too slow for testing.Short") } - go t.Parallel() + t.Parallel() _, s1, codec := testACLServerWithConfig(t, func(c *Config) { c.ACLTokenMinExpirationTTL = 10 * time.Millisecond