[Kimchi-devel] [PATCH v2] RollbackContext: Make it complied with Python Standard
Shu Ming
shuming at linux.vnet.ibm.com
Tue Jan 21 01:01:18 UTC 2014
Reviewed-by: Shu Ming <shuming at linux.vnet.ibm.com>
2014/1/16 13:27, Zhou Zheng Sheng :
> According to Python documentation, context managers should not re-raise
> the exception passed to __exit__ method. If __exit__() does not return
> True, Python re-raises this exception automatically. Context managers
> should only raise exception from __exit__() itself. This means we should
> only re-raise exception caught from the undo functions.
>
> Another problem of the current implementation is that it suppresses str
> exceptions. This is because of bug in old Python version. When an
> exception is raised from within a "with" statement, exc_value would be a
> raw str or int rather than Exception instance, so when we re-raise the
> stored exc_value, it triggers another exception saying "exceptions must
> be old-style classes or derived from BaseException, not str".
> This patch fix this problem by raising the stored exception value along
> with its store exception type.
>
> This patch also adds unit test for RollbackContext.
>
> http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__
>
> v1 -> v2
> Add tests/test_rollbackcontext.py to tests/Makefile.am.
> Add tests/test_rollbackcontext.py to Makefile.am PEP8 list.
>
> Signed-off-by: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
> ---
> Makefile.am | 1 +
> src/kimchi/rollbackcontext.py | 26 +++++------
> tests/Makefile.am | 1 +
> tests/test_rollbackcontext.py | 102 ++++++++++++++++++++++++++++++++++++++++++
> 4 files changed, 117 insertions(+), 13 deletions(-)
> create mode 100644 tests/test_rollbackcontext.py
>
> diff --git a/Makefile.am b/Makefile.am
> index 04ad696..b09a545 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -60,6 +60,7 @@ PEP8_WHITELIST = \
> tests/test_model.py \
> tests/test_plugin.py \
> tests/test_rest.py \
> + tests/test_rollbackcontext.py \
> tests/test_storagepool.py \
> tests/utils.py \
> $(NULL)
> diff --git a/src/kimchi/rollbackcontext.py b/src/kimchi/rollbackcontext.py
> index 2afd114..d493af2 100644
> --- a/src/kimchi/rollbackcontext.py
> +++ b/src/kimchi/rollbackcontext.py
> @@ -6,6 +6,7 @@
> # Authors:
> # Adam Litke <agl at linux.vnet.ibm.com>
> # ShaoHe Feng <shaohef at linux.vnet.ibm.com>
> +# Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
> #
> # This library is free software; you can redistribute it and/or
> # modify it under the terms of the GNU Lesser General Public
> @@ -45,24 +46,23 @@ class RollbackContext(object):
> return self
>
> def __exit__(self, exc_type, exc_value, traceback):
> - firstException = exc_value
> -
> + """
> + According to Python official doc. This function should only re-raise
> + the exception from undo functions. Python automatically re-raises the
> + original exception when this function does not return True.
> + http://docs.python.org/2/library/stdtypes.html#contextmanager.__exit__
> + """
> + undoExcInfo = None
> for undo, args, kwargs in self._finally:
> try:
> undo(*args, **kwargs)
> - except Exception as e:
> + except Exception:
> # keep the earliest exception info
> - if not firstException:
> - firstException = e
> - # keep the original traceback info
> - traceback = sys.exc_info()[2]
> + if undoExcInfo is None:
> + undoExcInfo = sys.exc_info()
>
> - # re-raise the earliest exception
> - if firstException is not None:
> - if type(firstException) is str:
> - sys.stderr.write(firstException)
> - else:
> - raise firstException, None, traceback
> + if exc_type is None and undoExcInfo is not None:
> + raise undoExcInfo[0], undoExcInfo[1], undoExcInfo[2]
>
> def defer(self, func, *args, **kwargs):
> self._finally.append((func, args, kwargs))
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 55d3ab4..59e344d 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -31,6 +31,7 @@ EXTRA_DIST = \
> test_osinfo.py \
> test_plugin.py \
> test_rest.py \
> + test_rollbackcontext.py \
> test_server.py \
> test_storagepool.py \
> test_vmtemplate.py \
> diff --git a/tests/test_rollbackcontext.py b/tests/test_rollbackcontext.py
> new file mode 100644
> index 0000000..fea430d
> --- /dev/null
> +++ b/tests/test_rollbackcontext.py
> @@ -0,0 +1,102 @@
> +#
> +# Project Kimchi
> +#
> +# Copyright IBM, Corp. 2014
> +#
> +# Authors:
> +# Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
> +#
> +# This library is free software; you can redistribute it and/or
> +# modify it under the terms of the GNU Lesser General Public
> +# License as published by the Free Software Foundation; either
> +# version 2.1 of the License, or (at your option) any later version.
> +#
> +# This library is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> +# Lesser General Public License for more details.
> +#
> +# You should have received a copy of the GNU Lesser General Public
> +# License along with this library; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
> +
> +import unittest
> +
> +from kimchi.rollbackcontext import RollbackContext
> +
> +
> +class FirstError(Exception):
> + '''A hypothetical exception to be raise in the test firstly.'''
> + pass
> +
> +
> +class SecondError(Exception):
> + '''A hypothetical exception to be raise in the test secondly.'''
> + pass
> +
> +
> +class RollbackContextTests(unittest.TestCase):
> +
> + def setUp(self):
> + self._counter = 0
> +
> + def _inc_counter(self):
> + self._counter += 1
> +
> + def _raise(self, exception=FirstError):
> + raise exception()
> +
> + def test_rollback(self):
> + with RollbackContext() as rollback:
> + rollback.prependDefer(self._inc_counter)
> + rollback.prependDefer(self._inc_counter)
> + self.assertEquals(self._counter, 2)
> +
> + def test_raise(self):
> + try:
> + with RollbackContext() as rollback:
> + rollback.prependDefer(self._inc_counter)
> + rollback.prependDefer(self._inc_counter)
> + raise FirstError()
> + rollback.prependDefer(self._inc_counter)
> + except FirstError:
> + # All undo before the FirstError should be run
> + self.assertEquals(self._counter, 2)
> + else:
> + self.fail('Should have raised FirstError')
> +
> + def test_raise_undo(self):
> + try:
> + with RollbackContext() as rollback:
> + rollback.prependDefer(self._inc_counter)
> + rollback.prependDefer(self._raise)
> + rollback.prependDefer(self._inc_counter)
> + except FirstError:
> + # All undo should be run
> + self.assertEquals(self._counter, 2)
> + else:
> + self.fail('Should have raised FirstError')
> +
> + def test_raise_prefer_original(self):
> + try:
> + with RollbackContext() as rollback:
> + rollback.prependDefer(self._raise, SecondError)
> + raise FirstError()
> + except FirstError:
> + pass
> + except SecondError:
> + self.fail('Should have preferred FirstError to SecondError')
> + else:
> + self.fail('Should have raised FirstError')
> +
> + def test_raise_prefer_first_undo(self):
> + try:
> + with RollbackContext() as rollback:
> + rollback.prependDefer(self._raise, SecondError)
> + rollback.prependDefer(self._raise, FirstError)
> + except FirstError:
> + pass
> + except SecondError:
> + self.fail('Should have preferred FirstError to SecondError')
> + else:
> + self.fail('Should have raised FirstError')
More information about the Kimchi-devel
mailing list