Hi Clemens. First of all, thank you for digging into the code and working to fix the issue you're seeing.

I am missing context here how the logic of the test code works;

Note that it is normal (in fact almost always required) to change test code with any change to the production side. The purpose of unit tests (you can tell this is a unit test from '/unit/' in the file path) is to exercise a small chunk ("unit" :) of code and make sure branching and method calls are all as expected. You've changed some logic, so the test is (rightly) failing, and you'll need to change what it's expecting accordingly. These tests are making use of mock [0] to hide the guts of some of the methods being called by the unit in question, just to make sure that those methods are being invoked the correct number of times, with the correct arguments. In this case...

Related to my changes, tox gets me an error:

nova.tests.unit.virt.libvirt.test_driver.LibvirtDriverTestCase.test_finish_migration_power_on

Captured traceback:

    Traceback (most recent call last):
      File "nova/tests/unit/virt/libvirt/test_driver.py", line 18707, in
test_finish_migration_power_on
        self._test_finish_migration()
      File
"/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/mock/mock.py",
line 1305, in patched
        return func(*args, **keywargs)
      File "nova/tests/unit/virt/libvirt/test_driver.py", line 18662, in
_test_finish_migration
        mock_raw_to_qcow2.assert_has_calls(convert_calls, any_order=True)
      File
"/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/mock/mock.py",
line 983, in assert_has_calls
        ), cause)
      File
"/opt/stack/nova/.tox/py27/local/lib/python2.7/site-packages/six.py",
line 737, in raise_from
        raise value
    AssertionError:
(call('/tmp/tmpC60sPk/tmpOHikWL/8ea1de33-64d7-4d1d-af02-88e6f7ec91c1/disk.swap'),)
not all found in call list

The stack trace is pointing you at [1] for all three failures. I can see from your code change that you're skipping the qcow conversion for disk.swap here [2]. So when I remove 'disk.swap' from the list on L19438, the three tests pass.

That will get you green in zuul, but I should warn you that one of the first things reviewers will notice is that you *only* had to change tests for the piece of your change at [2]. That means there's missing/incomplete test coverage for the other code paths you've touched, and you'll have to add some (or justify why it's unnecessary).

Are there any docs or helps alongside testing in Nova where to learn what to do?

I'm not sure if this is targeted at the right level for you, but here's the nova contributor guide [3]. If you're more of an interactive learner, feel free to jump on IRC in #openstack-nova and I'll be happy to walk you through some basics.

Perhaps also someone could give me a hint whether there are some conceptually misleading ideas behind my fix proposal which need to be driven into another direction …

Yup, that's what code review is for. Nova has a very high "open changes" to "reviewer bandwidth" ratio, so it's unfortunately pretty normal for changes to go unreviewed while they're still failing zuul testing. Getting those fixed up, and bringing attention to the issue here on the mailing list and/or IRC, should all get your change some better attention.

Thanks again for diving in.

efried

[0] https://docs.python.org/3/library/unittest.mock.html [1] https://opendev.org/openstack/nova/src/branch/master/nova/tests/unit/virt/li... [2] https://review.opendev.org/#/c/618621/4/nova/virt/libvirt/driver.py@8410 [3] https://docs.openstack.org/nova/latest/contributor/index.html