Inline with SB>> On Tue, Jul 19, 2011 at 4:46 PM, Salvatore Orlando < salvatore.orla...@eu.citrix.com> wrote:
> 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. > > Ok, If it doesn't break backward compatibility, which it doesn't seem to do, I have no issue with this change. > > > > 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. > > > Can we do either one of these things: a) Update the view builder with out agreed upon spec guidelines - i.e. "net-name" and "net-id" b) Leave it the way it is for this change and open a bug to address that issue. > > > > 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. > > This seems ok, we can discuss changing to 200 separately as part of API spec review later. > > 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? > I was talking about using 404 but we can defer this to API spec review. > > > 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<https://code.launchpad.net/%7Enetstack/quantum/quantum-unit-tests/+merge/68308> > You are reviewing the proposed merge of > lp:~netstack/quantum/quantum-unit-tests into lp:quantum. > I vote that we file bugs for things we know coming out of this CL and then we can merge the this branch, and start working on high priority API bugs. -- Somik Behera | Nicira Networks, Inc. | so...@nicira.com <sbeh...@nicira.com> | office: 650-390-6790 | cell: 512-577-6645 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
