Change in vdsm[master]: misc: Fix exception re-raising in RollbackContext
nsoffer at redhat.com
nsoffer at redhat.com
Tue Jan 7 17:32:10 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: misc: Fix exception re-raising in RollbackContext
......................................................................
Patch Set 8:
(6 comments)
Looks good, commit message and exceptions text needs some more love.
I love those tests.
....................................................
Commit Message
Line 18: "TypeError: exceptions must be old-style classes or derived from
Line 19: BaseException, not str"
Line 20:
Line 21: In this patch, the first exception is saved and re-raised with the
Line 22: i"raise expression, expression, expression" syntax with the original
Remove the "i" that sneaked in here.
Line 23: traceback.
Line 24: Tests Added:
Line 25: 1. Verify that the above scenario is handled correctly.
Line 26: 2. Verify that when an exception is raised both from the with statement
Line 19: BaseException, not str"
Line 20:
Line 21: In this patch, the first exception is saved and re-raised with the
Line 22: i"raise expression, expression, expression" syntax with the original
Line 23: traceback.
This explains the issue as we understood it before Zhou shows us the right way. We should explain here that the original code was incorrectly re-raising the original exception or the first undo exception, while it should only re-raise the first undo exception, as it does now.
And add empty line between paragraphs.
Line 24: Tests Added:
Line 25: 1. Verify that the above scenario is handled correctly.
Line 26: 2. Verify that when an exception is raised both from the with statement
Line 27: and from an undo statement, the one from the with statement is the
Line 28: one raised from __exit__().
Line 29:
Line 30: Relates-To:
Line 31: http://docs.python.org/2.6/reference/simple_stmts.html#the-raise-statement
Line 32: http://docs.python.org/2.6/library/stdtypes.html#contextmanager.__exit__
The second url needs another Relates-To: header, to be nice to automated tools parsing this.
The format seems to be:
Header-Words: text
And not:
Header-Words:
text
Line 33: Change-Id: I0052717e2307ad6fec2225b3ad5f438c5a60e1c6
....................................................
File tests/miscTests.py
Line 1091: class UndoException(Exception):
Line 1092: """A special exception for testing exceptions during undo functions"""
Line 1093:
Line 1094: class OriginalException(Exception):
Line 1095: """A special exception for testing exceptions in the with statement"""
Nice!
Using RuntimeError for validation is bad idea - the test may succeeds when such exception is raised by code you call to in the test, while these exceptions will never create false-positive results.
Line 1096:
Line 1097: def setUp(self):
Line 1098: self._called = 0
Line 1099:
Line 1176: except self.OriginalException:
Line 1177: return
Line 1178: except self.UndoException:
Line 1179: self.fail("Wrong exception was raised - from undo function. \
Line 1180: should have re-raised OriginalException")
Well it should not re-raise the exception - it is Python that raises it if we do not return True from __exit__.
Line 1181: except Exception:
Line 1182: self.fail("Wrong exception was raised")
Line 1183:
Line 1184: self.fail("Exception was not raised")
....................................................
File vdsm/storage/misc.py
Line 697:
Line 698: def __enter__(self):
Line 699: return self
Line 700:
Line 701: def __exit__(self, exc_type, exc_value, traceback):
It would be nice to have a comment here that explain why we do not re-raise the original exception, with a link to the docs.
Line 702: undoExcInfo = None
Line 703: for undo, args, kwargs in self._finally:
Line 704: try:
Line 705: undo(*args, **kwargs)
--
To view, visit http://gerrit.ovirt.org/22860
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I0052717e2307ad6fec2225b3ad5f438c5a60e1c6
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Ayal Baron <abaron at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Vered Volansky <vvolansk at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Zhou Zheng Sheng <zhshzhou at linux.vnet.ibm.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list