Open Stack

On 05/07/2014 10:48 AM, Ken'ichi Ohmichi wrote:
> Hi Sean,
>
> 2014-05-07 23:28 GMT+09:00 Sean Dague <sean at dague.net>:
>> On 05/07/2014 10:23 AM, Ken'ichi Ohmichi wrote:
>>> Hi David,
>>>
>>> 2014-05-07 22:53 GMT+09:00 David Kranz <dkranz at redhat.com>:
>>>> I just looked at a patch https://review.openstack.org/#/c/90310/3 which was
>>>> given a -1 due to not checking that every call to list_hosts returns 200. I
>>>> realized that we don't have a shared understanding or policy about this. We
>>>> need to make sure that each api is tested to return the right response, but
>>>> many tests need to call multiple apis in support of the one they are
>>>> actually testing. It seems silly to have the caller check the response of
>>>> every api call. Currently there are many, if not the majority of, cases
>>>> where api calls are made without checking the response code. I see a few
>>>> possibilities:
>>>>
>>>> 1. Move all response code checking to the tempest clients. They are already
>>>> checking for failure codes and are now doing validation of json response and
>>>> headers as well. Callers would only do an explicit check if there were
>>>> multiple success codes possible.
>>>>
>>>> 2. Have a clear policy of when callers should check response codes and apply
>>>> it.
>>>>
>>>> I think the first approach has a lot of advantages. Thoughts?
>>> Thanks for proposing this, I also prefer the first approach.
>>> We will be able to remove a lot of status code checks if going on
>>> this direction.
>>> It is necessary for bp/nova-api-test-inheritance tasks also.
>>> Current https://review.openstack.org/#/c/92536/ removes status code checks
>>> because some Nova v2/v3 APIs return different codes and the codes are already
>>> checked in client side.
>>>
>>> but it is necessary to create a lot of patch for covering all API tests.
>>> So for now, I feel it is OK to skip status code checks in API tests
>>> only if client side checks are already implemented.
>>> After implementing all client validations, we can remove them of API
>>> tests.
>> Do we still have instances where we want to make a call that we know
>> will fail and not through the exception?
>>
>> I agree there is a certain clarity in putting this down in the rest
>> client. I just haven't figured out if it's going to break some behavior
>> that we currently expect.
> If a server returns unexpected status code, Tempest fails with client
> validations
> like the following sample:
>
> Traceback (most recent call last):
>    File "/opt/stack/tempest/tempest/api/compute/servers/test_servers.py",
> line 36, in test_create_server_with_admin_password
>      resp, server = self.create_test_server(adminPass='testpassword')
>    File "/opt/stack/tempest/tempest/api/compute/base.py", line 211, in
> create_test_server
>      name, image_id, flavor, **kwargs)
>    File "/opt/stack/tempest/tempest/services/compute/json/servers_client.py",
> line 95, in create_server
>      self.validate_response(schema.create_server, resp, body)
>    File "/opt/stack/tempest/tempest/common/rest_client.py", line 596,
> in validate_response
>      raise exceptions.InvalidHttpSuccessCode(msg)
> InvalidHttpSuccessCode: The success code is different than the expected one
> Details: The status code(202) is different than the expected one([200])
>
>
> Thanks
> Ken'ichi Ohmichi
>
> _______________________________________________
> OpenStack-dev mailing list
> OpenStack-dev at lists.openstack.org
> http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev
Note that there are currently two different methods on RestClient that 
do this sort of thing. Your stacktrace shows "validate_response" which 
expects to be passed a schema. The other is "expected_success" which 
takes the expected response code and is only used by the image clients.
Both of these will need to stay around since not all APIs have defined 
schemas but the expected_success method should probably be changed to 
accept a list of valid success responses rather than just one as it does 
at present.

I hope we can get agreement to move response checking to the client. 
There was no opposition when we started doing this in nova to check 
schema. Does any one see a reason to not do this? It would both simplify 
the code and make sure responses are checked in all cases.

Sean, do you have a concrete example of what you are concerned about 
here? Moving the check from the value returned by a client call to 
inside the client code should not have any visible effect unless the 
value was actually wrong but not checked by the caller. But this would 
be a bug that was just found if a test started failing.

  -David

  -David