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