From 79061cdedb4d76cb5658c79f6520cee3f7a6ba1b Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Thu, 23 Mar 2017 19:05:35 -0400 Subject: [PATCH 1/6] Fix vet issues --- consul/flood.go | 2 -- consul/fsm.go | 5 ++++- consul/servers/router_test.go | 25 ++++++++++++++++++++----- consul/state/txn.go | 5 ++++- consul/txn_endpoint.go | 10 ++++++++-- consul/txn_endpoint_test.go | 10 ++++++++-- logger/logger.go | 2 +- 7 files changed, 45 insertions(+), 14 deletions(-) diff --git a/consul/flood.go b/consul/flood.go index 60fe0dfd4d..fb3b09b8f5 100644 --- a/consul/flood.go +++ b/consul/flood.go @@ -46,7 +46,6 @@ func (s *Server) Flood(portFn servers.FloodPortFn, global *serf.Serf) { }() for { - WAIT: select { case <-s.serfLAN.ShutdownCh(): return @@ -60,7 +59,6 @@ func (s *Server) Flood(portFn servers.FloodPortFn, global *serf.Serf) { case <-floodCh: goto FLOOD } - goto WAIT FLOOD: servers.FloodJoins(s.logger, portFn, s.config.Datacenter, s.serfLAN, global) diff --git a/consul/fsm.go b/consul/fsm.go index 608f1530ce..8cf4a22e69 100644 --- a/consul/fsm.go +++ b/consul/fsm.go @@ -309,7 +309,10 @@ func (c *consulFSM) applyTxn(buf []byte, index uint64) interface{} { } defer metrics.MeasureSince([]string{"consul", "fsm", "txn"}, time.Now()) results, errors := c.state.TxnRW(index, req.Ops) - return structs.TxnResponse{results, errors} + return structs.TxnResponse{ + Results: results, + Errors: errors, + } } func (c *consulFSM) applyAutopilotUpdate(buf []byte, index uint64) interface{} { diff --git a/consul/servers/router_test.go b/consul/servers/router_test.go index a914371517..f3e3e6d280 100644 --- a/consul/servers/router_test.go +++ b/consul/servers/router_test.go @@ -405,7 +405,10 @@ func TestRouter_GetDatacenterMaps(t *testing.T) { Datacenter: "dc0", AreaID: types.AreaWAN, Coordinates: structs.Coordinates{ - &structs.Coordinate{"node0.dc0", lib.GenerateCoordinate(10 * time.Millisecond)}, + &structs.Coordinate{ + Node: "node0.dc0", + Coord: lib.GenerateCoordinate(10 * time.Millisecond), + }, }, }) { t.Fatalf("bad: %#v", entry) @@ -415,9 +418,18 @@ func TestRouter_GetDatacenterMaps(t *testing.T) { Datacenter: "dc1", AreaID: types.AreaWAN, Coordinates: structs.Coordinates{ - &structs.Coordinate{"node1.dc1", lib.GenerateCoordinate(3 * time.Millisecond)}, - &structs.Coordinate{"node2.dc1", lib.GenerateCoordinate(2 * time.Millisecond)}, - &structs.Coordinate{"node3.dc1", lib.GenerateCoordinate(5 * time.Millisecond)}, + &structs.Coordinate{ + Node: "node1.dc1", + Coord: lib.GenerateCoordinate(3 * time.Millisecond), + }, + &structs.Coordinate{ + Node: "node2.dc1", + Coord: lib.GenerateCoordinate(2 * time.Millisecond), + }, + &structs.Coordinate{ + Node: "node3.dc1", + Coord: lib.GenerateCoordinate(5 * time.Millisecond), + }, }, }) { t.Fatalf("bad: %#v", entry) @@ -427,7 +439,10 @@ func TestRouter_GetDatacenterMaps(t *testing.T) { Datacenter: "dc2", AreaID: types.AreaWAN, Coordinates: structs.Coordinates{ - &structs.Coordinate{"node1.dc2", lib.GenerateCoordinate(8 * time.Millisecond)}, + &structs.Coordinate{ + Node: "node1.dc2", + Coord: lib.GenerateCoordinate(8 * time.Millisecond), + }, }, }) { t.Fatalf("bad: %#v", entry) diff --git a/consul/state/txn.go b/consul/state/txn.go index d2b3c6f1f5..158d63407f 100644 --- a/consul/state/txn.go +++ b/consul/state/txn.go @@ -124,7 +124,10 @@ func (s *StateStore) txnDispatch(tx *memdb.Txn, idx uint64, ops structs.TxnOps) // Capture any error along with the index of the operation that // failed. if err != nil { - errors = append(errors, &structs.TxnError{i, err.Error()}) + errors = append(errors, &structs.TxnError{ + OpIndex: i, + What: err.Error(), + }) } } diff --git a/consul/txn_endpoint.go b/consul/txn_endpoint.go index d5125a7d5f..45b511be39 100644 --- a/consul/txn_endpoint.go +++ b/consul/txn_endpoint.go @@ -24,10 +24,16 @@ func (t *Txn) preCheck(acl acl.ACL, ops structs.TxnOps) structs.TxnErrors { if op.KV != nil { ok, err := kvsPreApply(t.srv, acl, op.KV.Verb, &op.KV.DirEnt) if err != nil { - errors = append(errors, &structs.TxnError{i, err.Error()}) + errors = append(errors, &structs.TxnError{ + OpIndex: i, + What: err.Error(), + }) } else if !ok { err = fmt.Errorf("failed to lock key %q due to lock delay", op.KV.DirEnt.Key) - errors = append(errors, &structs.TxnError{i, err.Error()}) + errors = append(errors, &structs.TxnError{ + OpIndex: i, + What: err.Error(), + }) } } } diff --git a/consul/txn_endpoint_test.go b/consul/txn_endpoint_test.go index e502589a01..f7f375f7f1 100644 --- a/consul/txn_endpoint_test.go +++ b/consul/txn_endpoint_test.go @@ -251,7 +251,10 @@ func TestTxn_Apply_ACLDeny(t *testing.T) { // These get filtered but won't result in an error. default: - expected.Errors = append(expected.Errors, &structs.TxnError{i, permissionDeniedErr.Error()}) + expected.Errors = append(expected.Errors, &structs.TxnError{ + OpIndex: i, + What: permissionDeniedErr.Error(), + }) } } if !reflect.DeepEqual(out, expected) { @@ -509,7 +512,10 @@ func TestTxn_Read_ACLDeny(t *testing.T) { // These get filtered but won't result in an error. default: - expected.Errors = append(expected.Errors, &structs.TxnError{i, permissionDeniedErr.Error()}) + expected.Errors = append(expected.Errors, &structs.TxnError{ + OpIndex: i, + What: permissionDeniedErr.Error(), + }) } } if !reflect.DeepEqual(out, expected) { diff --git a/logger/logger.go b/logger/logger.go index 781ae19c2f..92fa611d50 100644 --- a/logger/logger.go +++ b/logger/logger.go @@ -39,7 +39,7 @@ type Config struct { func Setup(config *Config, ui cli.Ui) (*logutils.LevelFilter, *GatedWriter, *LogWriter, io.Writer, bool) { // The gated writer buffers logs at startup and holds until it's flushed. logGate := &GatedWriter{ - Writer: &cli.UiWriter{ui}, + Writer: &cli.UiWriter{Ui: ui}, } // Set up the level filter. From 364a4bfc384ca1ff2c9d380b53afcd9eef615374 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Thu, 23 Mar 2017 19:06:09 -0400 Subject: [PATCH 2/6] Only build binary if api tests are running --- scripts/test.sh | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/scripts/test.sh b/scripts/test.sh index 4f5f3556d7..47bcc79e23 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -1,14 +1,22 @@ #!/usr/bin/env bash set -e -# Create a temp dir and clean it up on exit -TEMPDIR=`mktemp -d -t consul-test.XXX` -trap "rm -rf $TEMPDIR" EXIT HUP INT QUIT TERM +# If we are testing the API, build and install consul +if grep -q "/consul/api" <<< "${GOFILES}"; then + # Create a temp dir and clean it up on exit + TEMPDIR=`mktemp -d -t consul-test.XXX` + trap "rm -rf ${TEMPDIR}" EXIT HUP INT QUIT TERM -# Build the Consul binary for the API tests -echo "--> Building consul" -go build -tags="${BUILD_TAGS}" -o $TEMPDIR/consul || exit 1 + # Build the Consul binary for the API tests + echo "--> Building consul" + go build -tags="${GOTAGS}" -o $TEMPDIR/consul + PATH="${TEMPDIR}:${PATH}" +fi + +# Install all packages - this will make running the suite faster +echo "--> Installing packages for faster tests" +go install -tags="${GOTAGS}" -a ./... # Run the tests echo "--> Running tests" -go list ./... | grep -v '/vendor/' | PATH=$TEMPDIR:$PATH xargs -n1 go test -tags="${BUILD_TAGS}" ${GOTEST_FLAGS:--cover -timeout=360s} +go test -timeout=360s -parallel=20 -tags="${GOTAGS}" ${GOFILES} ${TESTARGS} From 0a04d23f505d41fdeff338f671734efde6a717b7 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Thu, 23 Mar 2017 19:06:25 -0400 Subject: [PATCH 3/6] Modernize makefile a bit --- .travis.yml | 2 +- GNUmakefile | 46 +++++++++++++++++++--------------------------- 2 files changed, 20 insertions(+), 28 deletions(-) diff --git a/.travis.yml b/.travis.yml index b5f5705fe5..456564a0f7 100644 --- a/.travis.yml +++ b/.travis.yml @@ -8,6 +8,6 @@ branches: - master script: - - make ci + - make test vet sudo: false diff --git a/GNUmakefile b/GNUmakefile index 2e9a11da8c..9e3b646a59 100644 --- a/GNUmakefile +++ b/GNUmakefile @@ -4,59 +4,51 @@ GOTOOLS = \ github.com/mitchellh/gox \ golang.org/x/tools/cmd/cover \ golang.org/x/tools/cmd/stringer -PACKAGES=$(shell go list ./... | grep -v '/vendor/') -VETARGS?=-asmdecl -atomic -bool -buildtags -copylocks -methods \ - -nilfunc -printf -rangeloops -shift -structtags -unsafeptr -BUILD_TAGS?=consul +TEST ?= ./... +GOTAGS ?= consul +GOFILES ?= $(shell go list $(TEST) | grep -v /vendor/) # all builds binaries for all targets all: bin -ci: - if [ "${TRAVIS_PULL_REQUEST}" = "false" ]; then \ - $(MAKE) bin ;\ - fi - @$(MAKE) test - bin: tools @mkdir -p bin/ - @BUILD_TAGS='$(BUILD_TAGS)' sh -c "'$(CURDIR)/scripts/build.sh'" + @GOTAGS='$(GOTAGS)' sh -c "'$(CURDIR)/scripts/build.sh'" # dev creates binaries for testing locally - these are put into ./bin and $GOPATH dev: format - @CONSUL_DEV=1 BUILD_TAGS='$(BUILD_TAGS)' sh -c "'$(CURDIR)/scripts/build.sh'" + @CONSUL_DEV=1 GOTAGS='$(GOTAGS)' sh -c "'$(CURDIR)/scripts/build.sh'" # dist builds binaries for all platforms and packages them for distribution dist: - @BUILD_TAGS='$(BUILD_TAGS)' sh -c "'$(CURDIR)/scripts/dist.sh'" + @GOTAGS='$(GOTAGS)' sh -c "'$(CURDIR)/scripts/dist.sh'" cov: - gocov test ./... | gocov-html > /tmp/coverage.html + gocov test ${GOFILES} | gocov-html > /tmp/coverage.html open /tmp/coverage.html -test: format - @$(MAKE) vet +test: @./scripts/verify_no_uuid.sh - @BUILD_TAGS='$(BUILD_TAGS)' sh -c "'$(CURDIR)/scripts/test.sh'" + @env \ + GOTAGS="${GOTAGS}" \ + GOFILES="${GOFILES}" \ + TESTARGS="${TESTARGS}" \ + sh -c "'$(CURDIR)/scripts/test.sh'" cover: - go list ./... | xargs -n1 go test --cover + go test ${GOFILES} --cover format: @echo "--> Running go fmt" - @go fmt $(PACKAGES) + @go fmt ${GOFILES} vet: - @echo "--> Running go tool vet $(VETARGS) ." - @go list ./... \ - | grep -v /vendor/ \ - | cut -d '/' -f 4- \ - | xargs -n1 \ - go tool vet $(VETARGS) ;\ - if [ $$? -ne 0 ]; then \ + @echo "--> Running go vet" + @go vet ${GOFILES}; if [ $$? -eq 1 ]; then \ echo ""; \ echo "Vet found suspicious constructs. Please check the reported constructs"; \ - echo "and fix them if necessary before submitting the code for reviewal."; \ + echo "and fix them if necessary before submitting the code for review."; \ + exit 1; \ fi # build the static web ui and build static assets inside a Docker container, the From 38b1c61accbe8bf41aaee446e04435f8d7d9473d Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Thu, 23 Mar 2017 19:12:30 -0400 Subject: [PATCH 4/6] Install packages, then build --- scripts/test.sh | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/scripts/test.sh b/scripts/test.sh index 47bcc79e23..816e383008 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -1,6 +1,10 @@ #!/usr/bin/env bash set -e +# Install all packages - this will make running the suite faster +echo "--> Installing packages for faster tests" +go install -tags="${GOTAGS}" -a ./... + # If we are testing the API, build and install consul if grep -q "/consul/api" <<< "${GOFILES}"; then # Create a temp dir and clean it up on exit @@ -13,10 +17,6 @@ if grep -q "/consul/api" <<< "${GOFILES}"; then PATH="${TEMPDIR}:${PATH}" fi -# Install all packages - this will make running the suite faster -echo "--> Installing packages for faster tests" -go install -tags="${GOTAGS}" -a ./... - # Run the tests echo "--> Running tests" go test -timeout=360s -parallel=20 -tags="${GOTAGS}" ${GOFILES} ${TESTARGS} From 73fc8d2a5279460ba8142bb2ca98cf8790bd9618 Mon Sep 17 00:00:00 2001 From: Seth Vargo Date: Thu, 23 Mar 2017 19:14:38 -0400 Subject: [PATCH 5/6] Only install on Travis --- scripts/test.sh | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/scripts/test.sh b/scripts/test.sh index 816e383008..fe1fac5600 100755 --- a/scripts/test.sh +++ b/scripts/test.sh @@ -1,9 +1,11 @@ #!/usr/bin/env bash set -e -# Install all packages - this will make running the suite faster -echo "--> Installing packages for faster tests" -go install -tags="${GOTAGS}" -a ./... +if [ -n "$TRAVIS" ]; then + # Install all packages - this will make running the suite faster + echo "--> Installing packages for faster tests" + go install -tags="${GOTAGS}" -a ./... +fi # If we are testing the API, build and install consul if grep -q "/consul/api" <<< "${GOFILES}"; then From e7ec41ecbe1c923252f0a05edf3e32a791701693 Mon Sep 17 00:00:00 2001 From: James Phillips Date: Thu, 23 Mar 2017 17:55:47 -0700 Subject: [PATCH 6/6] Fixes up some new tests for the updated testutil. --- consul/leader_test.go | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/consul/leader_test.go b/consul/leader_test.go index 645058a0dc..427e07d709 100644 --- a/consul/leader_test.go +++ b/consul/leader_test.go @@ -374,7 +374,7 @@ func TestLeader_Reconcile_Races(t *testing.T) { // Wait for the server to reconcile the client and register it. state := s1.fsm.State() var nodeAddr string - testutil.WaitForResult(func() (bool, error) { + if err := testutil.WaitForResult(func() (bool, error) { _, node, err := state.GetNode(c1.config.NodeName) if err != nil { t.Fatalf("err: %v", err) @@ -385,9 +385,9 @@ func TestLeader_Reconcile_Races(t *testing.T) { } else { return false, nil } - }, func(err error) { - t.Fatalf("client should be registered") - }) + }); err != nil { + t.Fatalf("client should be registered: %v", err) + } // Add in some metadata via the catalog (as if the agent synced it // there). We also set the serfHealth check to failing so the reconile @@ -428,15 +428,15 @@ func TestLeader_Reconcile_Races(t *testing.T) { // Fail the member and wait for the health to go critical. c1.Shutdown() - testutil.WaitForResult(func() (bool, error) { + if err := testutil.WaitForResult(func() (bool, error) { _, checks, err := state.NodeChecks(nil, c1.config.NodeName) if err != nil { t.Fatalf("err: %v", err) } return checks[0].Status == structs.HealthCritical, errors.New(checks[0].Status) - }, func(err error) { - t.Fatalf("check status is %v, should be critical", err) - }) + }); err != nil { + t.Fatalf("check status should be critical: %v", err) + } // Make sure the metadata didn't get clobbered. _, node, err = state.GetNode(c1.config.NodeName)