Change in vdsm[master]: Adding supervdsm unit tests

ybronhei at redhat.com ybronhei at redhat.com
Wed Oct 24 12:38:46 UTC 2012


Yaniv Bronhaim has posted comments on this change.

Change subject: Adding supervdsm unit tests
......................................................................


Patch Set 8: (2 inline comments)

....................................................
File vdsm/supervdsm.py
Line 90
Line 91
Line 92
Line 93
Line 94
I don't think that putting a comment here can help better to understand it.. 
It is just confusing. 
First, I add the comment in the patch description that explains how those overidding works. 

Second, you ask for comment that related to the tests inside the  implementation code. It sounds redundant, maybe I should put this comment inside the tests setUp function, but I still think that comment is not needed here.. You can easily understand what the comment that you want says by following the tests code.

And third, you ask for ## # These variables are overridden during testing. If you make changes to them # be careful to maintain the option of creating a test instance ##
What changes? like new tests?

Maybe we can phrase a better comment if we find it important..


....................................................
File vdsm/supervdsmServer.py
Line 376: 
Line 377:         log.debug("Started serving super vdsm object")
Line 378:         servThread.join()
Line 379:     except Exception as e:
Line 380:         log.error("Could not start Super Vdsm: %s", e)
What was it before?... I don't understand your comment.

I found it better not to print the stacktrace here because there is no stack here, its the main function. 

You can ask why I did this before giving it -1... and if you still think that this is wrong please explain why - 'undesirable exception printing method' is not an explanation.. why is it undesirable for you?
Line 381:         sys.exit(1)
Line 382:     finally:
Line 383:         try:
Line 384:             if address is not None and os.path.exists(address):


--
To view, visit http://gerrit.ovirt.org/8600
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I08880b12db8a6ca9cc57d74d2ef2a86980a5b097
Gerrit-PatchSet: 8
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Adam Litke <agl at us.ibm.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Saggi Mizrahi <smizrahi at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>


More information about the vdsm-patches mailing list