2014-05-10 0:29 GMT+09:00 Matthew Treinish <mtrein...@kortar.org>: > On Thu, May 08, 2014 at 09:50:03AM -0400, David Kranz wrote: >> On 05/07/2014 10:48 AM, Ken'ichi Ohmichi wrote: >> >Hi Sean, >> > >> >2014-05-07 23:28 GMT+09:00 Sean Dague <s...@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 <dkr...@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@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. > > So expected_success() is just a better way of doing something like: > > assert.Equals(resp.status, 200) > > There isn't anything specific about the images clients with it. > validate_response() should just call expected_success(), which I pushed out > here: > https://review.openstack.org/93035 > > >> >> 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. >> > > Please draft a spec/bp for doing this, we can sort out the implementation > details in the spec review. There is definitely some overlap with the > jsonschema > work though so we need to think about how to best integrate the 2 efforts. For > example, for projects that don't use jsonschema yet does it make sense to > start > using jsonschema files like we do for nova tests to veriy the status codes. > Just > so we can have a common path for doing this. I think there may be value in > doing > it that way. We can discuss it more during the jsonschema summit session.
OK, I will add this as one of the session topics. Thanks Ken'ichi Ohmichi _______________________________________________ OpenStack-dev mailing list OpenStack-dev@lists.openstack.org http://lists.openstack.org/cgi-bin/mailman/listinfo/openstack-dev