Merge "Fix _validate_switch_role throwing incorrect error message"
diff --git a/patrole_tempest_plugin/rbac_utils.py b/patrole_tempest_plugin/rbac_utils.py
index 4bfa7fe..d952014 100644
--- a/patrole_tempest_plugin/rbac_utils.py
+++ b/patrole_tempest_plugin/rbac_utils.py
@@ -14,7 +14,6 @@
# under the License.
import sys
-import testtools
import time
from oslo_log import log as logging
@@ -137,10 +136,11 @@
self.switch_role_history.setdefault(key, None)
if self.switch_role_history[key] == toggle_rbac_role:
- # If the test was skipped, then this is a legitimate use case,
- # so do not throw an exception.
- exc_value = sys.exc_info()[1]
- if not isinstance(exc_value, testtools.TestCase.skipException):
+ # If an exception was thrown, like a skipException or otherwise,
+ # then this is a legitimate reason why `switch_role` was not
+ # called, so only raise an exception if no current exception is
+ # being handled.
+ if sys.exc_info()[0] is None:
self.switch_role_history[key] = False
error_message = '`toggle_rbac_role` must not be called with '\
'the same bool value twice. Make sure that you included '\
diff --git a/patrole_tempest_plugin/tests/unit/test_rbac_utils.py b/patrole_tempest_plugin/tests/unit/test_rbac_utils.py
index feccfe5..057ce20 100644
--- a/patrole_tempest_plugin/tests/unit/test_rbac_utils.py
+++ b/patrole_tempest_plugin/tests/unit/test_rbac_utils.py
@@ -195,23 +195,38 @@
autospec=True, return_value=False)
@mock.patch.object(rbac_utils, 'LOG', autospec=True)
@mock.patch.object(rbac_utils, 'sys', autospec=True)
- def test_rbac_utils_switch_roles_with_skip_exception(self, mock_sys,
- mock_log, _):
+ def test_rbac_utils_switch_roles_with_unhandled_exception(self, mock_sys,
+ mock_log, _):
+ """Test whether throwing an unhandled exception doesn't throw error.
+
+ If a skip exception, say, is thrown then this means that switch_role is
+ never called within the test function. But if an unhandled exception
+ or skip exception is thrown, then this should not result in an error
+ being raised.
+ """
self._mock_list_user_roles_on_project('member_id')
- mock_skip_exception = mock.Mock(spec=testtools.TestCase.skipException)
- mock_sys.exc_info.return_value = [None, mock_skip_exception]
+ # Skip exception is an example of a legitimate case where `switch_role`
+ # is thrown. AttributeError, on the other hand, is an example of an
+ # unexpected exception being thrown that should be allowed to bubble
+ # up, rather than being obfuscated by `switch_role` error being thrown
+ # instead.
+ unhandled_exceptions = [testtools.TestCase.skipException,
+ AttributeError]
- # Ordinarily switching to the same role would result in an error,
- # but because the skipException is thrown before the test finishes,
- # this is not treated as a failure.
- self.rbac_utils.switch_role(self.mock_test_obj, False)
- self.rbac_utils.switch_role(self.mock_test_obj, False)
- mock_log.error.assert_not_called()
+ for unhandled_exception in unhandled_exceptions:
+ mock_sys.exc_info.return_value = [unhandled_exception]
- self.rbac_utils.switch_role(self.mock_test_obj, True)
- self.rbac_utils.switch_role(self.mock_test_obj, True)
- mock_log.error.assert_not_called()
+ # Ordinarily switching to the same role would result in an error,
+ # but because the skipException is thrown before the test finishes,
+ # this is not treated as a failure.
+ self.rbac_utils.switch_role(self.mock_test_obj, False)
+ self.rbac_utils.switch_role(self.mock_test_obj, False)
+ mock_log.error.assert_not_called()
+
+ self.rbac_utils.switch_role(self.mock_test_obj, True)
+ self.rbac_utils.switch_role(self.mock_test_obj, True)
+ mock_log.error.assert_not_called()
@mock.patch.object(rbac_utils.rbac_utils, '_clear_user_roles',
autospec=True, return_value=False)