Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add unit tests for Coriolis api #272

Merged
merged 1 commit into from
Nov 16, 2023

Conversation

Cristi1324
Copy link
Contributor

@Cristi1324 Cristi1324 commented Nov 1, 2023

This PR adds unit tests for Coriolis/api/middleware/auth.py

Copy link
Contributor

@Dany9966 Dany9966 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some more changes required. For example, you did not check the result at all. You'll need to add a setUp method to set up the object you're testing, and also make use of testutils.get_wrapped_function to only test the underlying function, without calling webob.dec.wsgify decorator.

req_mock = mock.Mock()
headers_mock = mock.Mock()
environ_mock = {}
remote_addr_mock = mock.Mock()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to mock every attribute of req_mock. The code you're testing will set these for you. Only keep req_mock, and directly set req_mock.headers = { key: value}.


req_mock.headers = headers_mock
req_mock.environ = environ_mock
req_mock.remote_addr = remote_addr_mock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove these 3 lines

class CoriolisKeystoneContextTestCase(test_base.CoriolisBaseTestCase):
"""Test suite for the Coriolis api middleware auth."""

@mock.patch.object(jsonutils, "loads")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't really need to mock this. This only turns a JSON-formatted string into a dir. Instead of mocking its return value, you could do the following:

expected_service_catalog = {"catalog1": ["service1", "service2"]}
req_mock.headers = {
    # ...
    'X_SERVICE_CATALOG': str(expected_service_catalog), # this will turn your dir into a string
    # ...
}
# ...
# ...
mock_request_context.assert_called_once_with(
    # ...
    service_catalog=expected_service_catalog, # checks that the string from headers is the original dir
    # ...
)

'X_SERVICE_CATALOG': 'mock_catalog',
}

mock_roles = [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, we don't call the actual tested code, we just assume the result. In this example, you passed '1,2,3', expected_roles should be ['1', '2', '3']', and that's what you should pass as roles, instead of mock_roles`

r.strip() for r in req_mock.headers.get('X_ROLE', '').split(',')
]

result = CoriolisKeystoneContext(wsgi.Middleware)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can __call__ by result = CoriolisKeystoneContext(wsgi.Middleware)(req_mock)

@Cristi1324 Cristi1324 force-pushed the api-unit-tests branch 3 times, most recently from 6bbed9a to 3bbec5f Compare November 2, 2023 17:08
Copy link
Contributor

@Dany9966 Dany9966 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review pass done. Requesting changs. Please also add test cases for _get_user method.

elif 'X_TENANT' in req.headers:
# This is for legacy compatibility
return req.headers['X_TENANT']
return ''
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of returning an empty string, please raise an exception if no tenant is passed, as it should be impossible to instantiate a context without a tenant:

else:
    raise webob.exc.HTTPBadRequest(
        explanation=_("No 'X_TENANT_ID' or 'X_TENANT' passed."))

result = self.auth(req_mock)

self.assertEqual(
CoriolisKeystoneContext(wsgi.Middleware).application,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be self.auth.application

mock_request_context
):
req_mock = mock.Mock()
mock__get_project_id.return_value = 'mock_tenant'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to assign a value to these mocks. Instead, please pass mock__get_project_id.return_value and mock_get_user.return_value to mock_request_context.assert_called_once_with() call.

result
)

mock_get_user.assert_called_once()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace this assert with assert_called_once_with(req_mock)

)

mock_get_user.assert_called_once()
mock__get_project_id.assert_called_once()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please replace this assert with assert_called_once_with(req_mock)

coriolis/tests/api/middleware/test_auth.py Show resolved Hide resolved
def test_get_project_id_tenant_id(self):
req_mock = mock.Mock()

expected_result = 'mock_tennant'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Please replace 'mock_tennant' with 'mock_tenant'

def test_get_project_id_tenant(self):
req_mock = mock.Mock()

expected_result = 'mock_tennant'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: Please replace 'mock_tennant' with 'mock_tenant'

coriolis/tests/api/middleware/test_auth.py Show resolved Hide resolved
@Cristi1324 Cristi1324 force-pushed the api-unit-tests branch 8 times, most recently from 83bbbd0 to 5068fba Compare November 8, 2023 15:53
Copy link
Contributor

@Dany9966 Dany9966 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly LGTM

from coriolis.tests import test_base


class CoriolisTestException(Exception):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this, it doesn't seem like it's used anywhere

def __call__(self, req):
user = self._get_user(req)

if isinstance(user, webob.exc.HTTPUnauthorized):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove this also, it makes no sense to return an exception if we raise it in _get_user

coriolis/tests/api/middleware/test_auth.py Show resolved Hide resolved
@Cristi1324 Cristi1324 force-pushed the api-unit-tests branch 2 times, most recently from c14b79a to 04f5d08 Compare November 10, 2023 13:17
from oslo_middleware import request_id
import webob

from coriolis.api.middleware.auth import CoriolisKeystoneContext
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NIT: as per the agreed upon guidelines, we only import modules as a whole (and not module methods or classes). Do not import objects, only modules (*) (source: https://docs.openstack.org/hacking/latest/user/hacking.html#imports)
Therefore, please import from coriolis.api.middleware import auth, and change all CoriolisKeystoneContext occurances to auth.CoriolisKeystoneContext.

Copy link
Contributor

@Dany9966 Dany9966 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Dany9966 Dany9966 merged commit b9ce41e into cloudbase:master Nov 16, 2023
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants