Datastore refactor and added postgres tests #259
Conversation
There was a problem hiding this comment.
It is basically implementing the solution, but I do see some problems:
1 - It seems it does not handle concurrent cases correctly (two requests with the similar params might trigger two databases writes).
2 - I think it is worth double checking fnctl - I know it tests against 400. So it lacks tests for 409s.
| return | ||
| } | ||
|
|
||
| app, err = Api.Datastore.StoreApp(wapp.App) |
There was a problem hiding this comment.
What does it happen when two competing requests execute the same request in this case?
| return | ||
| } | ||
| if app != nil { | ||
| c.JSON(http.StatusConflict, simpleError(models.ErrAppsAlreadyExists)) |
There was a problem hiding this comment.
How would the client libraries react to 409? Have you tested fnctl? I think it might need some extra tests there to handle 409 responses.
pedronasser
changed the title
pedronasser
changed the title
pedronasser
force-pushed
the
fix-apps-routes-api
branch
from
ad57a80 to
ef83ce3
Compare
pedronasser
force-pushed
the
fix-apps-routes-api
branch
from
19e60f3 to
0d14f00
Compare
| @@ -14,15 +14,18 @@ build-docker: | |||
| docker build -t iron/functions:latest . | |||
|
|
|||
| test: | |||
| go test -v $(shell glide nv | grep -v examples | grep -v tool | grep -v fnctl) | |||
| go test -v $(shell go list ./... | grep -v vendor | grep -v examples | grep -v tool | grep -v fnctl) | |||
There was a problem hiding this comment.
Why the replacement between glide and go list?
There was a problem hiding this comment.
Because glide nv doesn't get the list of all packages.
In the test-docker I had to ignore the datastore package.
In this command we could continue using glide nv but I think is good to use the same.
There was a problem hiding this comment.
In this command we could continue using glide nv but I think is good to use the same.
If we could, then again, why change?
| test-docker: | ||
| docker run -ti --privileged --rm -e LOG_LEVEL=debug \ | ||
| -v /var/run/docker.sock:/var/run/docker.sock \ | ||
| -v $(DIR):/go/src/github.com/iron-io/functions \ | ||
| -w /go/src/github.com/iron-io/functions iron/go:dev go test \ | ||
| -v $(shell glide nv | grep -v examples | grep -v tool | grep -v fnctl) | ||
| -v $(shell go list ./... | grep -v vendor | grep -v examples | grep -v tool | grep -v fnctl | grep -v datastore) |
There was a problem hiding this comment.
glide nv will list all (n)on-(v)endored dependencies... See previous comment.
| return err | ||
| } | ||
|
|
||
| // Update app fields |
There was a problem hiding this comment.
Is this a necessary comment?
| if err != nil { | ||
| return err | ||
| } | ||
| // Update route fields |
| @@ -21,15 +21,16 @@ func setLogBuffer() *bytes.Buffer { | |||
| return &buf | |||
| } | |||
|
|
|||
| const tmpBolt = "/tmp/func_test_bolt.db" | |||
There was a problem hiding this comment.
any reason for not using ioutil.TempFile?
| @@ -125,17 +182,29 @@ func (ds *PostgresDatastore) GetApp(name string) (*models.App, error) { | |||
| json.Unmarshal([]byte(config), &res.Config) | |||
|
|
|||
| if err != nil { | |||
There was a problem hiding this comment.
if err == sql.ErrNoRows {
return nil, models.ErrAppsNotFound
} else if err != nil {
return nil, err
}
| } | ||
| } | ||
|
|
||
| func TestPostgres(t *testing.T) { |
There was a problem hiding this comment.
This provides appropriate coverage, although it is just a very long test. We're Go1.7 now, could we perhaps refactor it into subtests?
There was a problem hiding this comment.
@ccirello Can we do that in another PR?
I'm also thinking about moving theses tests inside their directories.
Like: moving postgres_test.go to postgres dir.
| @@ -53,22 +53,23 @@ func handleRouteCreate(c *gin.Context) { | |||
| } | |||
|
|
|||
| if app == nil { | |||
| // Create a new application and add the route to that new application | |||
There was a problem hiding this comment.
Is this a necessary comment?
| @@ -185,6 +186,8 @@ func (s *Server) bindHandlers() { | |||
| engine.NoRoute(handleSpecial) | |||
| } | |||
|
|
|||
| var ErrInternalServerError = errors.New("Something unexpected happened on the server") | |||
There was a problem hiding this comment.
Not anymore, I'll remove it
fixes #252 #253 #254