Skip to content

Conversation

@zingales
Copy link
Collaborator

No description provided.

Copy link

@ethomas2 ethomas2 left a comment

Choose a reason for hiding this comment

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

Test stuff looks good overall to me. Minor comments

mockDb := &DiyMockDataStore{}
env := &handlers.Environment{mockDb}

handlers.SetEnvironment(env)

Choose a reason for hiding this comment

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

Not fan of this global environment. I realize you're following the same patter that you had with databaseutil.

For one thing, if you ever set your tests to run in parallel (which they aren't right now) setting this global env isn't threadsafe. Test1 might overwrite Test2's env in the middle of test 2 running. Even worse, two tests might try to write to write to the global env at the same time and clobber each other's writes. You'll get some garbage struct for your env

To make your tests run in parallel, add t.Parallel() to the top of every test
https://golang.org/pkg/testing/#T.Parallel

To have go test detect race conditions like these, run it with go test -race

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm open to suggestions. I just don't know how to create non-end to end tests without mocking a db. And i'm not sure how to do that without A) having a global variable or B) passing in the environment object to all function calls. Which just gets annoying.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I guess i could make all the handlers functions, methods on the environment struct, but i'm not a fan of that.

server := httptest.NewServer(routers.DefineRoutes())
defer server.Close()

resp, err := http.Get(server.URL)

Choose a reason for hiding this comment

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

It's good practice to always defer resp.Body.Close() after making an http request
https://stackoverflow.com/questions/23928983/defer-body-close-after-receiving-response

If the returned error is nil, the Response will contain a non-nil Body which
the user is expected to close. If the Body is not closed, the Client's
underlying RoundTripper (typically Transport) may not be able to re-use a
persistent TCP connection to the server for a subsequent "keep-alive" request.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Even if i don't use the respone.body?

@ethomas2
Copy link

btw, there's a library out there which will take an interface and auto generate a mock struct that implements that interface

https://github.com/vektra/mockery

@zingales zingales changed the title Testframework Mock Db test framework Sep 17, 2018
@zingales zingales changed the title Mock Db test framework Mock Db test framework Option A Sep 17, 2018
@zingales
Copy link
Collaborator Author

closed in favor of #39

@zingales zingales closed this Sep 19, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants