Nir Soffer has posted comments on this change.

Change subject: storagetests: add test for mount.isMounted
......................................................................


Patch Set 2: -Code-Review

(5 comments)

Found more stuff to improve, see the comments.

https://gerrit.ovirt.org/#/c/55180/2//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2016-03-30 14:59:29 +0300
Line 6: 
Line 7: storagetests: add test for mount.isMounted
Line 8: 
Line 9: Given a spec, this test verifies that the expected fs_file is found in
Given a local path
Line 10: the /proc/mounts monkey patched temporary file by calling
Line 11: mount.isMounted.
Line 12: 
Line 13: Change-Id: I5f2c1c9f9fa03a079e8d94bd91f8e7b4fd43c0ee


Line 7: storagetests: add test for mount.isMounted
Line 8: 
Line 9: Given a spec, this test verifies that the expected fs_file is found in
Line 10: the /proc/mounts monkey patched temporary file by calling
Line 11: mount.isMounted.
This is not very clear. This patch add the missing tests ensuring that 
mount.isMounted works. The fact that we have to fake /proc/mounts is not 
interesting in the commit message.
Line 12: 
Line 13: Change-Id: I5f2c1c9f9fa03a079e8d94bd91f8e7b4fd43c0ee
Line 14: Related-To: https://bugzilla.redhat.com/1305529


https://gerrit.ovirt.org/#/c/55180/2/tests/mountTests.py
File tests/mountTests.py:

Line 289: #     <do something with /proc/mounts or /etc/mtab>
Line 290: #
Line 291: @contextmanager
Line 292: def fake_mounts(mounts_list):
Line 293:     data = "".join(line + "\n" for line in mounts_list)
mount_list -> mount_lines

Using type in the name is not a good idea, this is not a list of some objects 
but list of lines.

Use bytes instead of strings - tempoaryPath is expecting now bytes objects:
https://gerrit.ovirt.org/55489

Should be:

    data = b"".join(line + b"\n" for line in mount_lines)
Line 294:     with temporaryPath(data=data) as fake_mounts:
Line 295:         with monkeypatch.MonkeyPatchScope([
Line 296:             (mount, '_PROC_MOUNTS_PATH', fake_mounts),
Line 297:             (mount, '_ETC_MTAB_PATH', fake_mounts),


Line 301: 
Line 302: class TestRemoteSdIsMounted(TestCaseBase):
Line 303: 
Line 304:     def test_is_mounted(self):
Line 305:         with fake_mounts(["server:/path "
Use b"...", in Python 3 you cannot write a string to a file without specifying 
the encoding.
Line 306:                           "/rhev/data-center/mnt/server:_path "
Line 307:                           "nfs4 defaults 0 0"]):
Line 308:             self.assertTrue(mount.isMounted(
Line 309:                             "/rhev/data-center/mnt/server:_path"))


Line 305:         with fake_mounts(["server:/path "
Line 306:                           "/rhev/data-center/mnt/server:_path "
Line 307:                           "nfs4 defaults 0 0"]):
Line 308:             self.assertTrue(mount.isMounted(
Line 309:                             "/rhev/data-center/mnt/server:_path"))
Same, use b"..."
Line 310: 
Line 311:     def test_is_not_mounted(self):
Line 312:         with fake_mounts(["server:/path "
Line 313:                           "/rhev/data-center/mnt/server:_path "


-- 
To view, visit https://gerrit.ovirt.org/55180
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I5f2c1c9f9fa03a079e8d94bd91f8e7b4fd43c0ee
Gerrit-PatchSet: 2
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <[email protected]>
Gerrit-Reviewer: Allon Mureinik <[email protected]>
Gerrit-Reviewer: Freddy Rolland <[email protected]>
Gerrit-Reviewer: Idan Shaby <[email protected]>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <[email protected]>
Gerrit-Reviewer: Tal Nisan <[email protected]>
Gerrit-Reviewer: gerrit-hooks <[email protected]>
Gerrit-HasComments: Yes
_______________________________________________
vdsm-patches mailing list
[email protected]
https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches

Reply via email to