Conversation
| global have_rbac_decorator | ||
|
|
||
| if ("airship_tempest_plugin/tests/api/rbac" in filename or | ||
| if ("airship_tempest_plugin/tests/api/shipyard/rbac" in filename or |
There was a problem hiding this comment.
it shouldn't be just for shipyard as more rbac tests for other services could be added later
There was a problem hiding this comment.
This is added to bypass the naming convention validation for function tests; Naming convention validation would be applicable for all rbac tests only.
There was a problem hiding this comment.
ok, but maybe use regex so it is not hardcoded to only shipyard.
There was a problem hiding this comment.
this still needs to be updated. nothing here should be hardcoded to shipyard
| P101 | ||
| """ | ||
| if "airship_tempest_plugin/tests/api" in filename: | ||
| if "airship_tempest_plugin/tests/api/shipyard/rbac" in filename: |
There was a problem hiding this comment.
it shouldn't be just for shipyard as more rbac tests for other services could be added later
There was a problem hiding this comment.
This is added to bypass the naming convention validation for the function tests; Naming convention validation would be applicable for all rbac tests only.
There was a problem hiding this comment.
ok, but maybe use regex so it is not hardcoded to only shipyard.
There was a problem hiding this comment.
this still needs to be updated. nothing here should be hardcoded to shipyard
| P102 | ||
| """ | ||
| if "airship_tempest_plugin/tests/api/rbac/" in filename: | ||
| if "airship_tempest_plugin/tests/api/shipyard/rbac" in filename: |
There was a problem hiding this comment.
it shouldn't be just for shipyard as more rbac tests for other services could be added later
There was a problem hiding this comment.
This is added to bypass the naming convention validation for function tests; Naming convention validation would be applicable for all rbac tests only.
There was a problem hiding this comment.
ok, but maybe use regex so it is not hardcoded to only shipyard.
There was a problem hiding this comment.
this still needs to be updated. nothing here should be hardcoded to shipyard
| P103 | ||
| """ | ||
| if "airship_tempest_plugin/tests/api" in filename: | ||
| if "airship_tempest_plugin/tests/api/shipyard/rbac" in filename: |
There was a problem hiding this comment.
this is regardless of rbac. this should be enforced for all classes that instantiate a service client
There was a problem hiding this comment.
Ok, Will be modified accordingly.
There was a problem hiding this comment.
this still needs to be updated. nothing here should be hardcoded to shipyard
| self.expected_success(200, resp.status) | ||
| body = json.loads(body) | ||
| return rest_client.ResponseBody(resp, body) | ||
| if isinstance(body, list): |
There was a problem hiding this comment.
as this is a single GET - meaning it gets a specific workflow- would it ever return a list?
There was a problem hiding this comment.
Modified accordingly as it's returning a specific workflow
|
|
||
| from tempest.lib.common import rest_client | ||
|
|
||
|
|
There was a problem hiding this comment.
is the extra line needed?
There was a problem hiding this comment.
I don't see it as removed. Perhaps I am not seeing the latest updates made?
| self.expected_success(200, resp.status) | ||
| body = json.loads(body) | ||
| return rest_client.ResponseBody(resp, body) | ||
| if isinstance(body, list): |
There was a problem hiding this comment.
is this needing when doing a get on a specific artifact/resource?
There was a problem hiding this comment.
This is returning a List only; therefore used "ResponseBodyList".
| self.expected_success(200, resp.status) | ||
| body = json.loads(body) | ||
| return rest_client.ResponseBody(resp, body) | ||
| if isinstance(body, list): |
There was a problem hiding this comment.
is this needing when doing a get on a specific artifact/resource?
There was a problem hiding this comment.
This is returning a List only; therefore used "ResponseBodyList".
| self.expected_success(200, resp.status) | ||
| body = json.loads(body) | ||
| return rest_client.ResponseBody(resp, body) | ||
| if isinstance(body, list): |
There was a problem hiding this comment.
is this needing when doing a get on a specific artifact/resource?
There was a problem hiding this comment.
This is returning a List only; therefore used "ResponseBodyList".
| # under the License. | ||
| # | ||
|
|
||
| import logging |
There was a problem hiding this comment.
Used "logging.getLogger()" instead of "print()"
There was a problem hiding this comment.
I am not seeing your updates...
| from tempest.lib import exceptions | ||
|
|
||
| CONF = config.CONF | ||
| LOG = logging.getLogger(__name__) |
|
|
||
| from airship_tempest_plugin.tests.api.shipyard import base | ||
|
|
||
| from tempest import config |
| from tempest.lib import decorators | ||
| from tempest.lib import exceptions | ||
|
|
||
| CONF = config.CONF |
| CONF = config.CONF | ||
| LOG = logging.getLogger(__name__) | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: DocumentStagingTests
|
|
||
| @decorators.idempotent_id('743ab304-be35-4e45-ad47-e124dce3b9dc') | ||
| def test_get_collection(self): | ||
| """Get Config docs, Successful with response status 200 |
There was a problem hiding this comment.
maybe update docstring so it is clear that it is getting a specific collection
| def test_get_collection_list(self): | ||
| """Config docs status, Successful with response status 200""" | ||
| response = self.shipyard_document_staging_client.\ | ||
| get_configdocs_status() |
There was a problem hiding this comment.
maybe we should reconsider the name of this API method: " get_configdocs_status()" ... there may be a reason why it contains "status" but I forgot why at the moment. Just something to consider
There was a problem hiding this comment.
"get_configdocs_status()" is a function in "document_staging_client.py" which is resulting a list of all configuration docs and this function has been called from "test_document_staging_rbac.py" also; therefore not modified the name.
| self.assertEqual(response.response['status'], '200') | ||
| except exceptions.NotFound: | ||
| print("The Shipyard buffer does not contain this collection") | ||
| pass |
There was a problem hiding this comment.
don't use print ...
I think the NotFound exception should be raised...
Under what scenario does:
collection_id = self._get_collection_id()
not return a valid collection_id ?
There was a problem hiding this comment.
Removed print and used Log instead.
Collection_id is returning a valid one but as per Shipyard API document, it will return "404 NotFound exception" if the Shipyard buffer does not contain this particular collection and considered this scenario as Pass , used "except exceptions.NotFound".
| response = self.shipyard_document_staging_client.\ | ||
| get_configdocs_version(collection_id, "buffer") | ||
| self.assertEqual(response.response['status'], '200') | ||
| except exceptions.NotFound: |
There was a problem hiding this comment.
same comment as above regarding the NotFound exception
There was a problem hiding this comment.
As per Shipyard API document, it will return "404 NotFound exception" if the Shipyard buffer does not contain this particular collection and considered this scenario as Pass , used "except exceptions.NotFound".
| Successful with response status 200 | ||
| """ | ||
| collection_id = self._get_collection_id() | ||
| response = self.shipyard_document_staging_client.\ |
There was a problem hiding this comment.
just curious why you selectively use the try/except logic when collection_id is being passed as an argument
| collection_id = self._get_collection_id() | ||
| response = self.shipyard_document_staging_client.\ | ||
| get_configdocs_version(collection_id, "successful_site_action") | ||
| self.assertEqual(response.response['status'], '200') |
There was a problem hiding this comment.
same comment as above; "just curious why you selectively use the try/except logic when collection_id is being passed as an argument"
There was a problem hiding this comment.
In this scenario, "404 NotFound exception" is not returned; therefore "NotFound exception is not used.
As per Shipyard API document, wherever "404 NotFound Exception" may be returned in case of unavailability of the intended result(resource like collection, buffer etc), try/exception is used for those scenarios only.
| Successful with response status 200 | ||
| """ | ||
| collection_id = self._get_collection_id() | ||
| response = self.shipyard_document_staging_client.\ |
| response = self.shipyard_document_staging_client.\ | ||
| get_renderedconfigdocs() | ||
| self.assertEqual(response.response['status'], '200') | ||
| except exceptions.NotFound: |
There was a problem hiding this comment.
why are the exceptions being silenced? If the resource doesn't exists, then a setup or teardown should be present. Tempest tests shouldn't rely on resources already existing.
There was a problem hiding this comment.
As per Shipyard API documentation, for some of the API call, there might "404 NotFound" exception be raised, therefore try/exception block has been used. As of now, there are only test cases related to GET API call, once POST API call test cases will be added, setup and teardown will definitely be introduced.
| response = self.shipyard_document_staging_client.\ | ||
| get_renderedconfigdocs_version("buffer") | ||
| self.assertEqual(response.response['status'], '200') | ||
| except exceptions.NotFound: |
There was a problem hiding this comment.
As per Shipyard API documentation, for some of the API call, there might "404 NotFound" exception be raised, therefore try/exception block has been used. As of now, there are only test cases related to GET API call, once POST API call test cases will be added, setup and teardown will definitely be introduced.
| response = self.shipyard_document_staging_client.\ | ||
| get_renderedconfigdocs_version("successful_site_action") | ||
| self.assertEqual(response.response['status'], '200') | ||
| except exceptions.NotFound: |
There was a problem hiding this comment.
same comments as above
There was a problem hiding this comment.
As per Shipyard API documentation, for some of the API call, there might "404 NotFound" exception be raised, therefore try/exception block has been used. As of now, there are only test cases related to GET API call, once POST API call test cases will be added, setup and teardown will definitely be introduced.
rickbartra91
left a comment
rickbartra91
left a comment
There was a problem hiding this comment.
Please provide more detailed commit message. Which APIs are being tested? What is the meaning of F30?
I would like to see your team more involved in the code reviews. Until I see this happen, I will not continue to review as I don't have the capacity to do so. We need more participation in review. As mentioned, I shouldn't be the only gate keeper to getting code merged. Please make sure your team has proper access to review and leave comments.
Please also understand that in the future these tests need to be built out more and do assertions on the response body and not just checking response codes.
I also do not believe the NotFound exceptions should be silenced.
|
Please update tests to do assertions beyond asserting the HTTP response code. |
|
I am not seeing the updates you made. Please squash your commits into a single commit as right now it is 4 different commits. Perhaps after squashing, I will be able to see the changes you made without having to make a new PR? Also, I still do not see code review from your team.... This needs to change. |
2 participants
Shipyard GET API Tests - F30