From 162cf4bda945faf0a81ab15c95371d69050359ae Mon Sep 17 00:00:00 2001 From: Steven L Date: Fri, 3 May 2024 10:27:00 -0500 Subject: [PATCH 1/2] Enforcing go vet -copylocks and fixing current violations --- Makefile | 2 ++ common/domain/handler_MasterCluster_test.go | 12 ++++++------ common/domain/handler_NotMasterCluster_test.go | 2 +- .../persistence-tests/historyV2PersistenceTest.go | 14 +++++++------- .../persistence-tests/persistenceTestBase.go | 3 ++- host/pinotutils/pinotClient.go | 2 +- 6 files changed, 19 insertions(+), 16 deletions(-) diff --git a/Makefile b/Makefile index 76fb5bf5e70..392c7368e61 100644 --- a/Makefile +++ b/Makefile @@ -360,6 +360,8 @@ $(BUILD)/gomod-lint: go.mod internal/tools/go.mod common/archiver/gcloud/go.mod # it's a coarse "you probably don't need to re-lint" filter, nothing more. $(BUILD)/code-lint: $(LINT_SRC) $(BIN)/revive | $(BUILD) $Q echo "lint..." + $Q # non-optional vet checks. unfortunately these are not currently included in `go test`'s default behavior. + $Q go vet -copylocks ./... $Q $(BIN)/revive -config revive.toml -exclude './vendor/...' -exclude './.gen/...' -formatter stylish ./... $Q # look for go files with "//comments", and ignore "//go:build"-style directives ("grep -n" shows "file:line: //go:build" so the regex is a bit complex) $Q bad="$$(find . -type f -name '*.go' -not -path './idls/*' | xargs grep -n -E '^\s*//\S' | grep -E -v '^[^:]+:[^:]+:\s*//[a-z]+:[a-z]+' || true)"; \ diff --git a/common/domain/handler_MasterCluster_test.go b/common/domain/handler_MasterCluster_test.go index 406b5b7d02f..ddd00c55152 100644 --- a/common/domain/handler_MasterCluster_test.go +++ b/common/domain/handler_MasterCluster_test.go @@ -990,7 +990,7 @@ func (s *domainHandlerGlobalDomainEnabledPrimaryClusterSuite) setupGlobalDomain( return setupGlobalDomain(s.Suite, s.handler, s.ClusterMetadata, domainName) } -func setupGlobalDomain(s suite.Suite, handler *handlerImpl, clusterMetadata cluster.Metadata, domainName string) *types.DescribeDomainResponse { +func setupGlobalDomain(s *suite.Suite, handler *handlerImpl, clusterMetadata cluster.Metadata, domainName string) *types.DescribeDomainResponse { description := "some random description" email := "some random email" retention := int32(7) @@ -1030,7 +1030,7 @@ func setupGlobalDomain(s suite.Suite, handler *handlerImpl, clusterMetadata clus return getResp } -func setupLocalDomain(s suite.Suite, handler *handlerImpl, clusterMetadata cluster.Metadata, domainName string) *types.DescribeDomainResponse { +func setupLocalDomain(s *suite.Suite, handler *handlerImpl, clusterMetadata cluster.Metadata, domainName string) *types.DescribeDomainResponse { description := "some random description" email := "some random email" retention := int32(7) @@ -1060,8 +1060,8 @@ func setupLocalDomain(s suite.Suite, handler *handlerImpl, clusterMetadata clust return getResp } -func assertDomainEqual(s suite.Suite, autual, expected *types.DescribeDomainResponse) { - s.NotEmpty(autual.DomainInfo.GetUUID()) - expected.DomainInfo.UUID = autual.DomainInfo.GetUUID() - s.Equal(expected, autual) +func assertDomainEqual(s *suite.Suite, actual, expected *types.DescribeDomainResponse) { + s.NotEmpty(actual.DomainInfo.GetUUID()) + expected.DomainInfo.UUID = actual.DomainInfo.GetUUID() + s.Equal(expected, actual) } diff --git a/common/domain/handler_NotMasterCluster_test.go b/common/domain/handler_NotMasterCluster_test.go index 936d6ce76b7..2e043185236 100644 --- a/common/domain/handler_NotMasterCluster_test.go +++ b/common/domain/handler_NotMasterCluster_test.go @@ -767,7 +767,7 @@ func (s *domainHandlerGlobalDomainEnabledNotPrimaryClusterSuite) setupGlobalDoma return setupGlobalDomainWithMetadataManager(s.Suite, s.handler, s.ClusterMetadata, s.DomainManager, domainName) } -func setupGlobalDomainWithMetadataManager(s suite.Suite, handler *handlerImpl, clusterMetadata cluster.Metadata, domainManager persistence.DomainManager, domainName string) *types.DescribeDomainResponse { +func setupGlobalDomainWithMetadataManager(s *suite.Suite, handler *handlerImpl, clusterMetadata cluster.Metadata, domainManager persistence.DomainManager, domainName string) *types.DescribeDomainResponse { description := "some random description" email := "some random email" retention := int32(7) diff --git a/common/persistence/persistence-tests/historyV2PersistenceTest.go b/common/persistence/persistence-tests/historyV2PersistenceTest.go index 85fc999b0fc..d26aa2936c0 100644 --- a/common/persistence/persistence-tests/historyV2PersistenceTest.go +++ b/common/persistence/persistence-tests/historyV2PersistenceTest.go @@ -101,7 +101,7 @@ func (s *HistoryV2PersistenceSuite) TearDownSuite() { // TestGenUUIDs testing uuid.New() can generate unique UUID func (s *HistoryV2PersistenceSuite) TestGenUUIDs() { wg := sync.WaitGroup{} - m := sync.Map{} + m := &sync.Map{} concurrency := 1000 for i := 0; i < concurrency; i++ { wg.Add(1) @@ -377,7 +377,7 @@ func (s *HistoryV2PersistenceSuite) TestConcurrentlyCreateAndAppendBranches() { treeID := uuid.New() wg := sync.WaitGroup{} concurrency := 20 - m := sync.Map{} + m := &sync.Map{} // test create new branch along with appending new nodes for i := 0; i < concurrency; i++ { @@ -538,8 +538,8 @@ func (s *HistoryV2PersistenceSuite) TestConcurrentlyForkAndAppendBranches() { s.Nil(err) s.Equal((concurrency)+1, len(events)) - level1ID := sync.Map{} - level1Br := sync.Map{} + level1ID := &sync.Map{} + level1Br := &sync.Map{} // test forking from master branch and append nodes for i := 0; i < concurrency; i++ { wg.Add(1) @@ -588,7 +588,7 @@ func (s *HistoryV2PersistenceSuite) TestConcurrentlyForkAndAppendBranches() { branches = s.descTreeByToken(ctx, masterBr) s.Equal(concurrency, len(branches)) forkOnLevel1 := int32(0) - level2Br := sync.Map{} + level2Br := &sync.Map{} wg = sync.WaitGroup{} // test forking for second level of branch @@ -687,14 +687,14 @@ func (s *HistoryV2PersistenceSuite) TestConcurrentlyForkAndAppendBranches() { } -func (s *HistoryV2PersistenceSuite) getBranchByKey(m sync.Map, k int) []byte { +func (s *HistoryV2PersistenceSuite) getBranchByKey(m *sync.Map, k int) []byte { v, ok := m.Load(k) s.Equal(true, ok) br := v.([]byte) return br } -func (s *HistoryV2PersistenceSuite) getIDByKey(m sync.Map, k int) int64 { +func (s *HistoryV2PersistenceSuite) getIDByKey(m *sync.Map, k int) int64 { v, ok := m.Load(k) s.Equal(true, ok) id := v.(int64) diff --git a/common/persistence/persistence-tests/persistenceTestBase.go b/common/persistence/persistence-tests/persistenceTestBase.go index d4828c33c23..5ec46197562 100644 --- a/common/persistence/persistence-tests/persistenceTestBase.go +++ b/common/persistence/persistence-tests/persistenceTestBase.go @@ -74,7 +74,7 @@ type ( // TestBase wraps the base setup needed to create workflows over persistence layer. TestBase struct { - suite.Suite + *suite.Suite ShardMgr persistence.ShardManager ExecutionMgrFactory client.Factory ExecutionManager persistence.ExecutionManager @@ -114,6 +114,7 @@ const ( // NewTestBaseFromParams returns a customized test base from given input func NewTestBaseFromParams(t *testing.T, params TestBaseParams) *TestBase { res := &TestBase{ + Suite: &suite.Suite{}, DefaultTestCluster: params.DefaultTestCluster, VisibilityTestCluster: params.VisibilityTestCluster, ClusterMetadata: params.ClusterMetadata, diff --git a/host/pinotutils/pinotClient.go b/host/pinotutils/pinotClient.go index 16390cc53d3..8d85b20565d 100644 --- a/host/pinotutils/pinotClient.go +++ b/host/pinotutils/pinotClient.go @@ -29,7 +29,7 @@ import ( pnt "github.com/uber/cadence/common/pinot" ) -func CreatePinotClient(s suite.Suite, pinotConfig *config.PinotVisibilityConfig, logger log.Logger) pnt.GenericClient { +func CreatePinotClient(s *suite.Suite, pinotConfig *config.PinotVisibilityConfig, logger log.Logger) pnt.GenericClient { pinotRawClient, err := pinot.NewFromBrokerList([]string{pinotConfig.Broker}) s.Require().NoError(err) pinotClient := pnt.NewPinotClient(pinotRawClient, logger, pinotConfig) From 758139b25deec98b6a61f39bc967f71156e15ab1 Mon Sep 17 00:00:00 2001 From: Steven L Date: Fri, 3 May 2024 16:40:50 -0500 Subject: [PATCH 2/2] also get the other submodule. internal does not really have "a submodule" to check like this so it is skipped --- Makefile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Makefile b/Makefile index 392c7368e61..f9a292a4b3d 100644 --- a/Makefile +++ b/Makefile @@ -361,7 +361,7 @@ $(BUILD)/gomod-lint: go.mod internal/tools/go.mod common/archiver/gcloud/go.mod $(BUILD)/code-lint: $(LINT_SRC) $(BIN)/revive | $(BUILD) $Q echo "lint..." $Q # non-optional vet checks. unfortunately these are not currently included in `go test`'s default behavior. - $Q go vet -copylocks ./... + $Q go vet -copylocks ./... ./common/archiver/gcloud/... $Q $(BIN)/revive -config revive.toml -exclude './vendor/...' -exclude './.gen/...' -formatter stylish ./... $Q # look for go files with "//comments", and ignore "//go:build"-style directives ("grep -n" shows "file:line: //go:build" so the regex is a bit complex) $Q bad="$$(find . -type f -name '*.go' -not -path './idls/*' | xargs grep -n -E '^\s*//\S' | grep -E -v '^[^:]+:[^:]+:\s*//[a-z]+:[a-z]+' || true)"; \