Aim namespace tests for apps and dashboards #610
Conversation
suprjinx
changed the title
| for _, nsCode := range []string{"default", tt.namespace1Code, tt.namespace2Code} { | ||
| // ignore errors here since default exists on first run | ||
| //nolint:errcheck | ||
| _, _ = s.NamespaceFixtures.CreateNamespace(context.Background(), &models.Namespace{ |
There was a problem hiding this comment.
can we please in any case check all errors?
There was a problem hiding this comment.
yep and it will be really nice to clean and then start.
| require.Nil( | ||
| s.T(), | ||
| s.AIMClient().WithMethod( | ||
| http.MethodGet, | ||
| ).WithNamespace( | ||
| namespace1Code, | ||
| ).WithResponseType( | ||
| helpers.ResponseTypeJSON, | ||
| ).WithResponse( | ||
| &resp, | ||
| ).DoRequest( | ||
| "/apps", | ||
| ), | ||
| ) | ||
| // only app 1 should be present | ||
| assert.Equal(s.T(), 1, len(resp)) | ||
| assert.Equal(s.T(), app1ID, resp[0].ID) |
There was a problem hiding this comment.
why you didn't wrap it into method?
There was a problem hiding this comment.
extracted methods for the "positive" tests, but left error expectations in the main test method (awkward to share a method for this)
|
|
||
| // IDs from other namespace cannot be fetched, updated, or deleted | ||
| errResp := response.Error{} | ||
| client := s.AIMClient() |
There was a problem hiding this comment.
why you create each time instance? please merge main if you haven't done this yet
There was a problem hiding this comment.
i need to hold instance of client to retrieve status code for assertion
| client.WithMethod( | ||
| http.MethodGet, | ||
| ).WithNamespace( |
There was a problem hiding this comment.
you don't need to specify method anymore. GET is by default.
There was a problem hiding this comment.
in this case it makes it clear we are testing the get method, yes? if implicit, that is not so obvious from reading test.
There was a problem hiding this comment.
but that's why we have default values inside to do not overwrite/set each time them like probably many other packages in go or any other language.
There was a problem hiding this comment.
the other tests do have to indicate the http method, so i think it's reasonable to include here for symmetry.
| ).WithResponse( | ||
| &errResp, | ||
| ).DoRequest( | ||
| fmt.Sprintf("/apps/%s", app1ID), |
There was a problem hiding this comment.
I just that we don't need to use fmt.Sprintf. it will be done under the hood automatically =). I added this moment for just forgot.
There was a problem hiding this comment.
I think we should stick with what is expected by most programmers (use fmt.Sprintf for template string + values).
There was a problem hiding this comment.
but you can just pass DoRequest("/apps/%s", app1ID) like for example in errors.Errorf() function. You don't need to pass sprintf here, because sprintf is under the hood probably.
There was a problem hiding this comment.
That's a surprising behavior. Maybe add DoRequestf() to indicate templating is available?
| assert.Equal(s.T(), fiber.StatusOK, client.GetStatusCode()) | ||
| } | ||
|
|
||
| func (s *AppFlowTestSuite) createApp(namespace string, req *request.CreateApp) string { |
There was a problem hiding this comment.
please rename methods where you compare to have AndCompare in the name so we can understand what is going on.
This PR includes new "flow" tests for Aim Apps and Dashboards with particular testing of namespace scoping.
Also, the Apps endpoints were not filtering by namespace for GET, PUT, and DELETE -- added where clauses for this.
Fixes #609