feat: HTTP integration tests #51
Conversation
Pull Request Test Coverage Report for Build #83
💛 - Coveralls |
|
|
||
| import "github.com/aerogear/aerogear-app-metrics/pkg/mobile" | ||
|
|
||
| func GetEmptyDataMetric() mobile.Metric { |
There was a problem hiding this comment.
Any reason not to return a pointer to mobile.Metric here (and in the following functions)?
There was a problem hiding this comment.
Considering it's only used for tests I'd argue perf/memory usage is not a big issue here.
There was a problem hiding this comment.
I also wanted to make explicit that these would return a new instance instead of a pointer to something existing, due to the possibility of tests eventually modifying them
|
|
||
| res, err := http.Post(s.URL+"/metrics", "application/json", buffer) | ||
| assert.NoError(t, err, "did not expect an error posting to /metrics") | ||
| defer res.Body.Close() |
There was a problem hiding this comment.
Body is not read here, do you really need to close it?
| } | ||
| assert.Nil(t, err, "did not expect an error posting metrics") | ||
| _, err = ioutil.ReadAll(res.Body) |
There was a problem hiding this comment.
should you add a defer res.Body.Close() here?
There was a problem hiding this comment.
Ok, i see you're closing it after using the function
There was a problem hiding this comment.
I think it should be okay to defer close from this function as long as the rest of the tests don't want to use the contents of the body, I'll do this 👍
cmd/metrics-api/metrics-api.go
Outdated
| config := config.GetConfig() | ||
| metricsDao := setup.InitDao(config) |
There was a problem hiding this comment.
Some minor points:
- This has created an unnecessary dependency between the setup package and the config package.
- Non db related options are being passed into a function called
InitDao
I think the config struct should stay internal and the config package should only be depended upon at a top level. I would change this to setup.InitDao(config.DBConnectionString, config.DBMaxConnections) to avoid such a dependency. Would you agree?
paolobueno
force-pushed
the
AEROGEAR-2251-integration-tests
branch
from
e27465b
to
9833c6c
Compare
Description
Add integration tests that utilize the whole HTTP setup.
JIRA: https://issues.jboss.org/browse/AEROGEAR-2251
Additional Notes
Moved most of the initialization code to
internal/setup, following guidelines in https://github.com/golang-standards/project-layout#internalMoved some of the fixtures to the new
test.fixturespackage for sharing. Would love to iterate on the way this is done.