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

On Fri, 16 Jul 2010 13:05:52 -0000, Leonard Richardson <email address hidden> wrote:
> Review: Needs Information
> Thanks a lot for this branch. This is a branch I was planning to create as a way of introducing Benji to the web service, but instead we went through your branch and reviewed it.
>
> I don't have any opinion on the whiteboard thing, but I'll make sure someone with an opinion looks at this branch.
>
> = Questions about the exported objects =
>
> 1. Obviously you don't need to export everything to make this branch
> useful, but I wonder why you exported ISpecification.goalstatus but
> not ISpecification.goal.

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.

> 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.

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.

> 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.

> = 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.

> 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.

> 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?

> 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?

> 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.

> # Make sure that 'specurl' is published as 'specification_url'
> self.assertEqual(self.spec_object.specurl, spec.specification_url)
>
> Of course, it doesn't hurt to also test the identity between
> spec_object.name and spec.name. If you want to do this, define the
> code in a helper method and then invoke the method in every test
> method, to make the amount of code and the risk of copy-paste errors
> as small as possible.

I can do that.

> 7. Write a helper method for the
> test_*_implements_IHasSpecification{s,Target} methods to avoid
> copy-and-paste errors. (For instance,
> test_distribution_implements_IHasSpecifications is a copy of
> test_product_implements_IHasSpecifications, or was last time I
> checked.)

Ok.

> 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?

> 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.

Thanks,

James