Change in vdsm[master]: Refactor v2v jobs for reusability
alitke at redhat.com
alitke at redhat.com
Mon Aug 24 20:52:10 UTC 2015
Adam Litke has posted comments on this change.
Change subject: Refactor v2v jobs for reusability
......................................................................
Patch Set 5:
(8 comments)
https://gerrit.ovirt.org/#/c/44857/5/tests/jobsTests.py
File tests/jobsTests.py:
Line 28: class TestingJob(jobs.Job):
Line 29: _JOB_TYPE = 'testing'
Line 30:
Line 31: def __init__(self, job_id):
Line 32: jobs.Job.__init__(self, job_id)
> To make it easier to test, I would generate the job_id here, so tests do
Done
Line 33: self._progress = 0
Line 34: self._aborted = False
Line 35:
Line 36: @property
Line 38: return self._progress
Line 39:
Line 40: def _abort(self):
Line 41: self._aborted = True
Line 42:
> Lets add some job subclasses to make it easier to test filtering:
Done
Line 43:
Line 44: @expandPermutations
Line 45: class JobsTests(VdsmTestCase):
Line 46: TIMEOUT = 1
Line 46: TIMEOUT = 1
Line 47:
Line 48: def setUp(self):
Line 49: jobs._clear()
Line 50: self.job_id = str(uuid.uuid4())
> Should be in TestingJob, this work only for single job, which is not correc
Done
Line 51:
Line 52: def test_job_initial_state(self):
Line 53: job = TestingJob(self.job_id)
Line 54: self.assertEqual(jobs.STATUS.RUNNING, job.status)
Line 79: job = TestingJob(job_id)
Line 80: job._progress = values['progress']
Line 81: job._description = values['description']
Line 82: job._status = values['status']
Line 83: jobs.add(job)
> I don't think we need this helper.
Done
Line 84:
Line 85: def test_get_jobs_info(self):
Line 86: job_info = {'dfc3f588-d122-4443-a0c2-51fe969be8f5':
Line 87: {'status': jobs.STATUS.RUNNING,
Line 92: {'status': jobs.STATUS.DONE,
Line 93: 'description': 'another job',
Line 94: 'progress': 100,
Line 95: 'job_type': 'testing'}}
Line 96: self._add_jobs(job_info)
> We don't check all these values, so we can do instead (assuming that we mod
Done
Line 97: self.assertEqual(job_info, jobs.get_jobs_info())
Line 98:
Line 99: def test_get_jobs_info_empty(self):
Line 100: self.assertEqual({}, jobs.get_jobs_info())
Line 109: {'status': jobs.STATUS.DONE,
Line 110: 'description': 'another job',
Line 111: 'progress': 100,
Line 112: 'job_type': 'bar'}}
Line 113: self._add_jobs(job_info)
> This could be simplified like the test above:
Done
Line 114: self.assertNotIn('9f40a5d9-301e-4244-9a73-4d49d827d0b9',
Line 115: jobs.get_jobs_info(type_filter='foo'))
Line 116:
Line 117: def test_abort_job(self):
https://gerrit.ovirt.org/#/c/44857/5/vdsm/jobs.py
File vdsm/jobs.py:
Line 142: return response.error(e.name)
Line 143: return response.success()
Line 144:
Line 145:
Line 146: def get_jobs_info(type_filter=None):
> Since all other names are short and nice, I think we get shorten it to info
Done
Line 147: with _lock:
Line 148: jobs = _jobs.values()
Line 149: return {job.id: job.info() for job in jobs
Line 150: if not type_filter or job.job_type == type_filter}
https://gerrit.ovirt.org/#/c/44857/5/vdsm/v2v.py
File vdsm/v2v.py:
Line 107:
Line 108: def get_jobs_status():
Line 109: all_jobs = jobs.get_jobs_info()
Line 110: return {job_id: status for (job_id, status) in all_jobs.items()
Line 111: if status['job_type'] == _V2V_JOB_TYPE}
> Why not use your new shiny filtering in jobs.get_jobs_info() ?
Done
Line 112:
Line 113:
Line 114: def get_external_vms(uri, username, password):
Line 115: if not supported():
--
To view, visit https://gerrit.ovirt.org/44857
To unsubscribe, visit https://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ida6b1c460c5030c820c540e836e423d4632410df
Gerrit-PatchSet: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Arik Hadas <ahadas at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Shahar Havivi <shavivi at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list