chartsvc: Unit and integration tests #329
Conversation
There was a problem hiding this comment.
thanks @sameersbn, lgtm in general. A few suggestions for some more tests we could add.
| @@ -95,39 +95,39 @@ func listRepoCharts(w http.ResponseWriter, req *http.Request, params Params) { | |||
| func getChart(w http.ResponseWriter, req *http.Request, params Params) { | |||
| db, closer := dbSession.DB() | |||
| defer closer() | |||
| var chart *models.Chart | |||
There was a problem hiding this comment.
what's the reasoning behind these pointer to value changes?
There was a problem hiding this comment.
I was having trouble getting the mock interface match the expectation description. Simplest was to fix this was to remove the double reference.
mock: Unexpected Method Call
-----------------------------
One(**models.Chart)
0: (**models.Chart)(0xc4200b41a8)
The closest call I have is:
One(*models.Chart)
0: &models.Chart{ID:"", Name:"", Repo:models.Repo{Name:"", URL:""}, Description:"", Home:"", Keywords:[]string(nil), Maintainers:[]models.Maintainer(nil), Sources:[]string(nil), Icon:"", RawIcon:[]uint8(nil), ChartVersions:[]models.ChartVersion(nil)}There was a problem hiding this comment.
hmm okay, I think the tests in monocular had a similar thing though, but it's not a big problem either way.
cmd/chartsvc/handler_test.go
Outdated
| assert.Equal(t, cResponse.Type, "chart", "response type is chart") | ||
| assert.Equal(t, cResponse.ID, tt.chart.ID, "chart ID should be the same") | ||
| assert.Equal(t, cResponse.Attributes.(models.Chart).ChartVersions, tt.chart.ChartVersions, "chart version in the response should be the same") | ||
| }) |
There was a problem hiding this comment.
can we check that the latestChartVersion relationship points to tt.chart.ChartVersions[0]?
cmd/chartsvc/handler_test.go
Outdated
| cvResponse := newChartVersionResponse(&tt.chart, tt.chart.ChartVersions[i]) | ||
| assert.Equal(t, cvResponse.Type, "chartVersion", "response type is chartVersion") | ||
| assert.Equal(t, cvResponse.ID, tt.chart.ID+"-"+tt.chart.ChartVersions[i].Version, "reponse id should have chart version suffix") | ||
| assert.Equal(t, cvResponse.Attributes.(models.ChartVersion).Version, tt.chart.ChartVersions[i].Version, "chart version in the response should be the same") |
There was a problem hiding this comment.
check the selfLink and "chart" relationship here
| name string | ||
| chart models.Chart | ||
| }{ | ||
| {"my-chart", models.Chart{ |
There was a problem hiding this comment.
add a test for no chart versions
|
updated the PR. Please review. |
|
thanks for the separate commits, was a lot easier to review! lgtm! |
Closes #301