Skip to content

[API-858] Add Unit Tests to Baseline Code #2

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 14 commits into from
Oct 10, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions context/context.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,30 @@ const (
LoggerKey = "ContextLoggerKey"
)

type IContext interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interfaces in go shouldn't be named ISomething they should be named Somethinger instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also, this isn't used...

Copy link
Contributor Author

@solumos solumos Oct 5, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, for one-method interfaces. However, there doesn't seem to be an official "go" consensus beyond that (https://talks.golang.org/2014/names.slide#14)

Since it's just an interface that only embeds another interface, it's frivolous anyway (see https://github.com/golang/go/wiki/CodeReviewComments#interfaces)

context.Context
}

type ILoggableContext interface {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this isn't used, I don't thing we need this...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, I think I got caught in this anti-pattern - https://github.com/golang/go/wiki/CodeReviewComments#interfaces

IContext
Info(string)
Infof(string, ...interface{})
Error(string)
Errorf(string, ...interface{})
Trace(string)
Tracef(string, ...interface{})
}

type Context struct {
context.Context
RequestID string
Actor models.User
Logger log.Logger
Logger logx.Logger
}

func New(parent context.Context, id string, actor models.User, log log.Logger) *Context {
var _ IContext = &Context{}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's this for?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL. but, do we need it this extra interface?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No - I'm going to push an updated version shortly


func New(parent IContext, id string, actor models.User, log logx.Logger) *Context {
return &Context{
Context: parent,
RequestID: id,
Expand Down
261 changes: 261 additions & 0 deletions context/context_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,261 @@
package context

import (
"testing"
"time"
"context"

"github.com/percolate/shisa/log"
"github.com/percolate/shisa/models"
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/mock"
)

type FakeUser struct {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A fake for testing is going to be useful elsewhere, so could you move this into models/test/user.go that is package modeltest? Then you can have:

(models/test/user.go)

package modeltest

import (
	"github.com/percolate/shisa/models"
)
type FakeUser struct {
	id string
}

func (u *FakeUser) ID() string {
	return u.id
}

func MakeUser(id string) models.User {
	return &FakeUser{id: id}
}

models.User
}

type FakeLogger struct {
logx.Logger
mock.Mock
}

type FakeContext struct {
context.Context
mock.Mock
key, val interface{}
}

func (u *FakeUser) ID() string {
return "123456"
}

func (u *FakeUser) String() string {
return u.ID()
}

func (l *FakeLogger) Info(requestID, message string) {
l.Called(requestID, message)
}

func (l *FakeLogger) Infof(requestID, format string, args ...interface{}) {
l.Called(requestID, format, args[0])
}

func (l *FakeLogger) Error(requestID, message string) {
l.Called(requestID, message)
}

func (l *FakeLogger) Errorf(requestID, format string, args ...interface{}) {
l.Called(requestID, format, args[0])
}

func (l *FakeLogger) Trace(requestID, message string) {
l.Called(requestID, message)
}

func (l *FakeLogger) Tracef(requestID, format string, args ...interface{}) {
l.Called(requestID, format, args[0])
}

func (c *FakeContext) Deadline() (deadline time.Time, ok bool) {
c.Called()
return
}

func (c *FakeContext) Done() <-chan struct{} {
c.Called()
d := make(chan struct{})
go func() {
d <- struct{}{}
close(d)
}()
return d
}

func (c *FakeContext) Err() error {
c.Called()
return nil
}

func (c *FakeContext) Value(key interface{}) interface{} {
c.Called(key)
return c.val
}


func createParentContext() *FakeContext {
return &FakeContext{
Context: context.Background(),
}
}

func getContextForLogging(requestID string, logger *FakeLogger) *Context {
actor := &FakeUser{}
parent := createParentContext()

c := New(parent, requestID, actor, logger)

return c
}

func getContextForParent(parent *FakeContext) *Context {
actor := &FakeUser{}
logger := &FakeLogger{}

c := New(parent, "999999", actor, logger)

return c
}

func TestNew(t *testing.T) {
actor := &FakeUser{}
logger := &FakeLogger{}
parent := createParentContext()
id := "555"

c := New(parent, id, actor, logger)

assert.Equal(t, parent, c.Context)
assert.Equal(t, id, c.RequestID)
assert.Equal(t, actor, c.Actor)
assert.Equal(t, logger, c.Logger)
}

func TestInfo(t *testing.T) {
logtype := "Info"
requestID := "555"
message := "Hello test"
logger := &FakeLogger{}
logger.On(logtype, requestID, message).Return()
c := getContextForLogging(requestID, logger)

c.Info(message)

logger.AssertCalled(t, logtype, requestID, message)
}

func TestInfof(t *testing.T) {
logtype := "Infof"
requestID := "555"
format := "Hello test %s"
name := "name"
logger := &FakeLogger{}
logger.On(logtype, requestID, format, name).Return()
c := getContextForLogging(requestID, logger)

c.Infof(format, name)

logger.AssertCalled(t, logtype, requestID, format, name)
}

func TestError(t *testing.T) {
logtype := "Error"
requestID := "555"
message := "Hello test"
logger := &FakeLogger{}
logger.On(logtype, requestID, message).Return()
c := getContextForLogging(requestID, logger)

c.Error(message)

logger.AssertCalled(t, logtype, requestID, message)
}

func TestErrorf(t *testing.T) {
logtype := "Errorf"
requestID := "555"
format := "Hello test %s"
name := "name"
logger := &FakeLogger{}
logger.On(logtype, requestID, format, name).Return()
c := getContextForLogging(requestID, logger)

c.Errorf(format, name)

logger.AssertCalled(t, logtype, requestID, format, name)
}

func TestTrace(t *testing.T) {
logtype := "Trace"
requestID := "555"
message := "Hello test"
logger := &FakeLogger{}
logger.On(logtype, requestID, message).Return()
c := getContextForLogging(requestID, logger)

c.Trace(message)

logger.AssertCalled(t, logtype, requestID, message)
}

func TestTracef(t *testing.T) {
logtype := "Tracef"
requestID := "555"
format := "Hello test %s"
name := "name"
logger := &FakeLogger{}
logger.On(logtype, requestID, format, name).Return()
c := getContextForLogging(requestID, logger)

c.Tracef(format, name)

logger.AssertCalled(t, logtype, requestID, format, name)
}

func TestDeadline(t *testing.T) {
parent := &FakeContext{}
c := getContextForParent(parent)
defaulttime := time.Time{}
defaultok := false
parent.On("Deadline").Return(defaulttime, defaultok)

d, y := c.Deadline()

parent.AssertCalled(t, "Deadline")
assert.Equal(t, d, defaulttime)
assert.Equal(t, y, defaultok)
}

func TestDone(t *testing.T) {
parent := &FakeContext{}
c := getContextForParent(parent)
parent.On("Done").Return(struct {}{})

d := <- c.Done()

parent.AssertCalled(t, "Done")
assert.Equal(t, d, struct{}{})
}

func TestErr(t *testing.T) {
parent := &FakeContext{}
c := getContextForParent(parent)
parent.On("Err").Return(nil)

err := c.Err()

parent.AssertCalled(t, "Err")
assert.Equal(t, err, nil)
}

func TestValueID(t *testing.T) {
pkey := "ParentKey"
pval := true
parent := &FakeContext{
key: pkey,
val: pval,
}
parent.On("Value", pkey).Return(pval)
c := getContextForParent(parent)

idVal := c.Value(IDKey)
actorVal := c.Value(ActorKey)
loggerVal := c.Value(LoggerKey)
parentVal := c.Value(pkey)

assert.Equal(t, idVal, c.RequestID)
assert.Equal(t, actorVal, c.Actor)
assert.Equal(t, loggerVal, c.Logger)
assert.Equal(t, parentVal, pval)
}
2 changes: 1 addition & 1 deletion log/logger.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package log
package logx
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This package should be called log. The other name was a holdover from PAM


import (
"fmt"
Expand Down
2 changes: 1 addition & 1 deletion log/logwriter.go
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
package log
package logx

import (
"fmt"
Expand Down
4 changes: 3 additions & 1 deletion log/null.go
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
package log
package logx

import "log"

type null struct{}

type SentryMetadata struct{}

var (
NULL = new(null)
nullLogger = log.New(NULL, "", 0)
Expand Down