Open Stack

On 07/10/2013 03:38 AM, Giulio Fidente wrote:
> On 07/09/2013 11:01 PM, Jay Pipes wrote:
>> On 07/08/2013 08:49 AM, Giulio Fidente wrote:
>>> On 07/02/2013 04:01 PM, David Kranz wrote:
>>>>
>>>> Resource management
>>>> There was a discussion last year about whether resource management
>>>> should be in the client or in the test class. I will try to dig it up
>>>> from the old launchpad qa list if I can. We ended up managing this in
>>>> the test classes. This is a complicated issue. If you have a proposal
>>>> for how to do it differently the best thing would be to put up a WIP
>>>> patch with the essence of the base class structure and flow you are
>>>> proposing for both client and test classes.
>>>
>>> I wasn't any active at the time but let me add my 2cents to that
>>> discussion now.
>>>
>>> I don't think the choice to manage the resources in the test classes is
>>> bad but I find confusing and prone to errors that the resources are
>>> actually managed part in the base class and part in the test class; I'm
>>> thinking about the resources cleanup facilities for instance or the user
>>> credentials.
>>>
>>> The issue with the credentials in particular has been brought up in the
>>> list before.
>>>
>>> Is there some agreement on "fixing" this or an idea on how these things
>>> could be changed?
>>
>> What would you fix? The reason the credentials fixtures are "managed" in
>> the base test classes is to provide absolute isolation for the test
>> cases that subclass those base classes. Do you propose the subclasses be
>> responsible for creating their own tenant isolation?
>
> No, I'd like to see the credentials managed in the base class only.

Agreed, they should be.

> Going trough the code I think I found a couple of places where we could
> improve. For instance, what I *think* is problematic are cases like this:
>
> https://github.com/openstack/tempest/blob/master/tempest/api/volume/admin/test_multi_backend.py#L37
>
> where we define credentials from config in the test class but maybe we
> should can improve that in the base class here:
>
> https://github.com/openstack/tempest/blob/master/tempest/api/volume/base.py#L205
>
> by just adding the needed clients as per:
>
> https://github.com/openstack/tempest/blob/master/tempest/api/volume/base.py#L48
>
> or even define os as os_adm in the base admin class, makes sense?

Yeah, there are a number of issues with the above test case and base 
classes:

1) On line 206: 
https://github.com/openstack/tempest/blob/master/tempest/api/volume/base.py#L206, 
the cls.client is being set to the volume *types* client, not the 
volumes client.

2) The subclassed VolumeMultiBackendTest class is needlessly re-defining 
adm_user, adm_pass, adm_tenant on lines 37-39: 
https://github.com/openstack/tempest/blob/master/tempest/api/volume/admin/test_multi_backend.py#L37 
when those variables are already defined in the base class 
(base.BaseVolumeAdminTest) here: 
https://github.com/openstack/tempest/blob/master/tempest/api/volume/base.py#L197

3) The subclassed VolumeMultiBackendTest is incorrectly setting the 
cls.volumes_client variable on line 42: 
https://github.com/openstack/tempest/blob/master/tempest/api/volume/admin/test_multi_backend.py#L42 
instead of simply doing:

cls.volume_client = cls.os_adm.volume_client

Same exact thing for the volume types client on line 47, which should 
just be:

cls.volume_types_client = cls.os_adm.volume_types_client

> Also, speaking about actual resources and not credentials, would it be
> useful to enforce (trough the reviews) the usage of the utility methods
> provided by the base classes (which also take care of the cleanup)
> rather than create/delete resources in the test class?

Yes, but the test author must be sure that when they use the base class' 
create_XXX resource methods that:

a) The resource they create using the base class' create_XXX method does 
not affect the results of other test methods -- for instance, if 
test_A() creates a server instance that modifies the assumptions that 
test_B() has, that is a bad thing (a side effect) and should be avoided

b) The resource they create using the base class' create_XXX method does 
not require that the test methods in the test class are run in a 
specific order. If it does, that also is a bad side effect and means the 
test methods in the test case class cannot be run in parallel.

Best,
-jay