Change in vdsm[master]: storagetests: add test for mount.isMounted

ishaby at redhat.com ishaby at redhat.com
Mon Mar 28 07:55:27 UTC 2016


Idan Shaby has posted comments on this change.

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


Patch Set 1:

(2 comments)

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

Line 303: class TestRemoteSdIsMounted(TestCaseBase):
Line 304: 
Line 305:     @permutations([
Line 306:         # [<fs_spec>, <mount_line>]
Line 307:         ["server:/path",
> The spec is not relevant to mount.isMounted, it accept a local file created
Generally you are right - in a normal world this test should check mount.isMounted and the input should be the fs_file.
However, when checking if a file is mounted, the most relevant part of the scenario here is the conversion of fs_spec to fs_file, and it happens before the call to mount.isMounted.
The problem is that normalization of fs_spec is not performed before the conversion to fs_file and so there are cases that the file is not found in /proc/mounts because of it.

If I do what you suggest:
1. We will miss testing this part of the problem - the normalization.
2. The test will compare the given fs_file to the other given fs_file. So it can be reduced to strings comparison, what makes it stupid and useless.

It's true that mount.isMounted calls mount.getMountFromTarget, which calls normpath on the target (the fs_file), but it's redundant since normalizing the fs_file doesn't do anything.

Thus, I think that the input should remain of the form [<fs_spec>, <mount_line>], so that we can check the whole flow, including the normalization part of it.
Line 308:          "server:/path /rhev/data-center/mnt/server:_path nfs4 defaults 0 0"],
Line 309:     ])
Line 310:     def test_is_mounted(self, fs_spec, mount_line):
Line 311:         with fake_mounts([mount_line]):


Line 310:     def test_is_mounted(self, fs_spec, mount_line):
Line 311:         with fake_mounts([mount_line]):
Line 312:             self.assertTrue(mount.isMounted(self.generate_fs_file(fs_spec)))
Line 313: 
Line 314:     def generate_fs_file(self, fs_spec):
> Not needed, as the input to this test should be a fs_file as sent by the co
Please see the comment above.
Line 315:         mnt_path = fs_spec.replace('_', '__').replace('/', '_')


-- 
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: 1
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Allon Mureinik <amureini at redhat.com>
Gerrit-Reviewer: Freddy Rolland <frolland at redhat.com>
Gerrit-Reviewer: Idan Shaby <ishaby at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Tal Nisan <tnisan at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list