Code review comment for lp:~santhoshkumar/network-service/quantum_testing_framework

Revision history for this message
Salvatore Orlando (salvatore-orlando) wrote :

Hi Santosh!

It seems the testing infrastructure is pretty much vanilla code taken from glance/nova, so it is fine for me.

As far as the functional tests are concerned, your failure is not related to the bug I mentioned earlier. The tests fail as simplejson.load fails to de-serialize the response body. I noticed that wsgi.Serializer().deserialize(response, content_type) works better, even though XML deserialization does not work properly.

Anyway, once the deserialization issue is fixed I got the following error:

TypeError: NetworkInUse() is not JSON serializable

And this is the TypeError I was talking about earlier on.

However, since the issues are not related to the code you are contributing, but concern code which is already in trunk... I think merging pep8 fixes and the testing infrastructure is the right thing to do.

I will cast my "Approve" vote as soon as Dan/Rick approve the branch.

Cheers,
Salvatore

> > Hi Salvatore,
> >
> > Thanks for the feedback. We fixed the ImportError and the usage of
> > load_paste_app from config.py.
> >
> > We had modified the load_paste_app as it has an advantage of loading the
> .conf
> > file from a default set of locations similar to other Openstack projects. In
> > addition to that, load_paste_app will also return the configuration
> variables
> > defined in the .conf file which can be further used for other
> configurations.
> >
> > In wsgi.py, the 'arg_dict' is already using 'request' key to pass the
> request
> > variable to controller actions. After all these change we verified that app
> > can be started and a simple request to get networks works fine.
> >
> > We couldn't get the smoke tests passing both on the trunk and our branch.
> May
> > be we are missing some setup on our side. All the functional tests were
> > failing with the following error:
> >
> > <code> TypeError: [NetworkInUse() is not JSON serializable]. </code>
> >
> > We are not sure if these errors are due to our changes.
>
> Hi Santosh,
> I'm already aware of the TypeError. It seems that response for Faults cannot
> be generated when JSON is the content type. I will push a fix for this issue
> with a separate bug.
>
> For load_paste_app, also the previous code was loading the .conf file from a
> default set of locations... anyway, as long as the app successfully starts, it
> is fine!
>
> I will do a complete review in a few hours.
>
> Cheers,
> Salvatore

review: Needs Information

« Back to merge proposal