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