Dan Kenigsberg has posted comments on this change.
Change subject: minimalistic (incomplete) test for mkimage.py with fail for non-64base
data
......................................................................
Patch Set 1: I would prefer that you didn't submit this
(4 inline comments)
Josef, thanks for your contribution!
Would you make sure that your test works? (on my machine, it does not)
....................................................
File tests/mkimageTests.py
Line 1: #author: Josef Pacula, josef.pacula(a)gmail.com, 2012
We would need the GPL boiler plate in order to accept code in Vdsm. sorry. lawyers. you
know.
Line 12: testFiles = {'fileA': 'content of the file A',
I suggest that you first try writing a test with a base64 encoding of the content, e.g.
'Y29udGVudCBvZiB0aGUgZmlsZSBB'. Your testFiles causes an error condition, which
should happen only in an advanced test.
Line 17: self.assertTrue(mount.isMounted())
did this ever worked? how? you never call mount() here. If you do mount, you should limit
this tests to root user only.
....................................................
File vdsm/mkimage.py
Line 94: isopath = _getFileName(vmId, files)
there's a nasty bug in this function, and you are not completely fixing it.
Since isopath and dirname are mentioned in the "finally" block of this function,
they must be assigned before the "try". For example, when your code is run as
non-root, I still get
UnboundLocalError: local variable 'dirname' referenced before assignment
in any case, please split this bug-fixing from unittest - it deserves a commit message of
its own.
--
To view, visit
http://gerrit.ovirt.org/6275
To unsubscribe, visit
http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: I1a7656359beb5d116ad1919e237fe084a1208d71
Gerrit-PatchSet: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Josef Pacula <josef.pacula(a)gmail.com>
Gerrit-Reviewer: Dan Kenigsberg <danken(a)redhat.com>