nova.image.glance unit tests should not use fake glance service

[Jay Pipes]

The unit tests in nova.tests.image.test_glance unfortunately make use of a faked glance image service in nova.tests.image.fakes. What this does is actually mask a number of problems and makes it harder to assert for specific behavior in the real glanceclient client classes.

The unit tests should be rewritten to simply mock out the very specific code boundaries where tested calls interact with the glanceclient client classes, and that's it. Unit tests should just test one little unit of code, not giant swathes of dependent code.

[Jay Pipes (jaypipes)]

Note that one of the things that gets masked by the current fake glance service is the fact that glanceclient v1 client and glanceclient v2 client classes have very different behaviors for the exact same calls.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/81365

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/81384

[Joe Gordon (jogo)]

Any reason to keep the fake glance image service at all?

[Jay Pipes (jaypipes)]

Not in the unit tests, no, but I suppose it can be useful in the nova/tests/integrated/ stuff... in any case, I'm slowly working to remove as much of its usage as possible, and then re-evaluate when all of it is removed from the true unit tests.

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/81578

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/81365
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=0998bbb18ca2be5a052514473de4846d6f5ee380
Submitter: Jenkins
Branch: master

commit 0998bbb18ca2be5a052514473de4846d6f5ee380
Author: Jay Pipes <email address hidden>
Date: Tue Mar 18 16:31:22 2014 -0400

    GlanceImageService static methods to module scope

    A first step in refactoring/cleaning up the image API in Nova is
    to get the image unit tests to be real unit tests, not functional
    tests or using a fake Glance client. This patch merely moves three
    static methods that were hanging from GlanceImageService and puts
    these private functions at the module scope, where the rest of the
    helper functions reside (these three for some reason were put as
    static methods on the GlanceImageService instead of as private
    functions in the module scope).

    The next series of patches will modify the unit tests in
    nova.tests.image.test_glance to pull out usage of the fake glance
    service and use mocks to properly stress the edges of both the
    v1 and v2 glanceclient client structures.

    Change-Id: If998291bfdb98b1ebe2e46c4ac8fdbae0fcd4bd7
    Related-bug: #1293938

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/81384
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=7dd0780e854e5e9cfc01e93864f37cfd12c0dec9
Submitter: Jenkins
Branch: master

commit 7dd0780e854e5e9cfc01e93864f37cfd12c0dec9
Author: Jay Pipes <email address hidden>
Date: Tue Mar 18 17:58:34 2014 -0400

    Refactors nova.image.glance unit tests for show()

    This patches continues a patch series to cleanup and streamline the unit
    tests for the nova.image.glance module. Here, we create true unit tests
    for the GlanceImageService.show() method, stressing all code paths and
    edges but only for that specific method, using mocks and no fake glance
    image service.

    This patch removes "unit" tests from the TestGlanceImageService that
    called glance.GlanceImageService.show(), as those tests used the fake
    glance service and the code paths are now more than covered by these new
    mock-using tests.

    Change-Id: I200a9e9812d6de011fa6faa8fb8dc570075ce902
    Related-bug: #1293938

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/81578
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=7726e4829411b9166b594b17c6c42c308cf5c68b
Submitter: Jenkins
Branch: master

commit 7726e4829411b9166b594b17c6c42c308cf5c68b
Author: Jay Pipes <email address hidden>
Date: Wed Mar 19 12:50:51 2014 -0400

    Refactors unit tests of image service detail()

    We do not need to have "unit" tests that test the things like
    pagination work with the glanceclient.images.detail() call. glanceclient
    and glance have unit tests for that. What the unit tests for the glance
    image service detail() method in Nova need is to test only the code
    boundary that exists within the detail() method -- i.e. verify that the
    calls to the glanceclient and other helper methods are made with the
    right parameters.

    This patch pulls the various unit tests out of the
    TestGlanceImageService test class and into their own test class that
    uses mocks and verifies each code path within the detail() method.

    Next up is the create, update and delete methods of the
    GlanceImageService class. The final piece is to rework the download unit
    tests (which aren't unit tests but more functional tests that take
    around a second each to run :(

    Change-Id: I94897ff54a36a1af4258ba4a3c88b45c079f4bb5
    Related-bug: #1293938

[OpenStack Infra (hudson-openstack)]

Related fix proposed to branch: master
Review: https://review.openstack.org/85567

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/85630

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/85567
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=de9f5feba4f456248b22aa30c8012f530bd4038d
Submitter: Jenkins
Branch: master

commit de9f5feba4f456248b22aa30c8012f530bd4038d
Author: Jay Pipes <email address hidden>
Date: Sat Apr 5 16:42:16 2014 -0400

    Refactor unit tests for image service CRUD

    Refactors the image service unit tests of the create() method to be unit
    tests and not functional tests. There is no need to test whether an
    image ID that is specified in the payload to the glance image service is
    preserved; the unit and functional tests in the glanceclient package do
    that, and besides, all this was testing was the behavior of the fake
    glance client, which is pointless. There was also a test that created an
    image and then tested the show method would raise InstanceNotFound if
    not passing in the same instance name that was just created. This test
    was removed, as ImageNotFound is already thoroughly tested in the show()
    unit tests.

    Missing in the existing unit tests was a test of failure conditions
    resulting in error codes returned from the glanceclient.create() call,
    and this patch adds a unit test that stresses the failure paths.

    These new unit tests use mocks and assert that all code paths in the
    create(), update() and delete() methods are fully tested.

    Change-Id: I6d903979290b7d027c8dc66d2da712e1e1961a00
    Related-bug: #1293938

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/85630
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=9048078fccfe502a3e1a96a635c6e3fd6480aea7
Submitter: Jenkins
Branch: master

commit 9048078fccfe502a3e1a96a635c6e3fd6480aea7
Author: Jay Pipes <email address hidden>
Date: Mon Apr 7 00:30:21 2014 -0400

    Move _get_locations to module level plus tests

    Moves the nova.image.glance.GlanceImageService._get_locations object
    method into module level scope (the self argument was unused) and
    adds full unit tests for the nova.image.glance._get_location() function.

    Change-Id: Ib386330b70abe1e0353f1fafa3e68bc3e7a2d1b1
    Partial-bug: #1293938

[Tracy Jones (tjones-i)]

Is there anything else needed to close this bug? All reviews are merged

[Jay Pipes (jaypipes)]

Tracy, I will check into it. I believe the download tests are still using the fake service.

[Tracy Jones (tjones-i)]

ping...

[Jay Pipes (jaypipes)]

pong:

https://review.openstack.org/#/c/109893/
https://review.openstack.org/#/c/109892/
https://review.openstack.org/#/c/109894/

Once those are in, I have two more patches for this to be complete.

Prolly shoulda been a blueprint ;)

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/113098

[OpenStack Infra (hudson-openstack)]

Fix proposed to branch: master
Review: https://review.openstack.org/113099

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/113098
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=486623c0ca6d71ace5e6b0cd234be05f10f13dc4
Submitter: Jenkins
Branch: master

commit 486623c0ca6d71ace5e6b0cd234be05f10f13dc4
Author: Jay Pipes <email address hidden>
Date: Sat Aug 9 15:04:57 2014 -0700

    Removes GlanceClient stubs

    Refactors the tests in nova.tests.image.test_glance that were checking
    the behaviour of both the GlanceClientWrapper retry logic and the
    glanceclient.Client creation to use mock instead of the FakeGlanceClient
    in nova.tests.glance.stubs. The fake stub client was actually masking
    issues in the existing test cases, including not properly checking the
    identity headers that are actually supplied to the real
    glanceclient.Client constructor.

    Change-Id: I1c114df8e4ab2fccd966ed4af22181881590c443
    Partial-bug: #1293938

[OpenStack Infra (hudson-openstack)]

Reviewed: https://review.openstack.org/113099
Committed: https://git.openstack.org/cgit/openstack/nova/commit/?id=8a50755b9df445a07140f385f1ff32db20bf683b
Submitter: Jenkins
Branch: master

commit 8a50755b9df445a07140f385f1ff32db20bf683b
Author: Jay Pipes <email address hidden>
Date: Sat Aug 9 15:46:01 2014 -0700

    Remove final use of glance_stubs

    Removes the final piece of glance_stubs from the image unit tests.

    Change-Id: I0db3b6c83edaf91466e85d423ce75b3e75fd3517
    Closes-bug: #1293938