Enabled more golangci tests to improve code health (#1089)

* Enabled more golangci tests

* update

* update

* update
This commit is contained in:
yfei1
2020-02-06 14:07:41 -08:00
committed by GitHub
parent a9c327b430
commit 79daf50531
32 changed files with 87 additions and 75 deletions

View File

@ -171,17 +171,10 @@ linters:
- funlen
- gochecknoglobals
- goconst
- gocritic
- gocyclo
- gofmt
- goimports
- gosec
- interfacer # deprecated - "A tool that suggests interfaces is prone to bad suggestions"
- lll
- prealloc
- scopelint
- staticcheck
- stylecheck
#linters:
# enable-all: true

View File

@ -87,7 +87,7 @@ func (s *backendService) FetchMatches(req *pb.FetchMatchesRequest, stream pb.Bac
mmfWait := omerror.WaitOnErrors(logger, func() error {
select {
case <-mmfCtx.Done():
return fmt.Errorf("Mmf was never started")
return fmt.Errorf("mmf was never started")
case <-startMmfs:
}
@ -109,7 +109,7 @@ func (s *backendService) FetchMatches(req *pb.FetchMatchesRequest, stream pb.Bac
}).Error("error(s) in FetchMatches call.")
return fmt.Errorf(
"Error(s) in FetchMatches call. syncErr=[%s], mmfErr=[%s]",
"error(s) in FetchMatches call. syncErr=[%s], mmfErr=[%s]",
syncErr,
mmfErr,
)

View File

@ -151,6 +151,7 @@ func TestDoReleaseTickets(t *testing.T) {
}
for _, test := range tests {
test := test
t.Run(test.description, func(t *testing.T) {
ctx, cancel := context.WithCancel(utilTesting.NewContext(t))
cfg := viper.New()
@ -272,6 +273,7 @@ func TestDoAssignTickets(t *testing.T) {
}
for _, test := range tests {
test := test
t.Run(test.description, func(t *testing.T) {
ctx, cancel := context.WithCancel(utilTesting.NewContext(t))
cfg := viper.New()

View File

@ -68,6 +68,7 @@ func TestDoCreateTickets(t *testing.T) {
}
for _, test := range tests {
test := test
t.Run(test.description, func(t *testing.T) {
store, closer := statestoreTesting.NewStoreServiceForTesting(t, cfg)
defer closer()
@ -193,6 +194,7 @@ func TestDoDeleteTicket(t *testing.T) {
}
for _, test := range tests {
test := test
t.Run(test.description, func(t *testing.T) {
ctx, cancel := context.WithCancel(utilTesting.NewContext(t))
store, closer := statestoreTesting.NewStoreServiceForTesting(t, viper.New())
@ -246,6 +248,7 @@ func TestDoGetTicket(t *testing.T) {
}
for _, test := range tests {
test := test
t.Run(test.description, func(t *testing.T) {
ctx, cancel := context.WithCancel(utilTesting.NewContext(t))
store, closer := statestoreTesting.NewStoreServiceForTesting(t, viper.New())

View File

@ -131,6 +131,7 @@ func TestDoQueryTickets(t *testing.T) {
}
for _, test := range tests {
test := test
t.Run(test.description, func(t *testing.T) {
cfg := viper.New()
cfg.Set("storage.page.size", 1000)
@ -183,6 +184,7 @@ func TestGetPageSize(t *testing.T) {
}
for _, tt := range testCases {
tt := tt
t.Run(tt.name, func(t *testing.T) {
cfg := viper.New()
tt.configure(cfg)

View File

@ -24,7 +24,6 @@ import (
"github.com/golang/protobuf/jsonpb"
"github.com/sirupsen/logrus"
"google.golang.org/grpc"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
"open-match.dev/open-match/internal/config"
@ -44,7 +43,7 @@ type evaluator interface {
evaluate(context.Context, <-chan []*pb.Match) ([]string, error)
}
var errNoEvaluatorType = grpc.Errorf(codes.FailedPrecondition, "unable to determine evaluator type, either api.evaluator.grpcport or api.evaluator.httpport must be specified in the config")
var errNoEvaluatorType = status.Errorf(codes.FailedPrecondition, "unable to determine evaluator type, either api.evaluator.grpcport or api.evaluator.httpport must be specified in the config")
func newEvaluator(cfg config.View) evaluator {
newInstance := func(cfg config.View) (interface{}, func(), error) {
@ -88,7 +87,7 @@ func newGrpcEvaluator(cfg config.View) (evaluator, func(), error) {
grpcAddr := fmt.Sprintf("%s:%d", cfg.GetString("api.evaluator.hostname"), cfg.GetInt64("api.evaluator.grpcport"))
conn, err := rpc.GRPCClientFromEndpoint(cfg, grpcAddr)
if err != nil {
return nil, nil, fmt.Errorf("Failed to create grpc evaluator client: %w", err)
return nil, nil, fmt.Errorf("failed to create grpc evaluator client: %w", err)
}
evaluatorClientLogger.WithFields(logrus.Fields{
@ -113,7 +112,7 @@ func (ec *grcpEvaluatorClient) evaluate(ctx context.Context, pc <-chan []*pb.Mat
var err error
stream, err = ec.evaluator.Evaluate(ctx)
if err != nil {
return nil, fmt.Errorf("Error starting evaluator call: %w", err)
return nil, fmt.Errorf("error starting evaluator call: %w", err)
}
}

View File

@ -133,8 +133,10 @@ var getTests = []struct {
},
}
//nolint: gocritic, staticcheck
func Test_Get(t *testing.T) {
for _, tt := range getTests {
tt := tt
t.Run(tt.name, func(t *testing.T) {
if tt.verifySame == nil {
tt.verifySame = func(a, b interface{}) bool {

View File

@ -17,9 +17,10 @@ package config
import (
"fmt"
"log"
"github.com/fsnotify/fsnotify"
"github.com/spf13/viper"
"log"
)
// Read sets default to a viper instance and read user config to override these defaults.

View File

@ -59,10 +59,8 @@ func WaitOnErrors(logger *logrus.Entry, fs ...func() error) WaitFunc {
err := <-errors
if first == nil {
first = err
} else {
if err != nil {
logger.WithError(err).Warning("Multiple errors occurred in parallel execution. This error is suppressed by the error returned.")
}
} else if err != nil {
logger.WithError(err).Warning("Multiple errors occurred in parallel execution. This error is suppressed by the error returned.")
}
}
return first

View File

@ -15,10 +15,11 @@
package rpc
import (
"google.golang.org/grpc"
"net/http"
"open-match.dev/open-match/internal/config"
"sync"
"google.golang.org/grpc"
"open-match.dev/open-match/internal/config"
)
// ClientCache holds GRPC and HTTP clients based on an address.

View File

@ -15,9 +15,10 @@
package rpc
import (
"testing"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"testing"
)
const (

View File

@ -36,7 +36,6 @@ import (
"github.com/sirupsen/logrus"
"go.opencensus.io/plugin/ochttp"
"google.golang.org/grpc"
"google.golang.org/grpc/balancer/roundrobin"
"google.golang.org/grpc/credentials"
"google.golang.org/grpc/keepalive"
"google.golang.org/grpc/resolver"
@ -311,7 +310,7 @@ func newGRPCDialOptions(enableMetrics bool, enableRPCLogging bool, enableRPCPayl
opts := []grpc.DialOption{
grpc.WithStreamInterceptor(grpc_middleware.ChainStreamClient(si...)),
grpc.WithUnaryInterceptor(grpc_middleware.ChainUnaryClient(ui...)),
grpc.WithBalancerName(roundrobin.Name),
grpc.WithDefaultServiceConfig(`{"loadBalancingPolicy":"round_robin"}`),
grpc.WithKeepaliveParams(keepalive.ClientParameters{
Time: 20 * time.Second,
Timeout: 10 * time.Second,

View File

@ -16,19 +16,20 @@ package rpc
import (
"fmt"
"io/ioutil"
"net/http"
"os"
"testing"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"google.golang.org/grpc"
"io/ioutil"
"net/http"
"open-match.dev/open-match/internal/config"
"open-match.dev/open-match/internal/telemetry"
shellTesting "open-match.dev/open-match/internal/testing"
utilTesting "open-match.dev/open-match/internal/util/testing"
"open-match.dev/open-match/pkg/pb"
certgenTesting "open-match.dev/open-match/tools/certgen/testing"
"os"
"testing"
)
func TestSecureGRPCFromConfig(t *testing.T) {

View File

@ -67,9 +67,11 @@ func TestObtain(t *testing.T) {
listener, err := lh.Obtain()
if err != nil {
atomic.AddUint64(&errCount, 1)
} else if listener != nil {
}
if listener != nil {
atomic.AddUint64(&obtainCount, 1)
} else {
}
if err != nil && listener != nil {
t.Error("err and listener were both nil.")
}
wg.Done()

View File

@ -16,17 +16,18 @@ package rpc
import (
"fmt"
"github.com/stretchr/testify/assert"
"google.golang.org/grpc"
"io/ioutil"
"net/http"
"strings"
"testing"
"time"
"github.com/stretchr/testify/assert"
"google.golang.org/grpc"
"open-match.dev/open-match/internal/telemetry"
shellTesting "open-match.dev/open-match/internal/testing"
utilTesting "open-match.dev/open-match/internal/util/testing"
"open-match.dev/open-match/pkg/pb"
"strings"
"testing"
"time"
)
func TestStartStopServer(t *testing.T) {

View File

@ -52,6 +52,7 @@ func TestStringOperations(t *testing.T) {
}
for i, tt := range setTests {
tt := tt
t.Run(fmt.Sprintf("%#v-%d", tt.op, i), func(t *testing.T) {
actual := tt.op(tt.in1, tt.in2)
sort.Strings(tt.expected)

View File

@ -56,11 +56,11 @@ type indexFilter struct {
min, max float64
}
func extractIndexFilters(p *pb.Pool) []indexFilter {
filters := make([]indexFilter, 0)
func extractIndexFilters(p *pb.Pool) []*indexFilter {
filters := make([]*indexFilter, 0)
for _, f := range p.DoubleRangeFilters {
filters = append(filters, indexFilter{
filters = append(filters, &indexFilter{
name: rangeIndexName(f.DoubleArg),
min: f.Min,
max: f.Max,
@ -68,7 +68,7 @@ func extractIndexFilters(p *pb.Pool) []indexFilter {
}
for _, f := range p.TagPresentFilters {
filters = append(filters, indexFilter{
filters = append(filters, &indexFilter{
name: tagIndexName(f.Tag),
min: 0,
max: 0,
@ -76,7 +76,7 @@ func extractIndexFilters(p *pb.Pool) []indexFilter {
}
for _, f := range p.StringEqualsFilters {
filters = append(filters, indexFilter{
filters = append(filters, &indexFilter{
name: stringIndexName(f.StringArg, f.Value),
min: 0,
max: 0,
@ -84,7 +84,7 @@ func extractIndexFilters(p *pb.Pool) []indexFilter {
}
if len(filters) == 0 {
filters = []indexFilter{{
filters = []*indexFilter{{
name: allTickets,
min: math.Inf(-1),
max: math.Inf(1),

View File

@ -69,6 +69,7 @@ func TestExtractIndexedFields(t *testing.T) {
}
for _, test := range tests {
test := test
t.Run(test.description, func(t *testing.T) {
actual := extractIndexedFields(&pb.Ticket{SearchFields: test.searchFields})
@ -81,12 +82,12 @@ func TestExtractIndexFilters(t *testing.T) {
tests := []struct {
description string
pool *pb.Pool
expected []indexFilter
expected []*indexFilter
}{
{
description: "empty",
pool: &pb.Pool{},
expected: []indexFilter{
expected: []*indexFilter{
{
name: "allTickets",
min: math.Inf(-1),
@ -105,7 +106,7 @@ func TestExtractIndexFilters(t *testing.T) {
},
},
},
expected: []indexFilter{
expected: []*indexFilter{
{
name: "ri$foo",
min: -1,
@ -122,7 +123,7 @@ func TestExtractIndexFilters(t *testing.T) {
},
},
},
expected: []indexFilter{
expected: []*indexFilter{
{
name: "ti$foo",
min: 0,
@ -140,7 +141,7 @@ func TestExtractIndexFilters(t *testing.T) {
},
},
},
expected: []indexFilter{
expected: []*indexFilter{
{
name: "si$foo$vbar",
min: 0,
@ -151,6 +152,7 @@ func TestExtractIndexFilters(t *testing.T) {
}
for _, test := range tests {
test := test
t.Run(test.description, func(t *testing.T) {
actual := extractIndexFilters(test.pool)

View File

@ -22,9 +22,10 @@ import (
"bufio"
"bytes"
"fmt"
"strings"
"github.com/spf13/viper"
"open-match.dev/open-match/internal/config"
"strings"
)
const (

View File

@ -15,11 +15,12 @@
package telemetry
import (
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
"net/http"
"net/url"
"testing"
"github.com/spf13/viper"
"github.com/stretchr/testify/assert"
)
func TestConfigz(t *testing.T) {

View File

@ -15,10 +15,11 @@
package telemetry
import (
"github.com/stretchr/testify/assert"
"net/http"
"net/url"
"testing"
"github.com/stretchr/testify/assert"
)
func TestHelp(t *testing.T) {

View File

@ -17,11 +17,12 @@ package telemetry
import (
"context"
"fmt"
"github.com/stretchr/testify/assert"
"net/http"
"net/url"
"sync/atomic"
"testing"
"github.com/stretchr/testify/assert"
)
func angryHealthCheck(context.Context) error {
@ -47,6 +48,7 @@ func TestHealthCheck(t *testing.T) {
{"angryHealthCheck", []func(context.Context) error{angryHealthCheck}, "I'm angry"},
}
for _, tc := range testCases {
tc := tc
t.Run(tc.name, func(t *testing.T) {
assertHealthCheck(t, NewHealthCheck(tc.healthChecks), tc.errorString)
})

View File

@ -15,10 +15,11 @@
package telemetry
import (
"github.com/sirupsen/logrus"
"go.opencensus.io/zpages"
"net/http"
"net/http/pprof"
"github.com/sirupsen/logrus"
"go.opencensus.io/zpages"
"open-match.dev/open-match/internal/config"
)

View File

@ -24,7 +24,6 @@ import (
grpc_validator "github.com/grpc-ecosystem/go-grpc-middleware/validator"
"github.com/sirupsen/logrus"
"google.golang.org/grpc"
"google.golang.org/grpc/balancer/roundrobin"
"google.golang.org/grpc/keepalive"
"google.golang.org/grpc/resolver"
)
@ -54,7 +53,7 @@ func NewGRPCDialOptions(grpcLogger *logrus.Entry) []grpc.DialOption {
grpc.WithInsecure(),
grpc.WithStreamInterceptor(grpc_middleware.ChainStreamClient(si...)),
grpc.WithUnaryInterceptor(grpc_middleware.ChainUnaryClient(ui...)),
grpc.WithBalancerName(roundrobin.Name),
grpc.WithDefaultServiceConfig(`{"loadBalancingPolicy":"round_robin"}`),
grpc.WithKeepaliveParams(keepalive.ClientParameters{
Time: 20 * time.Second,
Timeout: 10 * time.Second,

View File

@ -27,7 +27,7 @@ import (
func QueryPool(ctx context.Context, mml pb.QueryServiceClient, pool *pb.Pool) ([]*pb.Ticket, error) {
query, err := mml.QueryTickets(ctx, &pb.QueryTicketsRequest{Pool: pool})
if err != nil {
return nil, fmt.Errorf("Error calling queryService.QueryTickets: %w", err)
return nil, fmt.Errorf("error calling queryService.QueryTickets: %w", err)
}
var tickets []*pb.Ticket
@ -38,7 +38,7 @@ func QueryPool(ctx context.Context, mml pb.QueryServiceClient, pool *pb.Pool) ([
}
if err != nil {
return nil, fmt.Errorf("Error receiving tickets from queryService.QueryTickets: %w", err)
return nil, fmt.Errorf("error receiving tickets from queryService.QueryTickets: %w", err)
}
tickets = append(tickets, resp.Tickets...)
@ -73,7 +73,7 @@ func QueryPools(ctx context.Context, mml pb.QueryServiceClient, pools []*pb.Pool
for i := 0; i < len(pools); i++ {
select {
case <-ctx.Done():
return nil, fmt.Errorf("Context canceled while querying pools: %w", ctx.Err())
return nil, fmt.Errorf("context canceled while querying pools: %w", ctx.Err())
case r := <-results:
if r.err != nil {
return nil, r.err

View File

@ -15,8 +15,9 @@
package e2e
import (
"open-match.dev/open-match/internal/testing/e2e"
"testing"
"open-match.dev/open-match/internal/testing/e2e"
)
func TestMain(m *testing.M) {

View File

@ -94,7 +94,7 @@ func TestMinimatch(t *testing.T) {
// Test profiles being tested for. Note that each profile embeds two pools - and
// the current MMF returns a match per pool in the profile - so each profile should
// output two matches that are comprised of tickets belonging to that pool.
sourceProfiles := []testProfile{
sourceProfiles := []*testProfile{
{name: "", pools: []*pb.Pool{testPools[e2e.Map1BeginnerPool], testPools[e2e.Map1AdvancedPool]}},
{name: "", pools: []*pb.Pool{testPools[e2e.Map2BeginnerPool], testPools[e2e.Map2AdvancedPool]}},
}
@ -117,10 +117,9 @@ func TestMinimatch(t *testing.T) {
for _, test := range tests {
test := test
testTickets := make([]testTicket, len(sourceTickets))
testProfiles := make([]testProfile, len(sourceProfiles))
testProfiles := make([]*testProfile, len(sourceProfiles))
copy(testTickets, sourceTickets)
copy(testProfiles, sourceProfiles)
t.Run(fmt.Sprintf("TestMinimatch-%v", test.description), func(t *testing.T) {
t.Parallel()
@ -134,12 +133,12 @@ func TestMinimatch(t *testing.T) {
// Create all the tickets and validate ticket creation succeeds. Also populate ticket ids
// to expected player pools.
for i, td := range testTickets {
for i := 0; i < len(testTickets); i++ {
resp, err := fe.CreateTicket(ctx, &pb.CreateTicketRequest{Ticket: &pb.Ticket{
SearchFields: &pb.SearchFields{
DoubleArgs: map[string]float64{
e2e.DoubleArgDefense: td.skill,
td.mapValue: float64(time.Now().Unix()),
e2e.DoubleArgDefense: testTickets[i].skill,
testTickets[i].mapValue: float64(time.Now().Unix()),
},
},
}})
@ -193,7 +192,7 @@ func TestMinimatch(t *testing.T) {
})
}
func testFetchMatches(ctx context.Context, t *testing.T, poolTickets map[string][]string, testProfiles []testProfile, om e2e.OM, fc *pb.FunctionConfig) {
func testFetchMatches(ctx context.Context, t *testing.T, poolTickets map[string][]string, testProfiles []*testProfile, om e2e.OM, fc *pb.FunctionConfig) {
// Fetch Matches for each test profile.
be := om.MustBackendGRPC()
for _, profile := range testProfiles {

View File

@ -93,11 +93,6 @@ func TestMakeMatches(t *testing.T) {
}
matchGen := func(tickets []*pb.Ticket) *pb.Match {
tids := []string{}
for _, ticket := range tickets {
tids = append(tids, ticket.GetId())
}
evaluationInput, err := ptypes.MarshalAny(&pb.DefaultEvaluationCriteria{
Score: scoreCalculator(tickets),
})

View File

@ -154,7 +154,7 @@ func CreateCertificateAndPrivateKey(params *Params) ([]byte, []byte, error) {
IsCA: params.CertificateAuthority,
}
if params.CertificateAuthority {
certTemplate.KeyUsage = certTemplate.KeyUsage | x509.KeyUsageCertSign
certTemplate.KeyUsage |= x509.KeyUsageCertSign
}
hostnames := expandHostnames(params.Hostnames)
for _, hostname := range hostnames {

View File

@ -96,6 +96,7 @@ func TestBadValues(t *testing.T) {
{"validity duration is required, otherwise the certificate would immediately expire", "pub.cert", "priv.key", &Params{Hostnames: []string{"127.0.0.1"}, RSAKeyLength: 2048}},
}
for _, testCase := range testCases {
testCase := testCase
t.Run(testCase.errorString, func(t *testing.T) {
err := CreateCertificateAndPrivateKeyFiles(
testCase.pub,
@ -129,6 +130,7 @@ func TestExpandHostnames(t *testing.T) {
},
}
for _, testCase := range testCases {
testCase := testCase
t.Run(fmt.Sprintf("expandHostnames(%s) => %s", testCase.input, testCase.expected), func(t *testing.T) {
assert := assert.New(t)
actual := expandHostnames(testCase.input)

View File

@ -127,8 +127,9 @@ func ReapNamespaces(params *Params) error {
return errors.Wrap(err, "listing namespace from kubernetes cluster failed")
}
for _, namespace := range namespaces.Items {
if !isOrphaned(namespace, params) {
for i := 0; i < len(namespaces.Items); i++ {
namespace := namespaces.Items[i]
if !isOrphaned(&namespace, params) {
log.Printf("skipping namespace %s created at %s", namespace.ObjectMeta.Name, namespace.ObjectMeta.CreationTimestamp)
continue
}
@ -150,7 +151,7 @@ func fileExists(name string) bool {
return err == nil
}
func isOrphaned(namespace corev1.Namespace, params *Params) bool {
func isOrphaned(namespace *corev1.Namespace, params *Params) bool {
deadline := time.Now().Add(-1 * params.Age).UTC()
return namespace.ObjectMeta.CreationTimestamp.Time.Before(deadline) && strings.HasPrefix(namespace.ObjectMeta.Name, "open-match")
}

View File

@ -113,8 +113,9 @@ func TestIsOrphaned(t *testing.T) {
},
}
for _, tc := range testCases {
tc := tc
t.Run(fmt.Sprintf("%s deleted= %t", tc.namespace.ObjectMeta.Name, tc.expected), func(t *testing.T) {
actual := isOrphaned(tc.namespace, &Params{
actual := isOrphaned(&tc.namespace, &Params{
Age: tc.age,
})
if actual != tc.expected {