Open Stack

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/libvirt/test_driver.py#L19437-L19439
[2] https://review.opendev.org/#/c/618621/4/nova/virt/libvirt/driver.py@8410
[3] https://docs.openstack.org/nova/latest/contributor/index.html