Change in vdsm[master]: tests: Add assertNotRaises assertion
nsoffer at redhat.com
nsoffer at redhat.com
Mon Jun 9 05:21:19 UTC 2014
Nir Soffer has posted comments on this change.
Change subject: tests: Add assertNotRaises assertion
......................................................................
Patch Set 1:
(9 comments)
Yoav, you should use your -1 button. The tests names are not clear, the class name is incorrect, and this code includes YAGNI violations - are you sure that this looks good to you?
http://gerrit.ovirt.org/#/c/28177/1/tests/testrunner.py
File tests/testrunner.py:
Line 180: return context
Line 181: with context:
Line 182: callableObj(*args, **kwargs)
Line 183:
Line 184: def assertNotRaises(self, excClass, callableObj=None, *args, **kwargs):
> My experience is that assertNotRaises usually means that the cody does not
I'll remove the class argument, but I don't think we should check BaseException.
Line 185: # This is required when exception raised during the call should be
Line 186: # considered as a test failure.
Line 187: context = not_raises(excClass, self)
Line 188: if callableObj is None:
Line 182: callableObj(*args, **kwargs)
Line 183:
Line 184: def assertNotRaises(self, excClass, callableObj=None, *args, **kwargs):
Line 185: # This is required when exception raised during the call should be
Line 186: # considered as a test failure.
> assertNotRaises is a good enough name - you can delete this comment. It too
Please suggest a better name (it takes more then 10 seconds).
Line 187: context = not_raises(excClass, self)
Line 188: if callableObj is None:
Line 189: return context
Line 190: with context:
Line 183:
Line 184: def assertNotRaises(self, excClass, callableObj=None, *args, **kwargs):
Line 185: # This is required when exception raised during the call should be
Line 186: # considered as a test failure.
Line 187: context = not_raises(excClass, self)
> why shorthand? rename excClass to exceptionClass or exceptionType
This is the same name used in assertRaises - consistency is important.
Line 188: if callableObj is None:
Line 189: return context
Line 190: with context:
Line 191: callableObj(*args, **kwargs)
Line 187: context = not_raises(excClass, self)
Line 188: if callableObj is None:
Line 189: return context
Line 190: with context:
Line 191: callableObj(*args, **kwargs)
> I know callableObj is an object. rename to "callable".
This is the same name used in assertRaises - consistency is important.
In another patch we can improves names in this module. This change should look like the original author wrote it.
Line 192:
Line 193: # FIXME: This is a forward port of the assertIn from python
Line 194: # 2.7, remove when no loger supporting earlier versions
Line 195: def assertIn(self, member, container, msg=None):
http://gerrit.ovirt.org/#/c/28177/1/tests/testrunnerTests.py
File tests/testrunnerTests.py:
Line 32: class OtherError(Exception):
Line 33: pass
Line 34:
Line 35:
Line 36: class TestRunnerTests(VdsmTestCase):
Will rename to AssertNotRaisesTests
Line 37:
Line 38: def test_context_failure(self):
Line 39: with self.assertRaises(self.failureException):
Line 40: with self.assertNotRaises(Error):
Line 34:
Line 35:
Line 36: class TestRunnerTests(VdsmTestCase):
Line 37:
Line 38: def test_context_failure(self):
> test_context_failure is a cryptic name.
test_assertNotRaises is a bad name, mixing underscore_andMixedCase - the worst we can do :-)
Line 39: with self.assertRaises(self.failureException):
Line 40: with self.assertNotRaises(Error):
Line 41: raise Error("generated error")
Line 42:
Line 41: raise Error("generated error")
Line 42:
Line 43: def test_context_pass(self):
Line 44: with self.assertNotRaises(Error):
Line 45: raise OtherError("generated error")
> as I said before, looks like YAGNI to me.
Will remove.
Line 46:
Line 47: def test_inline_failure(self):
Line 48: def func():
Line 49: raise Error("generated error")
Line 43: def test_context_pass(self):
Line 44: with self.assertNotRaises(Error):
Line 45: raise OtherError("generated error")
Line 46:
Line 47: def test_inline_failure(self):
> your mind was in the context of assertNotRaises, so you thought that test_i
I agree, the class name is wrong.
Line 48: def func():
Line 49: raise Error("generated error")
Line 50: with self.assertRaises(self.failureException):
Line 51: self.assertNotRaises(Error, func)
Line 54: def func():
Line 55: raise OtherError("generated error")
Line 56: self.assertNotRaises(Error, func)
Line 57:
Line 58: def test_subclass(self):
> again, cryptic name. test_assertNotRaises_FailsOnExceptionSubclass or somet
Will remove this test.
Line 59: with self.assertRaises(self.failureException):
Line 60: with self.assertNotRaises(Error):
--
To view, visit http://gerrit.ovirt.org/28177
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I33c96ca855753a75ffe2f82bdecab2e1612da08a
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Xavi Francisco <xfrancis at redhat.com>
Gerrit-Reviewer: Yoav Kleinberger <ykleinbe at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list