Nir Soffer has posted comments on this change. Change subject: qemuimg: Use --output json for 'check' and 'info' ......................................................................
Patch Set 3: (4 comments) https://gerrit.ovirt.org/#/c/59649/3/lib/vdsm/qemuimg.py File lib/vdsm/qemuimg.py: Line 59: return "ecode=%s, stdout=%s, stderr=%s, message=%s" % ( Line 60: self.ecode, self.stdout, self.stderr, self.message) Line 61: Line 62: Line 63: def parse_qemuimg_json(output): This looks like a private helper, lets rename it and move it to the bottom. Line 64: obj = json.loads(output) Line 65: if not isinstance(obj, dict): Line 66: raise ValueError("not a JSON object") Line 67: return obj https://gerrit.ovirt.org/#/c/59649/3/tests/qemuimgTests.py File tests/qemuimgTests.py: Line 38: return 0, json.dumps(data), [] Line 39: Line 40: Line 41: @expandPermutations Line 42: class GeneralTests(TestCaseBase): I would not add tests for this code, this is a private helper without any interesting logic. Better test this using one of the module public functions that use json format. We should create tests that change only when the behaviour changes, not when we change the implementation. Line 43: Line 44: def test_parse_qemuimg_json(self): Line 45: output = '{"a": 1}' Line 46: self.assertEqual({"a": 1}, qemuimg.parse_qemuimg_json(output)) Line 65: @expandPermutations Line 66: class InfoTests(TestCaseBase): Line 67: CLUSTER_SIZE = 65536 Line 68: Line 69: def _data(self): _data is little to generic, how about _fake_output? Line 70: return { Line 71: "virtual-size": 1048576, Line 72: "filename": "leaf.img", Line 73: "cluster-size": self.CLUSTER_SIZE, Line 361: Line 362: Line 363: class CheckTests(TestCaseBase): Line 364: Line 365: def _data(self): _data is little to generic, how about _fake_output? Line 366: return { Line 367: "image-end-offset": 262144, Line 368: "total-clusters": 16, Line 369: "check-errors": 0, -- To view, visit https://gerrit.ovirt.org/59649 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5db4fab6434246e7f1789ae8e438a0024b862f85 Gerrit-PatchSet: 3 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: 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
