On Fri, 26 Nov 2010 13:11:21 -0000, Guilherme Salgado <email address hidden> wrote:
> Hi Michael,
> 
> Thanks for the review!
> 
> On Fri, 2010-11-26 at 01:12 +0000, Michael Hudson-Doyle wrote:
> > Review: Approve
> > Hi Salgado,
> > 
> > Yay for progress on this issue.  I entirely support the postponing of
> > all the difficult stuff :-)
> > 
> > I have some quibbles about the tests, but nothing serious I hope.
> 
> I like all your suggestions and since it's Friday and I'm all excited
> with the weekend approaching, I'm proposing even more changes. :)

Hah!

> > > === modified file 'lib/canonical/launchpad/interfaces/_schema_circular_imports.py'
> > > --- lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-11-17 22:18:34 +0000
> > > +++ lib/canonical/launchpad/interfaces/_schema_circular_imports.py	2010-11-25 22:50:47 +0000
> > > @@ -34,6 +34,10 @@
> > >      )
> > >  from lp.blueprints.interfaces.specification import ISpecification
> > >  from lp.blueprints.interfaces.specificationbranch import ISpecificationBranch
> > > +from lp.blueprints.interfaces.specificationtarget import (
> > > +    IHasSpecifications,
> > > +    ISpecificationTarget,
> > > +    )
> > >  from lp.bugs.interfaces.bug import (
> > >      IBug,
> > >      IFrontPageBugAddForm,
> > > @@ -516,3 +520,13 @@
> > >
> > >  # IProductSeries
> > >  patch_reference_property(IProductSeries, 'product', IProduct)
> > > +
> > > +# ISpecificationTarget
> > > +patch_entry_return_type(
> > > +    ISpecificationTarget, 'getSpecification', ISpecification)
> > > +
> > > +# IHasSpecifications
> > > +patch_collection_property(
> > > +    IHasSpecifications, 'all_specifications', ISpecification)
> > > +patch_collection_property(
> > > +    IHasSpecifications, 'valid_specifications', ISpecification)
> > 
> > It's a shame that we don't have the app specific circular import files
> > yet... oh well.
> 
> That'd be a slight improvement, but it's still a PITA to maintain these
> "patches".

Yeah.  I saw what you had to do with SprintSpecification too :/

> > > === added file 'lib/lp/blueprints/tests/test_webservice.py'
> > > --- lib/lp/blueprints/tests/test_webservice.py	1970-01-01 00:00:00 +0000
> > > +++ lib/lp/blueprints/tests/test_webservice.py	2010-11-25 22:50:47 +0000
> > > @@ -0,0 +1,410 @@
> > > +# Copyright 2009 Canonical Ltd.  This software is licensed under the
> > > +# GNU Affero General Public License version 3 (see the file LICENSE).
> > > +
> > > +"""Webservice unit tests related to Launchpad blueprints."""
> > > +
> > > +__metaclass__ = type
> > > +
> > > +from canonical.testing import DatabaseFunctionalLayer
> > > +from canonical.launchpad.testing.pages import webservice_for_person
> > > +from lp.blueprints.interfaces.specification import (
> > > +    SpecificationDefinitionStatus,
> > > +    SpecificationPriority)
> > > +from lp.testing import (
> > > +    launchpadlib_for, TestCaseWithFactory)
> > > +
> > > +
> > > +class SpecificationWebserviceTestCase(TestCaseWithFactory):
> > > +
> > > +    def makeProduct(self):
> > > +        return self.factory.makeProduct(name="fooix")
> > > +
> > > +    def makeDistribution(self):
> > > +        return self.factory.makeDistribution(name="foobuntu")
> > 
> > I don't particularly like these helpers.  Why can't the tests that
> > care about the names specify the names?
> 
> Yeah, good catch; I got rid of them.

Thanks.

> > > +    def getLaunchpadlib(self):
> > > +        user = self.factory.makePerson()
> > > +        return launchpadlib_for("testing", user, version='devel')
> > > +
> > > +    def getSpecOnWebservice(self, spec_object):
> > > +        launchpadlib = self.getLaunchpadlib()
> > > +        if spec_object.product is not None:
> > > +            pillar_name = spec_object.product.name
> > > +        else:
> > > +            pillar_name = spec_object.distribution.name
> > 
> > I think you can spell this spec_object.target.name can't you?
> 
> Yes! :)
> 
> > 
> > > +        return launchpadlib.load(
> > > +            str(launchpadlib._root_uri) + '/%s/+spec/%s'
> > > +            % (pillar_name, spec_object.name))
> > > +
> > > +    def getPillarOnWebservice(self, pillar_obj):
> > > +        launchpadlib = self.getLaunchpadlib()
> > > +        return launchpadlib.load(
> > > +            str(launchpadlib._root_uri) + '/' + pillar_obj.name)
> > 
> > I'm pretty sure you can pass just '/' + pillar_obj.name here.
> 
> I can indeed, but only for the first occurrence.  The second triggers
> https://bugs.launchpad.net/launchpad-foundations/+bug/681767

:(

> > > +class SpecificationAttributeWebserviceTests(SpecificationWebserviceTestCase):
> > > +    """Test accessing specification attributes over the webservice."""
> > > +    layer = DatabaseFunctionalLayer
> > > +
> > > +    def makeSimpleSpecification(self):
> > > +        self.name = "some-spec"
> > > +        self.title = "some-title"
> > > +        self.url = "http://example.org/some_url"
> > > +        self.summary = "Some summary."
> > > +        status = SpecificationDefinitionStatus.PENDINGAPPROVAL
> > > +        self.definition_status = status.title
> > > +        self.assignee_name = "james-w"
> > > +        assignee = self.factory.makePerson(name=self.assignee_name)
> > > +        self.drafter_name = "jml"
> > > +        drafter = self.factory.makePerson(name=self.drafter_name)
> > > +        self.approver_name = "bob"
> > > +        approver = self.factory.makePerson(name=self.approver_name)
> > > +        self.owner_name = "mary"
> > > +        owner = self.factory.makePerson(name=self.owner_name)
> > > +        priority = SpecificationPriority.HIGH
> > > +        self.priority = priority.title
> > > +        self.whiteboard = "Some whiteboard"
> > > +        self.product = self.factory.makeProduct()
> > > +        return self.factory.makeSpecification(
> > > +            product=self.product, name=self.name,
> > > +            title=self.title, specurl=self.url,
> > > +            summary=self.summary,
> > > +            status=status,
> > > +            assignee=assignee, drafter=drafter, approver=approver,
> > > +            priority=priority,
> > > +            owner=owner, whiteboard=self.whiteboard)
> > > +
> > > +    def getSimpleSpecificationResponse(self):
> > > +        self.spec_object = self.makeSimpleSpecification()
> > > +        return self.getSpecOnWebservice(self.spec_object)
> > > +
> > > +    def test_representation_is_empty_on_1_dot_0(self):
> > > +        # ISpecification is exposed on the 1.0 version so that they can be
> > > +        # linked against branches, but none of its fields is exposed on that
> > > +        # version as we expect it to undergo significant refactorings before
> > > +        # it's ready for prime time.
> > > +        spec = self.makeSimpleSpecification()
> > > +        user = self.factory.makePerson()
> > > +        webservice = webservice_for_person(user)
> > > +        response = webservice.get(
> > > +            '/%s/+spec/%s' % (spec.product.name, spec.name))
> > > +        expected_keys = sorted(
> > > +            [u'self_link', u'http_etag', u'resource_type_link'])
> > > +        self.assertEqual(response.status, 200)
> > > +        self.assertEqual(sorted(response.jsonBody().keys()), expected_keys)
> > > +
> > > +    def test_representation_contains_name(self):
> > > +        spec = self.getSimpleSpecificationResponse()
> > > +        self.assertEqual(self.name, spec.name)
> > 
> > Argh, I hate this style of test.  It's only one line longer to write
> > it as:
> > 
> >     def test_representation_contains_name(self):
> >         spec_object = self.factory.makeSpecification()
> >         spec_webservice = self.getSpecOnWebservice(spec_object)
> >         self.assertEqual(spec_object.name, spec_webservice.name)
> > 
> > and then it avoids the action at a distance effects of these tests.
> > Can you change them all?  If you like, I'll write you a sed script or
> > something to do it :-)
> 
> Nah, I'm happy to do it myself.  I have to admit I didn't bother
> refactoring these tests; I just refactored the ones that were failing
> after the changes I did and the stuff I reverted.

Cool.  Thanks for changing these.

> > > +    def test_representation_contains_target(self):
> > > +        spec = self.getSimpleSpecificationResponse()
> > > +        self.assertEqual(self.product.name, spec.target.name)
> > > +
> > > +    def test_representation_contains_title(self):
> > > +        spec = self.getSimpleSpecificationResponse()
> > > +        self.assertEqual(self.title, spec.title)
> [...]
> 
> > > +class SpecificationTargetTests(SpecificationWebserviceTestCase):
> > > +    """Tests for accessing specifications via their targets."""
> > > +    layer = DatabaseFunctionalLayer
> > > +
> > > +    def test_get_specification_on_product(self):
> > > +        product = self.makeProduct()
> > > +        spec_object = self.factory.makeSpecification(
> > > +            product=product, name="some-spec")
> > > +        product_on_webservice = self.getPillarOnWebservice(product)
> > > +        spec = product_on_webservice.getSpecification(name="some-spec")
> > > +        self.assertEqual("some-spec", spec.name)
> > > +        self.assertEqual("fooix", spec.target.name)
> > > +
> > > +    def test_get_specification_on_distribution(self):
> > > +        distribution = self.makeDistribution()
> > > +        spec_object = self.factory.makeSpecification(
> > > +            distribution=distribution, name="some-spec")
> > > +        distro_on_webservice = self.getPillarOnWebservice(distribution)
> > > +        spec = distro_on_webservice.getSpecification(name="some-spec")
> > > +        self.assertEqual("some-spec", spec.name)
> > > +        self.assertEqual("foobuntu", spec.target.name)
> > > +
> > > +    def test_get_specification_on_productseries(self):
> > > +        product = self.makeProduct()
> > > +        productseries = self.factory.makeProductSeries(
> > > +            product=product, name="fooix-dev")
> > > +        spec_object = self.factory.makeSpecification(
> > > +            product=product, name="some-spec", goal=productseries)
> > > +        product_on_webservice = self.getPillarOnWebservice(product)
> > > +        productseries_on_webservice = product_on_webservice.getSeries(
> > > +            name="fooix-dev")
> > > +        spec = productseries_on_webservice.getSpecification(name="some-spec")
> > > +        self.assertEqual("some-spec", spec.name)
> > > +        self.assertEqual("fooix", spec.target.name)
> > > +
> > > +    def test_get_specification_on_distroseries(self):
> > > +        distribution = self.makeDistribution()
> > > +        distroseries = self.factory.makeDistroSeries(
> > > +            distribution=distribution, name="maudlin")
> > > +        spec_object = self.factory.makeSpecification(
> > > +            distribution=distribution, name="some-spec",
> > > +            goal=distroseries)
> > > +        distro_on_webservice = self.getPillarOnWebservice(distribution)
> > > +        distroseries_on_webservice = distro_on_webservice.getSeries(
> > > +            name_or_version="maudlin")
> > > +        spec = distroseries_on_webservice.getSpecification(name="some-spec")
> > > +        self.assertEqual("some-spec", spec.name)
> > > +        self.assertEqual("foobuntu", spec.target.name)
> > > +
> > > +    def test_get_specification_not_found(self):
> > > +        product = self.makeProduct()
> > > +        product_on_webservice = self.getPillarOnWebservice(product)
> > > +        spec = product_on_webservice.getSpecification(name="nonexistant")
> > > +        self.assertEqual(None, spec)
> > 
> > I'd rather these tests didn't depend on names chosen way up the file
> > too :/
> 
> Now that I've got rid of the makeProduct/makeDistribution helpers they
> don't. :)

:-)

> > > +class IHasSpecificationsTests(SpecificationWebserviceTestCase):
> > > +    """Tests for accessing IHasSpecifications methods over the webservice."""
> > > +    layer = DatabaseFunctionalLayer
> > > +
> > > +    def assertNamesOfSpecificationsAre(self, expected_names, specifications):
> > > +        names = [s.name for s in specifications]
> > > +        self.assertEqual(sorted(expected_names), sorted(names))
> > > +
> > > +    def test_product_all_specifications(self):
> > > +        product = self.makeProduct()
> > > +        self.factory.makeSpecification(product=product, name="spec1")
> > > +        self.factory.makeSpecification(product=product, name="spec2")
> > > +        product_on_webservice = self.getPillarOnWebservice(product)
> > > +        self.assertNamesOfSpecificationsAre(
> > > +            ["spec1", "spec2"], product_on_webservice.all_specifications)
[...]
> > > +    def test_person_valid_specifications(self):
> > > +        person = self.factory.makePerson(name="james-w")
> > > +        product = self.factory.makeProduct()
> > > +        self.factory.makeSpecification(
> > > +            product=product, name="spec1", drafter=person)
> > > +        self.factory.makeSpecification(
> > > +            product=product, name="spec2", approver=person,
> > > +            status=SpecificationDefinitionStatus.OBSOLETE)
> > > +        self.factory.makeSpecification(
> > > +            product=product, name="spec3")
> > > +        launchpadlib = self.getLaunchpadlib()
> > > +        person_on_webservice = launchpadlib.load(
> > > +            str(launchpadlib._root_uri) + '/~james-w')
> > > +        self.assertNamesOfSpecificationsAre(
> > > +            ["spec1"], person_on_webservice.valid_specifications)
> > 
> > I think these tests are fine, but they seem to violate layering a bit
> > -- surely we test the all_specifications property at least this well
> > in other code?
> 
> I wouldn't count on that.  There might be a pagetest somewhere which
> exercises it, but that probably uses (the very limited) sample data and
> so we shouldn't rely on that.

True :/

> And to be honest, I really dislike the tests above, so I'm willing to
> turn them into non-launchpadlib-using tests and move them somewhere
> else.  Then here I'd just show that all_specs/valid_specs is available
> on objects that provide IHasSpecifications.  I think such a test
> together with the new test_implements.py should be enough.  What do you
> think?

That would be great.  I think you can land this branch now though, if
you want.  I had a quick look at your later changes, they look fine
too.

Cheers,
mwh