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

[Max Reitz <mreitz@redhat.com>]

[-- Attachment #1.1: Type: text/plain, Size: 2900 bytes --]

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 <jsnow@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!

[...]

> 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. :)

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

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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]