On 2/26/20 6:18 AM, Max Reitz wrote:
> On 26.02.20 01:44, John Snow wrote:
>> The idea is that instead of increasing the arguments to job_run all the
>> time, create a more general-purpose job runner that can be subclassed to
>> do interesting things with.
>>
>> Signed-off-by: John Snow <js...@redhat.com>
>> ---
>> tests/qemu-iotests/255 | 9 +-
>> tests/qemu-iotests/257 | 12 ++-
>> tests/qemu-iotests/287 | 19 +++-
>> tests/qemu-iotests/iotests.py | 176 ++++++++++++++++++++++++----------
>> 4 files changed, 158 insertions(+), 58 deletions(-)
>
> I like it!
>
High praise!
> [...]
>
>> diff --git a/tests/qemu-iotests/287 b/tests/qemu-iotests/287
>> index 0ab58dc011..f06e6ff084 100755
>> --- a/tests/qemu-iotests/287
>> +++ b/tests/qemu-iotests/287
>> @@ -165,13 +165,22 @@ def test_bitmap_populate(config):
>> if not config.disabled:
>> ebitmap.dirty_group(2)
>>
>> +
>> + class TestJobRunner(iotests.JobRunner):
>> + def on_pending(self, event):
>> + if config.mid_writes:
>> + perform_writes(drive0, 2)
>> + if not config.disabled:
>> + ebitmap.dirty_group(2)
>
> I actually prefer inlining the pre_finalize() functions (over calling
> the existing one), but then we can also remove the original function. :)
>
Not sure I understand you correctly. You're saying you prefer this
strategy where I inline the logic vs others where I call out to a function?
If so, I agree if only for purity -- the function looks and acts like a
callback instead of a callback-that-calls-another-callback.
>> + super().on_pending(event)
>> +
>> job = populate(drive0, 'target', 'bitpop0')
>> assert job['return'] == {'return': {}}
>> - vm.run_job(job['id'],
>> - auto_dismiss=job['auto-dismiss'],
>> - auto_finalize=job['auto-finalize'],
>> - pre_finalize=pre_finalize,
>> - cancel=config.cancel)
>> + job_runner = TestJobRunner(vm, job['id'],
>> + auto_dismiss=job['auto-dismiss'],
>> + auto_finalize=job['auto-finalize'],
>> + cancel=config.cancel)
>> + job_runner.run()
>> log('')
>>
>>
>> diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
>> index 3390fab021..37a8b4d649 100644
>> --- a/tests/qemu-iotests/iotests.py
>> +++ b/tests/qemu-iotests/iotests.py
>> @@ -460,6 +460,130 @@ def remote_filename(path):
>> else:
>> raise Exception("Protocol %s not supported" % (imgproto))
>>
>> +
>> +class JobRunner:
>
> [...]
>
>> + def on_ready(self, event):
>> + if self.logging:
>> + self._vm.qmp_log('job-complete', id=self._id)
>> + else:
>> + self._vm.qmp('job-complete', id=self._id)
>
> I suppose this is a bug fix. (The old version always called qmp_log.)
>
Technically yes. It was needed for 040.
> But what about adding a do_qmp method to JobRunner that does the
> “if self.logging { self._vm.qmp_log() } else { self._vm.qmp }” part so
> we don’t have to inline that everywhere?
>
> Max
>
I'll just clean up the logging series I had to do it at a more
fundamental level.
Just testing the temperature of the water.
--js
