Nir Soffer has posted comments on this change. Change subject: sdm: Use guarded.context in copy_data ......................................................................
Patch Set 13: (4 comments) https://gerrit.ovirt.org/#/c/61693/13/tests/storage_sdm_copy_data_test.py File tests/storage_sdm_copy_data_test.py: Line 136 Line 137 Line 138 Line 139 Line 140 We need to write these tests now. Line 116: Line 117: with checked_locks() as checker: Line 118: job.run() Line 119: wait_for_job(job) Line 120: checker.validate(self.get_lock_list(src_vol, dst_vol)) I don't think that anyone can understand from this code what is the expected behavior of the code (e.g. what locks were taken and in which order). I think we should use a much simpler approach - we can mock guarded.context, so we test only that we passed the correct locks. We already tested that guarded.context does the right thing with locks. this will make the test much simpler. This assumes that the code is using guarded.context - theoretically someone can write perfect code without it, but the test will fail, but practically we want all code to use guarded.context for locking, so this is not a real issue. I would test this in another test that does not do any data copying (mock qemu.convert?), and test only the locks passed to the context while job was running. Line 121: Line 122: self.assertEqual(jobs.STATUS.DONE, job.status) Line 123: self.assertEqual(100.0, job.progress) Line 124: self.assertNotIn('error', job.info()) https://gerrit.ovirt.org/#/c/61693/13/vdsm/storage/sdm/api/copy_data.py File vdsm/storage/sdm/api/copy_data.py: Line 95 Line 96 Line 97 Line 98 Line 99 We must implement this todo to finish this milestone, you can add it in the next patch if you like. Line 96: self._vol = None Line 97: Line 98: # @property Line 99: # def host_id(self): Line 100: # return self._host_id Please remove if not needed. Line 101: Line 102: @property Line 103: def locks(self): Line 104: img_ns = sd.getNamespace(sc.IMAGE_NAMESPACE, self.sd_id) -- To view, visit https://gerrit.ovirt.org/61693 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5c2a9527f5e3787069e0847e1bf775a60321b306 Gerrit-PatchSet: 13 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Liron Aravot <lara...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/vdsm-patches@lists.fedorahosted.org
