Change in vdsm[master]: virt: add device setup and teardown

nsoffer at redhat.com nsoffer at redhat.com
Thu Apr 28 23:12:06 UTC 2016


Nir Soffer has posted comments on this change.

Change subject: virt: add device setup and teardown
......................................................................


Patch Set 12:

(1 comment)

https://gerrit.ovirt.org/#/c/55135/12/tests/vmfakelib.py
File tests/vmfakelib.py:

Line 419:     def __init__(self, device, fail_setup=None, fail_teardown=None):
Line 420:         self.fail_setup = fail_setup
Line 421:         self.fail_teardown = fail_teardown
Line 422:         self.device = device
Line 423:         self.is_setup = False
> I was thinking about it. For current tests, it's afaik not needed. Future w
True,  but since we don't use it now, I think it is better to remove it. The purpose of this class is to test setup or teardown flows, which never include setting up a device twice - this should raise.

We can use more strict way to record the state.

    CREATED = "created"
    SETUP = "setup"
    TEARDOWN = "teardown"

    class Device():
        def __init__(self):
            self.state = CREATED
        def setup(self):
            assert self.state is CREATED
            self.state = SETUP
        def teardown(self):
            assert self.state is SETUP
            self.state = TEARDOWN

Using this we can delete about half of the assertions in the tests. Instead of testing:

    assert device.was_setup
    assert device.was_teardown

We can check:

    assert device.state is TEARDOWN

The only way the test will succeed is if the setup and teardown were called exactly once in the correct order.

Also the test failure would be much nicer:

    "setup" != "teardown"

Instead of:

    True is not False

What do you think?
Line 424:         self.was_setup = False
Line 425:         self.was_teardown = False
Line 426: 
Line 427:     @recorded


-- 
To view, visit https://gerrit.ovirt.org/55135
To unsubscribe, visit https://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I3f99b855de43cff693b99b6873a835b7ad56db1b
Gerrit-PatchSet: 12
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Milan Zamazal <mzamazal at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: gerrit-hooks <automation at ovirt.org>
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list