Re: [PATCH 1/2] iotests: add JobRunner class

[Max Reitz]

On 26.02.20 18:58, John Snow wrote:
> 
> 
> 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 <address@hidden>
>>> ---
>>>  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?

Yes.

> If so, I agree if only for purity -- the function looks and acts like a
> callback instead of a callback-that-calls-another-callback.

That was my thinking.

>>> +                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.

OK.  So you’re looking to basically get VM.qmp() to log automatically if
necessary?  (or maybe qmp_log() to not log unless necessary)

Max

signature.asc
Description: OpenPGP digital signature