Eduardo has uploaded a new change for review.
Change subject: getAllVolumes() unit test. ......................................................................
getAllVolumes() unit test.
Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Signed-off-by: Eduardo ewarszaw@redhat.com --- M tests/Makefile.am A tests/getAllVolumesTests.py A tests/glob_1c60971a-8647-44ac-ae33-6520887f8843.out A tests/lvs_3386c6f2-926f-42c4-839c-38287fac8998.out 4 files changed, 129 insertions(+), 1 deletion(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/92/4592/1 -- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(4 inline comments)
.................................................... File tests/getAllVolumesTests.py Line 27: sys.path.append('/usr/share/vdsm') no playing with sys.path inside modules, please.
unit tests should not assume that vdsm is installed.
Line 33: class TestBlockGetAllVolumes(TestCaseBase): I'd rather have a more testable code to begin with but that's fine too.
Line 70: # Danger Will Robinson! Danger! right! eval is evil. why not edit glob.out and read it line by line?
.................................................... File tests/Makefile.am Line 39: getAllVolumesTests.py make loves tabs
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Yeela Kaplan has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 1: I would prefer that you didn't submit this
(3 inline comments)
Please check comments
.................................................... File tests/glob_1c60971a-8647-44ac-ae33-6520887f8843.out Line 1: ['/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/5a481bbb-e259-427b-9e59-17a35166395c/b29b49db-a814-4e91-b990-05a6a8ba9033.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/7fccc8ef-596b-4632-bc8b-47d69576d02d/f0447a23-9849-43e9-ad09-1e604adba473.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/7fccc8ef-596b-4632-bc8b-47d69576d02d/90e29ef5-988b-4069-b53d-a8e0fd85b805.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/d3df6efc-b0af-448f-b5c9-31eddf3164bb/963e8158-1104-4317-9f38-7095b1f1ab7c.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/d3df6efc-b0af-448f-b5c9-31eddf3164bb/36c42d2b-f12e-4e43-a197-a3bd70f8ae38.meta', '/rhe! v/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/d3df6efc-b0af-448f-b5c9-31eddf3164bb/90e29ef5-988b-4069-b53d-a8e0fd85b805.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/b0e9e7af-bd04-47a7-b469-af2603d7d387/3a522f49-1c6a-492a-a1b8-842738dd325d.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/686f9732-1939-489c-8de8-7f38015f8bd1/99721285-5f8e-4646-a4e7-cd4bb8ea8b02.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/d3acfb07-4f99-402e-9498-d3cffacb9abc/4b240fc8-f80e-4a81-aa42-b31df2d47782.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/692fbedf-534d-42eb-b723-52940170adac/9b9186bf-7b7b-415c-a358-02dcf0d3de52.meta', '/rhev/data-center! /mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/692fbedf-534d-42eb-b723-52940170adac/90e29ef5-988b-4069-b53d-a8e0fd85b805.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/7ea080e1-d99b-4900-9399-e4919209fd47/90e29ef5-988b-4069-b53d-a8e0fd85b805.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/cd32daba-0d91-42da-91f7-7158a1e9db3e/abf17a9b-3e96-4b70-9d75-aa3cd7e639a5.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/94224eff-3c0a-455d-bd19-530b31be43d9/6ee03d5e-9950-42d2-870c-034c3a5393e6.meta', '/rhev/data-center/mnt/qanashead.qa.lab.tlv.redhat.com:_export_leonid/1c60971a-8647-44ac-ae33-6520887f8843/images/94224eff-3c0a-455d-bd19-530b31be43d9/90e29ef5-988b-4069-b53d-a8e0fd85b805.meta'] You should add this file's name to Makefile.am in the dist_noinst_DATA section
.................................................... File tests/lvs_3386c6f2-926f-42c4-839c-38287fac8998.out Line 32: A3ReZW-YMBD-69WV-diwc-H0WJ-ugtm-ikJ0dM|outbox|3386c6f2-926f-42c4-839c-38287fac8998|-wi-a---|134217728|0|/dev/mapper/1Dafna-rhevm11333889(22)| You should add this file's name to Makefile.am in the dist_noinst_DATA section
.................................................... File tests/Makefile.am Line 40: please insert before guestIFTests (alphabetical)
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 2: I would prefer that you didn't submit this
(4 inline comments)
I also don't like the extraneous files. Just put the strings in the python file.
.................................................... File tests/getAllVolumesTests.py Line 50: storage.blockSD.lvm.getLV = self.mGetLV You need to remember to revert the monkey patch or other tests might malefunction
Line 68: inp = open(os.path.join(sdHead, "glob_%s.out" % sdName), "r").read() Don't trust refcount to close your files
Line 69: # Danger Will Robinson! Danger! I cannot accept this course of action
Line 73: return True I don't think this passes pep8
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 2 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Saggi Mizrahi has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 3: I would prefer that you didn't submit this
Non of my comments have been addressed
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Ayal Baron has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 3: I would prefer that you didn't submit this
(1 inline comment)
.................................................... File tests/getAllVolumesTests.py Line 69: inp = open(os.path.join(sdHead, "glob_%s.out" % sdName), I agree that there is no reason to keep this info in a separate file. which would remove the need for open and eval here altogether.
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Ayal Baron has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 5: Looks good to me, but someone else must approve
even though I don't like the external files.
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Yeela Kaplan has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 5: Looks good to me, but someone else must approve
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 5: (1 inline comment)
/me tired.
.................................................... File tests/getAllVolumesTests.py Line 72: return eval(inp) I dislike eval, and do not understand why you refuse to use a simple, readable, file-per-line content for glob_%s.out.
you are truly stiff-necked ;-)
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 5: I would prefer that you didn't submit this
requires non-trivial rebase.
if you're in a good mood, please kill eval.
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 5 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has posted comments on this change.
Change subject: getAllVolumes() unit test. ......................................................................
Patch Set 6: Verified; Looks good to me, approved
rebasing myself.
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
Dan Kenigsberg has submitted this change and it was merged.
Change subject: getAllVolumes() unit test. ......................................................................
getAllVolumes() unit test.
Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Signed-off-by: Eduardo ewarszaw@redhat.com --- M tests/Makefile.am A tests/getAllVolumesTests.py A tests/glob_1c60971a-8647-44ac-ae33-6520887f8843.out A tests/lvs_3386c6f2-926f-42c4-839c-38287fac8998.out M vdsm/storage/fileSD.py 5 files changed, 137 insertions(+), 2 deletions(-)
Approvals: Dan Kenigsberg: Verified; Looks good to me, approved
-- To view, visit http://gerrit.ovirt.org/4592 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: merged Gerrit-Change-Id: Ic7cfb77f7675a58858507acf2b708317eb30a43d Gerrit-PatchSet: 6 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Ayal Baron abaron@redhat.com Gerrit-Reviewer: Dan Kenigsberg danken@redhat.com Gerrit-Reviewer: Eduardo ewarszaw@redhat.com Gerrit-Reviewer: Saggi Mizrahi smizrahi@redhat.com Gerrit-Reviewer: Yeela Kaplan ykaplan@redhat.com
vdsm-patches@lists.fedorahosted.org