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

solumos · 2017-09-28T02:17:04Z

No description provided.

kevinbirch

the mock stuff is fine for what it is, but I prefer different approach. I'll discuss with you IRL.

kevinbirch · 2017-09-29T21:53:51Z

log/logger.go

@@ -1,4 +1,4 @@
-package log
+package logx


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

kevinbirch · 2017-09-29T21:55:01Z

context/context.go

@@ -14,14 +14,30 @@ const (
 	LoggerKey = "ContextLoggerKey"
 )

+type IContext interface {


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

also, this isn't used...

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)

kevinbirch · 2017-09-29T21:55:49Z

context/context.go

 }

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


what's this for?

@kevinbirch It's a compile-time check that Context implements IContext

https://stackoverflow.com/questions/13194272/what-does-a-underscore-and-interface-name-after-keyword-var-in-golang-mean

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

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

kevinbirch · 2017-09-29T22:02:54Z

context/context_test.go

+	"github.com/stretchr/testify/mock"
+)
+
+type FakeUser struct {


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} }

kevinbirch · 2017-09-29T22:12:51Z

context/context.go

+	context.Context
+}
+
+type ILoggableContext interface {


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

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

kevinbirch · 2017-10-09T21:48:36Z

Makefile

+BUILD_DIR := build
+COVERAGE_DIR := $(BUILD_DIR)/coverage
+
+SHISA_PKGS := $(shell go list ./... | grep -Ev 'examples|test')


I think we would want to have test coverage for the test helpers. For example, the net/http/httptest package has test for those helpers.

I agree, but as we just talked about, subpackages get tricky to test with the way we're generating the coverage reports

kevinbirch · 2017-10-09T21:50:45Z

Makefile

+COVERAGE_DIR := $(BUILD_DIR)/coverage
+
+SHISA_PKGS := $(shell go list ./... | grep -Ev 'examples|test')
+SHISA_TEST_PKGS := $(addprefix coverage/,$(SHISA_PKGS))


Did you have a chance to look into generating a unified coverage report that includes cross-package coverage? We discussed taking some examples from this golang issue

as we just discussed, it might make sense to handle incidental coverage in a subsequent PR

kevinbirch · 2017-10-09T22:42:32Z

.circleci/config.yml

@@ -20,7 +21,7 @@ jobs:
      - run:
          name: Validate Markdown
          command: |
-            mdl $(find . -type f -name '*.md')
+            mdl $(find . -path ./vendor -prune -o -type f -name '*.md' -print)


N.B. ignoring the vendor/ path for mdl

kevinbirch · 2017-10-09T22:43:17Z

Gopkg.toml

+
+[[constraint]]
+  name = "github.com/stretchr/testify"
+  revision = "890a5c3458b43e6104ff5da8dfa139d013d77544"


N.B. explicitly using the latest commit since they haven't made a release in a loooong time

kevinbirch · 2017-10-09T22:44:32Z

Makefile

+test: ${COVERAGE_DIR} ${SHISA_TEST_PKGS}
+
+coverage/%:
+	go test -v -coverprofile=$(TOP_DIR)/$(COVERAGE_DIR)/$(@F)_coverage.out -covermode=atomic github.com/percolate/shisa/$(@F)


N.B. using full package path instead of changing into the package directory

solumos added 2 commits September 27, 2017 22:14

add context tests and allow context to build

9dbbff4

add tests to env package

3a50e21

solumos requested a review from kevinbirch September 28, 2017 22:20

solumos self-assigned this Sep 28, 2017

solumos added 4 commits September 29, 2017 17:00

fix formatting and reference to logx

853b4a4

update Makefile/circle conf/gitignore for coverage

b836a52

create symlinks in circleci build job

f36b503

fix whitespace in circleci config

5d692ab

kevinbirch requested changes Sep 29, 2017

View reviewed changes

solumos and others added 8 commits October 5, 2017 16:23

update codecov script

45009d7

add charlatan mocks, use them in tests, and logx-> log

3d231b8

Merge branch 'master' into add-tests

46a0422

remove cci symlinks

ae49285

update tests to track master change

5d8c464

add explicit package to go test recipe

0d6f709

vendor github.com/stretchr/testify

90d3885

ignore vendor/* for markdown validation

d734f3c

kevinbirch requested changes Oct 9, 2017

View reviewed changes

kevinbirch approved these changes Oct 10, 2017

View reviewed changes

solumos merged commit d5dd716 into master Oct 10, 2017

solumos deleted the add-tests branch October 10, 2017 18:20

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

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

Uh oh!

Conversation

solumos commented Sep 28, 2017

Uh oh!

kevinbirch left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

solumos Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

solumos Oct 5, 2017 •

edited

Loading