On 31.08.19 13:41, Vladimir Sementsov-Ogievskiy wrote:
> 09.08.2019 19:14, Max Reitz wrote:
>> This patch adds some tests on how commit copes with filter nodes.
>>
>> Signed-off-by: Max Reitz <mreitz@redhat.com>
>> ---
>>   tests/qemu-iotests/040     | 177 +++++++++++++++++++++++++++++++++++++
>>   tests/qemu-iotests/040.out |   4 +-
>>   2 files changed, 179 insertions(+), 2 deletions(-)
>>
>> diff --git a/tests/qemu-iotests/040 b/tests/qemu-iotests/040
>> index 6db9abf8e6..a0a0db8889 100755
>> --- a/tests/qemu-iotests/040
>> +++ b/tests/qemu-iotests/040

[...]

>> +    def tearDown(self):
>> +        self.vm.shutdown(has_quit=self.has_quit)
>> +
>> +        for index in range(len(self.pattern_files)):
> 
> you may use enumerate for such cases:
> for ind, file in enumerate(self.pattern_files):
>     ...

Ah, nice.

>> +            result = qemu_io('-f', iotests.imgfmt,
>> +                             '-c', 'read -P %i %iM 1M' % (index + 1, index),
>> +                             self.pattern_files[index])
>> +            self.assertFalse('Pattern verification failed' in result)
> 
> A bit better would be to keep this loop in a function and do "writes" through it too,
> to make it more obvious that they are the same.. But I'm OK with it as is.

Hm, yes.  I’ll have a look.

>> +
>> +        os.remove(self.img3)
>> +        os.remove(self.img2)
>> +        os.remove(self.img1)
>> +        os.remove(self.img0)
>> +
>> +    # Filters make for funny filenames, so we cannot just use
>> +    # self.imgX to get them
>> +    def get_filename(self, node):
>> +        return self.vm.node_info(node)['image']['filename']
>> +
> 
> maybe:
> def assertHasNode(self, node_name):
>    self.assertIsNotNone(self.vm.node_info(node_name))
> 
> and similar for assertNoNode...

Hm, I don’t know.  It fits on one line either way.

>> +    def test_filterless_commit(self):
>> +        self.assert_no_active_block_jobs()
> 
> why not just to include this call into setUp() ? Or even, just drop it?
> We create and start new vm in setUp, it don't have any block jobs for sure.

Other tests do it the same way, e.g. 030, 040, and 041.

[...]

>> +        self.assertIsNone(self.vm.node_info('top-filter'))
>> +        self.assertIsNone(self.vm.node_info('cow-3'))
>> +        self.assertIsNotNone(self.vm.node_info('cow-2'))
> 
> It would be good to assert here the cow-2 became drv0 child. However, otherwise
> it should be automatically dropped, so it's not necessary.

Yep, like cow-3.  I’ll look into it anyway.

>> +
>> +        # 3 has been comitted into 2
>> +        self.pattern_files[3] = self.img2
>> +
>> +    def test_filtered_active_commit_without_filter(self):
>> +        self.assert_no_active_block_jobs()
>> +        result = self.vm.qmp('block-commit',
>> +                             job_id='commit',
>> +                             device='top-filter',
>> +                             top_node='cow-3',
>> +                             base_node='cow-2')
>> +        self.assert_qmp(result, 'return', {})
> 
> can we check that really "active" commit is started, i.e. mirror block job?

We do:

>> +        self.complete_and_wait(drive='commit')

wait_ready is True by default, so this will first wait for a READY
event.  That only happens for active commit.

Max