Fix _validate_switch_role throwing incorrect error message
Currently, if switch_role in rbac_utils is not called correctly
(or not at all), then a role switch error is thrown. It is only
not thrown if a skip exception is currently being handled--that is,
if the test is skipped, then the validation is ignored.
However, this logic is not entirely correct. For example, when
an unexpected error is thrown, like an AttributeError, then the
validation check fails, and so a role switch error is thrown -- when
instead the AttributeError should be thrown instead.
This patch changes the behavior of _validate_switch_role to only
throw a switch role error if no other exception is currently
being handled.
Change-Id: I8c3944e766210a31aa684e29c45e39470b738640
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)