Skip to content

Commit

Permalink
patch: remove forloop var pass by reference
Browse files Browse the repository at this point in the history
Sucessful `testVerifyEnviromentRegistrars` with patch:

```
=== RUN   TestNewEnvironment
=== RUN   TestNewEnvironment/Verify_Multi_Registrar_Environment
--- PASS: TestNewEnvironment/Verify_Multi_Registrar_Environment (0.00s)
--- PASS: TestNewEnvironment (0.00s)
PASS
ok      github.com/xmidt-org/webpa-common/v2/service/consul     0.019s

```

Failure `testVerifyEnviromentRegistrars` with forloop bug:

```
panic:
assert: mock: The method has been called over 1 times.
        Either do one more Mock.On("Register").Return(...), or remove extra call.
        This call was unexpected:
                Register(*api.AgentServiceRegistration)
                0: &api.AgentServiceRegistration{Kind:"", ID:"api:deadbeef", Name:"deadbeef-api", Tags:[]string{"role=deadbeef, region=1"}, Port:443, Address:"deadbeef.com", SocketPath:"", TaggedAddresses:map[string]api.ServiceAddress(nil), EnableTagOverride:false, Meta:map[string]string(nil), Weights:(*api.AgentWeights)(nil), Check:(*api.AgentServiceCheck)(nil), Checks:api.AgentServiceChecks(nil), Proxy:(*api.AgentServiceConnectProxyConfig)(nil), Connect:(*api.AgentServiceConnect)(nil), Namespace:"", Partition:"", Locality:(*api.Locality)(nil)}
        at: [/Users/odc/Documents/GitHub/xmidt-org/webpa-common/service/consul/mocks_test.go:60 /Users/odc/go/pkg/mod/github.com/go-kit/kit@v0.13.0/sd/consul/registrar.go:30 /Users/odc/Documents/GitHub/xmidt-org/webpa-common/service/registrars.go:13 /Users/odc/Documents/GitHub/xmidt-org/webpa-common/service/environment.go:198 /Users/odc/Documents/GitHub/xmidt-org/webpa-common/service/consul/environment_test.go:191] [recovered]
        panic:
assert: mock: The method has been called over 1 times.
        Either do one more Mock.On("Register").Return(...), or remove extra call.
        This call was unexpected:
                Register(*api.AgentServiceRegistration)
                0: &api.AgentServiceRegistration{Kind:"", ID:"api:deadbeef", Name:"deadbeef-api", Tags:[]string{"role=deadbeef, region=1"}, Port:443, Address:"deadbeef.com", SocketPath:"", TaggedAddresses:map[string]api.ServiceAddress(nil), EnableTagOverride:false, Meta:map[string]string(nil), Weights:(*api.AgentWeights)(nil), Check:(*api.AgentServiceCheck)(nil), Checks:api.AgentServiceChecks(nil), Proxy:(*api.AgentServiceConnectProxyConfig)(nil), Connect:(*api.AgentServiceConnect)(nil), Namespace:"", Partition:"", Locality:(*api.Locality)(nil)}
        at: [/Users/odc/Documents/GitHub/xmidt-org/webpa-common/service/consul/mocks_test.go:60 /Users/odc/go/pkg/mod/github.com/go-kit/kit@v0.13.0/sd/consul/registrar.go:30 /Users/odc/Documents/GitHub/xmidt-org/webpa-common/service/registrars.go:13 /Users/odc/Documents/GitHub/xmidt-org/webpa-common/service/environment.go:198 /Users/odc/Documents/GitHub/xmidt-org/webpa-common/service/consul/environment_test.go:191]
```
  • Loading branch information
denopink committed Nov 11, 2024
1 parent 7a78cc9 commit 01cd46e
Show file tree
Hide file tree
Showing 4 changed files with 70 additions and 10 deletions.
2 changes: 1 addition & 1 deletion service/consul/environment.go
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ func newRegistrars(l *adapter.Logger, registrationScheme string, c gokitconsul.C
rid := zap.String("id", registration.ID)
in := zap.String("instance", instance)
l.Logger = l.Logger.With(rid, in)
consulRegistrar, err = NewRegistrar(c, u, &registration, l)
consulRegistrar, err = NewRegistrar(c, u, registration, l)
if err != nil {
return
}
Expand Down
60 changes: 60 additions & 0 deletions service/consul/environment_test.go
Original file line number Diff line number Diff line change
@@ -1,13 +1,15 @@
package consul

import (
"reflect"
"testing"

"github.com/hashicorp/consul/api"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
"github.com/stretchr/testify/require"
"github.com/xmidt-org/webpa-common/v2/adapter"

"github.com/xmidt-org/webpa-common/v2/service"
)

Expand Down Expand Up @@ -136,8 +138,66 @@ func testNewEnvironmentFull(t *testing.T) {
ttlUpdater.AssertExpectations(t)
}

func testVerifyEnviromentRegistrars(t *testing.T) {
var (
require = require.New(t)
logger = adapter.DefaultLogger()
clientFactory = prepareMockClientFactory()
client = new(mockClient)
ttlUpdater = new(mockTTLUpdater)
registrations = []api.AgentServiceRegistration{
{
ID: "deadbeef",
Name: "deadbeef",
Tags: []string{"role=deadbeef, region=1"},
Address: "deadbeef.com",
Port: 8080,
},
{
ID: "api:deadbeef",
Name: "deadbeef-api",
Tags: []string{"role=deadbeef, region=1"},
Address: "deadbeef.com",
Port: 443,
},
}
co = Options{
Client: &api.Config{
Address: "localhost:8500",
Scheme: "https",
},
Registrations: registrations,
}
)

clientFactory.On("NewClient", mock.MatchedBy(func(*api.Client) bool { return true })).Return(client, ttlUpdater).Once()

client.On("Register", mock.MatchedBy(func(r *api.AgentServiceRegistration) bool {
return reflect.DeepEqual(registrations[0], *r)
})).Return(error(nil)).Once()
client.On("Register", mock.MatchedBy(func(r *api.AgentServiceRegistration) bool {
return reflect.DeepEqual(registrations[1], *r)
})).Return(error(nil)).Once()

client.On("Deregister", mock.MatchedBy(func(r *api.AgentServiceRegistration) bool {
return reflect.DeepEqual(registrations[0], *r)
})).Return(error(nil)).Once()
client.On("Deregister", mock.MatchedBy(func(r *api.AgentServiceRegistration) bool {
return reflect.DeepEqual(registrations[1], *r)
})).Return(error(nil)).Once()
consulEnv, err := NewEnvironment(logger, service.DefaultScheme, co)
require.NoError(err)

consulEnv.Register()
consulEnv.Deregister()
clientFactory.AssertExpectations(t)
ttlUpdater.AssertExpectations(t)
client.AssertExpectations(t)
}

func TestNewEnvironment(t *testing.T) {
t.Run("Empty", testNewEnvironmentEmpty)
t.Run("ClientError", testNewEnvironmentClientError)
t.Run("Full", testNewEnvironmentFull)
t.Run("Verify Multi Registrar Environment", testVerifyEnviromentRegistrars)
}
4 changes: 2 additions & 2 deletions service/consul/registrar.go
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ type ttlRegistrar struct {
}

// NewRegistrar creates an sd.Registrar, binding any TTL checks to the Register/Deregister lifecycle as needed.
func NewRegistrar(c gokitconsul.Client, u ttlUpdater, r *api.AgentServiceRegistration, logger *adapter.Logger) (sd.Registrar, error) {
func NewRegistrar(c gokitconsul.Client, u ttlUpdater, r api.AgentServiceRegistration, logger *adapter.Logger) (sd.Registrar, error) {
var (
ttlChecks []ttlCheck
err error
Expand All @@ -148,7 +148,7 @@ func NewRegistrar(c gokitconsul.Client, u ttlUpdater, r *api.AgentServiceRegistr
}
}

var registrar sd.Registrar = gokitconsul.NewRegistrar(c, r, logger)
var registrar sd.Registrar = gokitconsul.NewRegistrar(c, &r, logger)

// decorate the given registrar if we have any TTL checks
if len(ttlChecks) > 0 {
Expand Down
14 changes: 7 additions & 7 deletions service/consul/registrar_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ func testNewRegistrarNoChecks(t *testing.T) {
ttlUpdater = new(mockTTLUpdater)
tickerFactory = prepareMockTickerFactory()

registration = &api.AgentServiceRegistration{
registration = api.AgentServiceRegistration{
ID: "service1",
Address: "somehost.com",
Port: 1111,
Expand Down Expand Up @@ -80,7 +80,7 @@ func testNewRegistrarNoTTL(t *testing.T) {
ttlUpdater = new(mockTTLUpdater)
tickerFactory = prepareMockTickerFactory()

registration = &api.AgentServiceRegistration{
registration = api.AgentServiceRegistration{
ID: "service1",
Address: "somehost.com",
Port: 1111,
Expand Down Expand Up @@ -131,7 +131,7 @@ func testNewRegistrarCheckMalformedTTL(t *testing.T) {
ttlUpdater = new(mockTTLUpdater)
tickerFactory = prepareMockTickerFactory()

registration = &api.AgentServiceRegistration{
registration = api.AgentServiceRegistration{
ID: "service1",
Address: "somehost.com",
Port: 1111,
Expand Down Expand Up @@ -161,7 +161,7 @@ func testNewRegistrarCheckTTLTooSmall(t *testing.T) {
ttlUpdater = new(mockTTLUpdater)
tickerFactory = prepareMockTickerFactory()

registration = &api.AgentServiceRegistration{
registration = api.AgentServiceRegistration{
ID: "service1",
Address: "somehost.com",
Port: 1111,
Expand Down Expand Up @@ -191,7 +191,7 @@ func testNewRegistrarChecksMalformedTTL(t *testing.T) {
ttlUpdater = new(mockTTLUpdater)
tickerFactory = prepareMockTickerFactory()

registration = &api.AgentServiceRegistration{
registration = api.AgentServiceRegistration{
ID: "service1",
Address: "somehost.com",
Port: 1111,
Expand Down Expand Up @@ -223,7 +223,7 @@ func testNewRegistrarChecksTTLTooSmall(t *testing.T) {
ttlUpdater = new(mockTTLUpdater)
tickerFactory = prepareMockTickerFactory()

registration = &api.AgentServiceRegistration{
registration = api.AgentServiceRegistration{
ID: "service1",
Address: "somehost.com",
Port: 1111,
Expand Down Expand Up @@ -272,7 +272,7 @@ func testNewRegistrarTTL(t *testing.T) {
close(update2Done)
}

registration = &api.AgentServiceRegistration{
registration = api.AgentServiceRegistration{
ID: "service1",
Address: "somehost.com",
Port: 1111,
Expand Down

0 comments on commit 01cd46e

Please sign in to comment.