<div dir="ltr"><div><p class="">Hi Matthew,</p>
<p class=""> </p>
<p class=""><a name="_MailOriginal">> -----Original Message-----</a></p>
<p class="">> From:
Matthew Treinish [mailto:<a href="mailto:mtreinish@kortar.org">mtreinish@kortar.org</a>]</p>
<p class="">> Sent:
Saturday, May 10, 2014 12:29 AM</p>
<p class="">> To:
OpenStack Development Mailing List (not for usage questions)</p>
<p class="">> Subject:
Re: [openstack-dev] [qa] Checking for return codes in tempest client</p>
<p class="">> calls</p>
<p class="">> </p>
<p class="">> On Thu, May
08, 2014 at 09:50:03AM -0400, David Kranz wrote:</p>
<p class="">> > On
05/07/2014 10:48 AM, Ken'ichi Ohmichi wrote:</p>
<p class="">> > >Hi
Sean,</p>
<p class="">> > ></p>
<p class="">> >
>2014-05-07 23:28 GMT+09:00 Sean Dague <<a href="mailto:sean@dague.net"><span style="color:windowtext;text-decoration:none">sean@dague.net</span></a>>:</p>
<p class="">> >
>>On 05/07/2014 10:23 AM, Ken'ichi Ohmichi wrote:</p>
<p class="">> >
>>>Hi David,</p>
<p class="">> >
>>></p>
<p class="">> >
>>>2014-05-07 22:53 GMT+09:00 David Kranz <<a href="mailto:dkranz@redhat.com"><span style="color:windowtext;text-decoration:none">dkranz@redhat.com</span></a>>:</p>
<p class="">> >
>>>>I just looked at a patch <a href="https://review.openstack.org/#/c/90310/3"><span style="color:windowtext;text-decoration:none">https://review.openstack.org/#/c/90310/3</span></a></p>
<p class="">> >
>>>>which was given a -1 due to not checking that every call to</p>
<p class="">> >
>>>>list_hosts returns 200. I realized that we don't have a shared</p>
<p class="">> >
>>>>understanding or policy about this. We need to make sure that
each</p>
<p class="">> >
>>>>api is tested to return the right response, but many tests need
to</p>
<p class="">> >
>>>>call multiple apis in support of the one they are actually</p>
<p class="">> >
>>>>testing. It seems silly to have the caller check the response
of</p>
<p class="">> >
>>>>every api call. Currently there are many, if not the majority
of,</p>
<p class="">> >
>>>>cases where api calls are made without checking the response
code.</p>
<p class="">> >
>>>>I see a few</p>
<p class="">> >
>>>>possibilities:</p>
<p class="">> >
>>>></p>
<p class="">> >
>>>>1. Move all response code checking to the tempest clients. They</p>
<p class="">> >
>>>>are already checking for failure codes and are now doing</p>
<p class="">> >
>>>>validation of json response and headers as well. Callers would</p>
<p class="">> >
>>>>only do an explicit check if there were multiple success codes</p>
<p class="">> possible.</p>
<p class="">> >
>>>></p>
<p class="">> >
>>>>2. Have a clear policy of when callers should check response
codes</p>
<p class="">> >
>>>>and apply it.</p>
<p class="">> >
>>>></p>
<p class="">> >
>>>>I think the first approach has a lot of advantages. Thoughts?</p>
<p class="">> >
>>>Thanks for proposing this, I also prefer the first approach.</p>
<p class="">> >
>>>We will be able to remove a lot of status code checks if going on</p>
<p class="">> >
>>>this direction.</p>
<p class="">> >
>>>It is necessary for bp/nova-api-test-inheritance tasks also.</p>
<p class="">> >
>>>Current <a href="https://review.openstack.org/#/c/92536/"><span style="color:windowtext;text-decoration:none">https://review.openstack.org/#/c/92536/</span></a> removes status code</p>
<p class="">> >
>>>checks because some Nova v2/v3 APIs return different codes and the</p>
<p class="">> >
>>>codes are already checked in client side.</p>
<p class="">> >
>>></p>
<p class="">> >
>>>but it is necessary to create a lot of patch for covering all API
tests.</p>
<p class="">> >
>>>So for now, I feel it is OK to skip status code checks in API tests</p>
<p class="">> >
>>>only if client side checks are already implemented.</p>
<p class="">> >
>>>After implementing all client validations, we can remove them of</p>
<p class="">> >
>>>API tests.</p>
<p class="">> >
>>Do we still have instances where we want to make a call that we know</p>
<p class="">> >
>>will fail and not through the exception?</p>
<p class="">> >
>></p>
<p class="">> >
>>I agree there is a certain clarity in putting this down in the rest</p>
<p class="">> >
>>client. I just haven't figured out if it's going to break some</p>
<p class="">> >
>>behavior that we currently expect.</p>
<p class="">> > >If
a server returns unexpected status code, Tempest fails with client</p>
<p class="">> >
>validations like the following sample:</p>
<p class="">> > ></p>
<p class="">> >
>Traceback (most recent call last):</p>
<p class="">> >
> File</p>
<p class="">> >
>"/opt/stack/tempest/tempest/api/compute/servers/test_servers.py",</p>
<p class="">> >
>line 36, in test_create_server_with_admin_password</p>
<p class="">> >
> resp, server =
self.create_test_server(adminPass='testpassword')</p>
<p class="">> >
> File
"/opt/stack/tempest/tempest/api/compute/base.py", line 211,</p>
<p class="">> > >in
create_test_server</p>
<p class="">> >
> name, image_id, flavor,
**kwargs)</p>
<p class="">> >
> File</p>
<p class="">> ></p>
<p class="">> >"/opt/stack/tempest/tempest/services/compute/json/servers_client.py",</p>
<p class="">> >
>line 95, in create_server</p>
<p class="">> >
>
self.validate_response(schema.create_server, resp, body)</p>
<p class="">> >
> File
"/opt/stack/tempest/tempest/common/rest_client.py", line 596,</p>
<p class="">> > >in
validate_response</p>
<p class="">> >
> raise exceptions.InvalidHttpSuccessCode(msg)</p>
<p class="">> >
>InvalidHttpSuccessCode: The success code is different than the</p>
<p class="">> >
>expected one</p>
<p class="">> >
>Details: The status code(202) is different than the expected</p>
<p class="">> >
>one([200])</p>
<p class="">> > ></p>
<p class="">> > ></p>
<p class="">> >
>Thanks</p>
<p class="">> >
>Ken'ichi Ohmichi</p>
<p class="">> > ></p>
<p class="">> >
>_______________________________________________</p>
<p class="">> >
>OpenStack-dev mailing list</p>
<p class="">> > ><a href="mailto:OpenStack-dev@lists.openstack.org"><span style="color:windowtext;text-decoration:none">OpenStack-dev@lists.openstack.org</span></a></p>
<p class="">> > ><a href="http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev"><span style="color:windowtext;text-decoration:none">http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev</span></a></p>
<p class="">> > Note
that there are currently two different methods on RestClient that</p>
<p class="">> > do
this sort of thing. Your stacktrace shows "validate_response" which</p>
<p class="">> >
expects to be passed a schema. The other is "expected_success" which</p>
<p class="">> > takes
the expected response code and is only used by the image</p>
<p class="">> >
clients.</p>
<p class="">> > Both
of these will need to stay around since not all APIs have defined</p>
<p class="">> >
schemas but the expected_success method should probably be changed to</p>
<p class="">> > accept
a list of valid success responses rather than just one as it</p>
<p class="">> > does
at present.</p>
<p class="">> </p>
<p class="">> So
expected_success() is just a better way of doing something like:</p>
<p class="">> </p>
<p class="">> assert.Equals(resp.status,
200)</p>
<p class="">> </p>
<p class="">> There isn't
anything specific about the images clients with it.</p>
<p class="">> validate_response()
should just call expected_success(), which I pushed out</p>
<p class="">> here:</p>
<p class="">> <a href="https://review.openstack.org/93035"><span style="color:windowtext;text-decoration:none">https://review.openstack.org/93035</span></a></p>
<p class=""><span style="color:black"> </span></p>
<p class=""><span style="color:black">There can be possibility to have multiple success return
code (Nova Server Ext events API return 200 & 207 as success code).
Currently there is no such API schema but we need to consider this case. In validate_response(),
it was handled and we should expand the expected_success() also for the same. I
have put this in review comment also.</span></p>
<p class=""><span style="color:black"> </span></p>
<p class="">> </p>
<p class="">> </p>
<p class="">> ></p>
<p class="">> > I hope
we can get agreement to move response checking to the client.</p>
<p class="">> > There
was no opposition when we started doing this in nova to check</p>
<p class="">> >
schema. Does any one see a reason to not do this? It would both</p>
<p class="">> >
simplify the code and make sure responses are checked in all cases.</p>
<p class="">> ></p>
<p class="">> > Sean,
do you have a concrete example of what you are concerned about</p>
<p class="">> > here?
Moving the check from the value returned by a client call to</p>
<p class="">> > inside
the client code should not have any visible effect unless the</p>
<p class="">> > value
was actually wrong but not checked by the caller. But this would</p>
<p class="">> > be a
bug that was just found if a test started failing.</p>
<p class="">> ></p>
<p class="">> </p>
<p class="">> Please
draft a spec/bp for doing this, we can sort out the implementation</p>
<p class="">> details in
the spec review. There is definitely some overlap with the</p>
<p class="">> jsonschema
work though so we need to think about how to best integrate</p>
<p class="">> the 2
efforts. For example, for projects that don't use jsonschema yet does it</p>
<p class="">> make sense
to start using jsonschema files like we do for nova tests to veriy</p>
<p class="">> the status
codes. Just so we can have a common path for doing this. I think</p>
<p class="">> there may
be value in doing it that way. We can discuss it more during the</p>
<p class="">> jsonschema
summit session.</p>
<p class="">> </p>
<p class="">> </p>
<p class="">> -Matt
Treinish</p>
<p class=""><br></p></div><div><br></div>-- <br><div>Thanks & Regards</div><div>Ghanshyam Mann</div><div><br></div>
</div>