Re: [PATCH 2/2] iotests: modify test 040 to use JobRunner

[John Snow]

On 2/26/20 6:31 AM, Max Reitz wrote:
> On 26.02.20 01:44, John Snow wrote:
>> Instead of having somewhat reproduced it for itself.
>>
>> Signed-off-by: John Snow <address@hidden>
>> ---
>>  tests/qemu-iotests/040 | 51 +++++++++++++++++++++---------------------
>>  1 file changed, 25 insertions(+), 26 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
>> index 90b59081ff..579dafc797 100755
>> --- a/tests/qemu-iotests/040
>> +++ b/tests/qemu-iotests/040
>> @@ -483,34 +483,33 @@ class TestErrorHandling(iotests.QMPTestCase):
>>                            file=('top-dbg' if top_debug else 'top-file'),
>>                            backing='mid-fmt')
>>  
>> +
>> +    class TestJobRunner(iotests.JobRunner):
>> +        expected_events = ('BLOCK_JOB_COMPLETED',
>> +                           'BLOCK_JOB_ERROR',
>> +                           'BLOCK_JOB_READY')
>> +
>> +        def __init__(self, *args, test, **kwargs):
>> +            super().__init__(*args, **kwargs)
>> +            self.log = []
>> +            self.test = test
>> +
>> +        def on_pause(self, event):
>> +            result = self._vm.qmp('block-job-resume', device=self._id)
>> +            self.test.assert_qmp(result, 'return', {})
>> +            super().on_pause(event)
> 
> Not that it functionally matters, but I suppose I’d call
> super().on_pause() before resuming (because the job isn’t exactly paused
> afterwards).
> 

Reasonable detail to consider.

It's also likely valid to just *omit* calling the base class pause event
when overriding behavior -- If we decide to send resume commands in the
future, we'll want to avoid sending conflicting/duplicate events.

In this case, the base event is just a NOP so it doesn't matter, but it
probably is good hygiene to avoid changing the state *and then* calling
the base class.

So I think this is a valid observation that should be worked into the
docstring for the JobRunner class on how best to make use of it.

>> +
>> +        def on_block_job_event(self, event):
>> +            if event['event'] not in self.expected_events:
>> +                self.test.fail("Unexpected event: %s" % event)
>> +            super().on_block_job_event(event)
>> +            self.log.append(iotests.filter_qmp_event(event))
> 
> Hasn’t the event been through filter_qmp_event() already?
> 

Oh, yeah. When I converted 040 here I just kind of shoehorned it onto
the new API in a somewhat mechanical fashion, but you're right.