Launchpad itself

Code review comment for lp:~james-w/launchpad/expose-blueprints

expose-blueprints
Merge into devel

Revision history for this message

Leonard Richardson (leonardr) wrote on 2010-07-16:

> > I don't have any opinion on the whiteboard thing, but I'll make sure someone
> with an opinion looks at this branch.

Curtis, do you have any objection to publishing the blueprint whiteboard through the web service?

> That was something I picked up from the work I merged from ajmitch. goal
> is a bit more work as it is not a simple attribute, so I didn't bother
> working on that, I just left goalstatus exported for now.

I'd rather remove goalstatus for now (to avoid cluttering up the interface), or do a little work to export goal as well. I think exporting goal is just a matter of changing the Attribute to an IReference(schema=???), but I don't know what that ??? is. Is it a distro series? Project series? Some abstract class representing the union of all possible series? Curtis, do you know?

If we can figure this out, and the schema is something that's already exported through the web service, then it should be easy to export goal.

> > 2. "I added two new methods to IHasSpecifications, as if we exposed
> > the existing attributes we would serve the collections when just
> > getting the specification." Can you explain? There are many places
> > where we publish a collection associated with an entry. (Just within
> > IPerson we have 'languages', 'gpg_keys', 'pending_gpg_keys',
> > 'wiki_names', 'irc_nicknames', etc.) Just getting the person doesn't
> > give you these collections--it gives you links to the collections.
>
> Ok, I still think methods are more appropriate for this given parallels
> with e.g. searchTasks, but I can change if you feel strongly.

searchTasks() needs to be a named operation because it takes arguments. These attributes here are more like IBug.bug_tasks.

> Also, doesn't exposing collection attributes require them to have a
> traversal? I don't think these do, as there is just /~person/+specs that
> takes query arguments.

Exposing a _top-level_ collection requires a traversal, but lazr.restful automatically takes care of traversal from an object to one of its associated collections.

> > 3. Rather than publishing updateLifecycleStatus as a named operation, I
> > think you could publish it as the mutator for the 'completer' field. I
> > know that you're not publishing 'completer' right now, but I imagine
> > it will be published eventually, and publishing it now with a mutator
> > means that end-users will have to learn one less strangely-named named
> > operation.
> >
> > The only catch is that updateLifecycleStatus() returns None if the
> > lifecycle is not in a state for being completed. It would be better to
> > return a 400 response code, which probably means raising a certain
> > exception. (I'm fuzzy on the details but I can help you with this when
> > the time comes.) We'd write a new method just for use in the web
> > service, which wrapped an updateLifecycleStatus() call with
> > appropriate exception-raising code.
> >
> > (At the very least, call this operation something like 'complete', and
> > pass in the REQUEST_USER automatically--I don't think it makes much
> > sense to complete a blueprint on someone else's behalf.)
>
> I completely missed that this was exposed and there are no tests for
> it. I would lean towards deferring it all to a later branch.

Ok, get rid of it for now.

> > = Questions about the tests =
> >
> > 1. You changed the default priority for a specification created through
> > the factory to LOW, and then changed some tests that were assuming a
> > factory-created specification started out UNDEFINED. Is it easy to
> > leave the earlier behavior alone? Or are specifications with UNDEFINED
> > priority considered invalid, or don't show up in lists, or anything else?
>
> I can leave the earlier behaviour alone, but the tests should still be
> updated to request what they assert. In that case I don't think it
> matters whether they get LOW or UNDEFINED.

Ok, I'm fine with the current behavior.

> > 2. You changed the factory's makeSpecification method to take a lot of
> > new arguments, which is fine, but you also changed
> > ISpecificationSet.new() to take the same arguments, which seems like
> > overkill. Can makeSpecification create a generic specification object
> > and then customize it?
>
> It could, but I was just following the pattern I have seen everywhere
> else in the factory. I'm happy to change if you feel strongly.

I'm not familiar with the factory behavior and it looked weird to me. I'll defer to Curtis.

> > 3. Due to limitations of launchpadlib and the web service there's no
> > way around constructing the URL to a product when loading it in
> > launchpadlib. But, once you have that URL, is it possible to navigate
> > to a specific spec through launchpadlib object traversal? If so, I'd
> > like to see this in the test. If not, I think this branch should
> > publish some way of getting from an IHasSpecification to an
> > ISpecification, given the name--probably as a named operation.
>
> This is ISpecificationTarget.getSpecification isn't it?
>
> I don't know if you can traverse objects to get a spec, but I don't
> think you can. How can I find out for sure?

Yes, I missed getSpecification when I wrote this.

Try something like this:

def getSpecOnWebservice(self, spec_object):
    ...
    pillar = launchpadlib.load(str(launchpadlib._root_uri) + '/' + pillar_name)
    return pillar.getSpecification(spec_object.name)

Let me know if you can't get it to work. I would really like to get rid of the launchpadlib.load() entirely, but this branch is not the appropriate place.

Oh, another thing I forgot to mention is that once we/you upgrade Launchpad's launchpadlib, you should be able to pass relative URLs into Launchpad.load()

> > 4. In IHasSpecificationsTests, you could save a lot of setup code by
> > writing a helper methods that takes a list of (name, status) tuples
> > and creates corresponding specifications.
>
> Except that the target object argument differs plenty, so it's only two
> tests per target. Do you suggest **kwargs or similar?

Sorry, I missed that. Use your best judgement, but give refactoring a shot--there's a lot of samey code here. **kwargs sounds like a good place to start.

> > 5. Your test_representation_contains_* methods test the web service
> > representation against the data used to create the specification
> > object. They should be testing the web service representation against
> > the data inside the specification object itself.
> >
> > For instance,
> >
> > self.assertEqual(self.url, spec.specification_url)
> >
> > should be:
> >
> > self.assertEqual(self.spec_object.specurl, spec.specification_url)
> >
> > And what do we want to test here, really? lazr.restful is tested
> > separately, so we already know that export() works. We want to verify
> > that 1) export() was called on certain fields and not others, and 2)
> > when neccessary, export was called with extra arguments like
> > export_as. I would be satisfied with shorter tests that look at
> > lp_entries, lp_attributes, and lp_collections to make sure the proper
> > fields were exported: and then more detailed tests for any fields
> > whose export() calls took special arguments:
>
> Given that I have had problems before with something apparently being
> exported fine given LP's webservice tests, but not being able to access
> that thing using launchpadlib, I wanted to test the whole stack.

Fair enough. Would you be okay with a test that compared the entire representation as received by launchpadlib against some expected value? Take a look at lib/lp/bugs/stories/webservice/xx-bug.txt. It uses pprint_entry to display the full representation of an object derived from a JSON document.

The difference between that test and your test is that your test does an end-to-end test with launchpadlib. From launchpadlib you can get an object that can be passed into pprint_entry with:

entry._wadl_resource.representation

I know that's a little ugly but if you're interested in using this in tests, we can make it part of the official launchpadlib API.

> > 8. The tests might run faster if you only created one launchpadlib
> > instance and shared it across test methods. Don't take my word for
> > this, though.
>
> They probably would yes, they are currently very slow.
>
> How do I share it across test methods though? Create a new layer?

I believe so. Call it WebServiceClientLayer? There was a time when a launchpadlib instance was started up in one of the existing layers, but we removed that because it slowed the tests down too much.

As a quick hack, defining it in the setup() method would at least let you share it within a test class.

> > 9. What do the changes in sprint-links.txt mean?
>
> As blueprints are now exported their json representation is now dumped
> in the pages for them.
>
> This includes all the attributes, including one that was "Proposed" but
> unrelated to the test.
>
> It would have been tricky to update test to avoid matching that one,
> and it's not worth it anyway, as the templates have changed and the page
> doesn't even use those strings anymore even when the blueprint is in the
> state it is checking for.

This sounds good to me, but I'd like Curtis to approve this change.

> > I don't have any opinion on the whiteboard thing, but I'll make sure someone
> with an opinion looks at this branch.

Curtis, do you have any objection to publishing the blueprint whiteboard through the web service?

> > 2. "I added two new methods to IHasSpecifications, as if we exposed
> > the existing attributes we would serve the collections when just
> > getting the specification." Can you explain? There are many places
> > where we publish a collection associated with an entry. (Just within
> > IPerson we have 'languages', 'gpg_keys', 'pending_gpg_keys',
> > 'wiki_names', 'irc_nicknames', etc.) Just getting the person doesn't
> > give you these collections--it gives you links to the collections.
> 
> Ok, I still think methods are more appropriate for this given parallels
> with e.g. searchTasks, but I can change if you feel strongly.

searchTasks() needs to be a named operation because it takes arguments. These attributes here are more like IBug.bug_tasks.

> Also, doesn't exposing collection attributes require them to have a
> traversal? I don't think these do, as there is just /~person/+specs that
> takes query arguments.

Exposing a _top-level_ collection requires a traversal, but lazr.restful automatically takes care of traversal from an object to one of its associated collections.

> > 3. Rather than publishing updateLifecycleStatus as a named operation, I
> > think you could publish it as the mutator for the 'completer' field. I
> > know that you're not publishing 'completer' right now, but I imagine
> > it will be published eventually, and publishing it now with a mutator
> > means that end-users will have to learn one less strangely-named named
> > operation.
> >
> > The only catch is that updateLifecycleStatus() returns None if the
> > lifecycle is not in a state for being completed. It would be better to
> > return a 400 response code, which probably means raising a certain
> > exception. (I'm fuzzy on the details but I can help you with this when
> > the time comes.) We'd write a new method just for use in the web
> > service, which wrapped an updateLifecycleStatus() call with
> > appropriate exception-raising code.
> >
> > (At the very least, call this operation something like 'complete', and
> > pass in the REQUEST_USER automatically--I don't think it makes much
> > sense to complete a blueprint on someone else's behalf.)
> 
> I completely missed that this was exposed and there are no tests for
> it. I would lean towards deferring it all to a later branch.

Ok, get rid of it for now.

> > = Questions about the tests =
> >
> > 1. You changed the default priority for a specification created through
> > the factory to LOW, and then changed some tests that were assuming a
> > factory-created specification started out UNDEFINED. Is it easy to
> > leave the earlier behavior alone? Or are specifications with UNDEFINED
> > priority considered invalid, or don't show up in lists, or anything else?
> 
> I can leave the earlier behaviour alone, but the tests should still be
> updated to request what they assert. In that case I don't think it
> matters whether they get LOW or UNDEFINED.

Ok, I'm fine with the current behavior.

> > 2. You changed the factory's makeSpecification method to take a lot of
> > new arguments, which is fine, but you also changed
> > ISpecificationSet.new() to take the same arguments, which seems like
> > overkill. Can makeSpecification create a generic specification object
> > and then customize it?
> 
> It could, but I was just following the pattern I have seen everywhere
> else in the factory. I'm happy to change if you feel strongly.

I'm not familiar with the factory behavior and it looked weird to me. I'll defer to Curtis.

> > 3. Due to limitations of launchpadlib and the web service there's no
> > way around constructing the URL to a product when loading it in
> > launchpadlib. But, once you have that URL, is it possible to navigate
> > to a specific spec through launchpadlib object traversal? If so, I'd
> > like to see this in the test. If not, I think this branch should
> > publish some way of getting from an IHasSpecification to an
> > ISpecification, given the name--probably as a named operation.
> 
> This is ISpecificationTarget.getSpecification isn't it?
> 
> I don't know if you can traverse objects to get a spec, but I don't
> think you can. How can I find out for sure?

Yes, I missed getSpecification when I wrote this.

Try something like this:

def getSpecOnWebservice(self, spec_object):
    ...
    pillar = launchpadlib.load(str(launchpadlib._root_uri) + '/' + pillar_name)
    return pillar.getSpecification(spec_object.name)

Let me know if you can't get it to work. I would really like to get rid of the launchpadlib.load() entirely, but this branch is not the appropriate place.

Oh, another thing I forgot to mention is that once we/you upgrade Launchpad's launchpadlib, you should be able to pass relative URLs into Launchpad.load()

> > 4. In IHasSpecificationsTests, you could save a lot of setup code by
> > writing a helper methods that takes a list of (name, status) tuples
> > and creates corresponding specifications.
> 
> Except that the target object argument differs plenty, so it's only two
> tests per target. Do you suggest **kwargs or similar?

Sorry, I missed that. Use your best judgement, but give refactoring a shot--there's a lot of samey code here. **kwargs sounds like a good place to start.

> > 5. Your test_representation_contains_* methods test the web service
> > representation against the data used to create the specification
> > object. They should be testing the web service representation against
> > the data inside the specification object itself.
> >
> > For instance,
> >
> >   self.assertEqual(self.url, spec.specification_url)
> >
> > should be:
> >
> >   self.assertEqual(self.spec_object.specurl, spec.specification_url)
> >
> > And what do we want to test here, really? lazr.restful is tested
> > separately, so we already know that export() works. We want to verify
> > that 1) export() was called on certain fields and not others, and 2)
> > when neccessary, export was called with extra arguments like
> > export_as. I would be satisfied with shorter tests that look at
> > lp_entries, lp_attributes, and lp_collections to make sure the proper
> > fields were exported: and then more detailed tests for any fields
> > whose export() calls took special arguments:
> 
> Given that I have had problems before with something apparently being
> exported fine given LP's webservice tests, but not being able to access
> that thing using launchpadlib, I wanted to test the whole stack.

The difference between that test and your test is that your test does an end-to-end test with launchpadlib. From launchpadlib you can get an object that can be passed into pprint_entry with:

entry._wadl_resource.representation

I know that's a little ugly but if you're interested in using this in tests, we can make it part of the official launchpadlib API.

> > 8. The tests might run faster if you only created one launchpadlib
> > instance and shared it across test methods. Don't take my word for
> > this, though.
> 
> They probably would yes, they are currently very slow.
> 
> How do I share it across test methods though? Create a new layer?

I believe so. Call it WebServiceClientLayer? There was a time when a launchpadlib instance was started up in one of the existing layers, but we removed that because it slowed the tests down too much.

As a quick hack, defining it in the setup() method would at least let you share it within a test class.

> > 9. What do the changes in sprint-links.txt mean?
> 
> As blueprints are now exported their json representation is now dumped
> in the pages for them.
> 
> This includes all the attributes, including one that was "Proposed" but
> unrelated to the test.
> 
> It would have been tricky to update test to avoid matching that one,
> and it's not worth it anyway, as the templates have changed and the page
> doesn't even use those strings anymore even when the blueprint is in the
> state it is checking for.

This sounds good to me, but I'd like Curtis to approve this change.

« Back to merge proposal