Hi Somik, thanks for your review! See my replies inline.
> Great work on this changelist Salvatore! This was much needed. I do have a few > questions, while not directly related to this branch, they did bring up > questions in my mind around our official API Spec? I felt the Error codes > weren't correct and looking at the API Spec wiki, it seems the wiki has > another completely separate list of error codes. > > Which is the API spec that you are working off? Just so that we can all be on > the same page and update the spec. from a single source. I think besides that > we are very close to checking in. I just wouldn't want our unit test to giving > us "green" light even when they are diverging from our Spec., thats why I had > these questions. > > 1) The WSGI logging, at debug level can be helpful. I think the test log already includes WSGI log as well. Maybe we can increase the amount of debug information logged at that level to make test logs more explicative. > > 2) What's the motivation for introducing options in the ABC? Currently all > plugin specific config is inside the plugin. Why introduce this for > FakePlugin? I was kind of expecting this question :) I use them to pass information from the API router to the plugin; for the Fake plugin, it is just the name of sqlite db to use. I thought it was a good thing to have, as this mechanism will allow the API router to pass configuration options to the plugin (see also bug #803086). However, if you don't deem it necessary we can have FakePlugin using an hard-coded db. > > 3) def _test_show_network_not_found(self, format): -- HTTP code for Not Found > is 404, why are we asserting 420? > Error code is the section of the API spec which was subject to major number of changes. We decided to have more explicative error codes than 404 when a specific object managed by Quantum was not found. > 4) def _test_rename_network(self, format): -- The final assert violates the > data format of specified in quantum_plugin_base. The 'id' should be 'net-id' > and name should be 'net-name' I agree there's a problem here, but it is not in the unit tests. The view builders currently implemented don't follow the API convention. 'id' and 'name' are returned as attributes, consistently with Openstack API specification. Our spec instead refers to net-id and net-name elements. I think best course of action is to stay with our spec at the moment, and then discuss whether we should change it. > > 5) Also as for 202, shouldn't we use 200:OK ( Request Completed) rather than > 202: Accepted ( accepted for processing without guarantees of completion.) > I followed Openstack API conventions: create operations return 200, delete/update 202. However, I can easily make 200 for everything and remove sources of possible confusion. > 6) def _test_delete_network_in_use(self, format): -- I see 421 here, no idea > what does that represent. > 421 is the error code we defined for 'network in use' - which then has changed to 460 > 7) def _test_show_port_portnotfound(self, format): -- ERROR code 430? > Same as above - but then it has disappeare from the error list > 8) _test_delete_port_in_use(self, format): - We have 432 here, can we use well > defined error codes from here - > http://www.w3.org/Protocols/rfc2616/rfc2616-sec10.html > Are you referring to 404? > 9) I also noticed that the error are not consistent with what we have in the > API Spec - http://wiki.openstack.org/QuantumAPISpec but we can tackle those as > bugs but our unit tests should reflect them as bugs. Yep, I noticed that. That is probably because the spec was reviewed after the first draft of the API was implemented. We definitely need to synchronize spec and code. This is one of the thing I noticed while developin the unit tests. My opinion is that any discussion on the API spec will involve discussion, and therefore time. I was therefore aiming at merging the unit tests, which are semantically correct, and then start a discussion around fixing the API, as I mentioned at the end of today's meeting. What's your opinion? Should we first sync code and spec and then merge unit tests. Alternatively we could sync the unit tests alone with the API spec, and merge them. As you might expect most of the test will fail. Then we can push bug fixes to align the API code with the spec. -- https://code.launchpad.net/~netstack/quantum/quantum-unit-tests/+merge/68308 Your team Netstack is subscribed to branch lp:~netstack/quantum/quantum-unit-tests. -- Mailing list: https://launchpad.net/~netstack Post to : netstack@lists.launchpad.net Unsubscribe : https://launchpad.net/~netstack More help : https://help.launchpad.net/ListHelp
