Code review comment for lp:~jcsackett/launchpad/shortlist-696913

Hi Jon,

I'm glad to see you address this nuisance. You'll make all our lives easier. Danilo was planning to look into it as well this week, since we were getting some of the errors on Translations, but this may just save him the time.

The big question I have though is: could this change confuse clients written to our 1.0 API? We made a promise to keep that API stable. Or do we consider it a plain bug in the client if it can't cope with a collection no longer being snapshotted?

I'm voting "needs fixing," but please don't take that as a vote of no confidence in your approach. It's merely that I'm requesting some changes to the test code that would probably warrant a new review round.

> === modified file 'lib/lp/blueprints/tests/test_hasspecifications.py'
> --- lib/lp/blueprints/tests/test_hasspecifications.py 2010-11-29 18:04:07 +0000
> +++ lib/lp/blueprints/tests/test_hasspecifications.py 2011-01-12 13:35:47 +0000

> @@ -178,3 +184,49 @@
> product=product, name="spec3")
> self.assertNamesOfSpecificationsAre(
> ["spec1"], person.valid_specifications)
> +
> +
> +class HasSpecificationsSnapshotTestCase(TestCaseWithFactory):
> + """A TestCase for snapshots of specification targets."""
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super(HasSpecificationsSnapshotTestCase, self).setUp()

This method looks redundant. Might as well leave it out AFAICS.

> + def check_omissions(self, target, providing):

What omissions are being checked, exactly?

The word "omission" has strong negative connotations: it generally implies that something is missing that shouldn't be. If this method is meant to check that collections are _properly omitted_ from the snapshot, maybe there's some better word.

> + """snapshots of objects with IHasSpecification should not snapshot
> + marked attributes.

Please start your sentence with a capital letter!

This is a statement of fact, but the first line of a docstring is expected to describe what the method actually does for the caller.

> + wrap an export with 'donotsnapshot' to force the snapshot to not
> + include that attribute.

Please start your sentence with a capital letter!

Is the imperative an instruction to this method's caller? Or a description of what the method does in the imperative form? Or general advice to programmers?

> + """
> + snapshot = Snapshot(target, providing=providing)

It's probably just my shallow understanding of Zope, but I would have expected "providing=IHasSpecifications" to work here.

> + omitted = [
> + 'all_specifications',
> + 'valid_specifications',
> + ]
> + for attribute in omitted:
> + self.assertFalse(
> + hasattr(snapshot, attribute),
> + "snapshot should not include %s." % attribute)

Please start your sentence with a capital letter!

> === modified file 'lib/lp/registry/tests/test_milestone.py'
> --- lib/lp/registry/tests/test_milestone.py 2010-10-04 19:50:45 +0000
> +++ lib/lp/registry/tests/test_milestone.py 2011-01-12 13:35:47 +0000

> @@ -81,6 +91,48 @@
> [1, 2, 3])
>
>
> +class HasMilestonesSnapshotTestCase(TestCaseWithFactory):
> + """A TestCase for snapshots of pillars with milestones."""
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super(HasMilestonesSnapshotTestCase, self).setUp()

This one looks redundant again.

> + def check_omissions(self, target, providing):
> + """snapshots of objects with IHasSpecification should not snapshot
> + marked attributes.
> +
> + wrap an export with 'donotsnapshot' to force the snapshot to not
> + include that attribute.
> + """

Mind those capitals.

You're repeating the checking code here, but mixing in the data difference that's specific to this test. Please see if you can put the method somewhere reusable so it gets maintained in one place.

One way to sculpt this would be as a test matcher. See lib/lp/testing/matchers.py.

> + snapshot = Snapshot(target, providing=providing)
> + omitted = [
> + 'milestones',
> + 'all_milestones',
> + ]
> + for attribute in omitted:
> + self.assertFalse(
> + hasattr(snapshot, attribute),
> + "snapshot should not include %s." % attribute)

When reusing this code, it'd be useful for the error message to state target's type as well.

> + def test_product(self):
> + product = self.factory.makeProduct()
> + self.check_omissions(product, IProduct)
> +
> + def test_distribution(self):
> + distribution = self.factory.makeDistribution()
> + self.check_omissions(distribution, IDistribution)
> +
> + def test_distroseries(self):
> + distroseries = self.factory.makeDistroSeries()
> + self.check_omissions(distroseries, IDistroSeries)
> +
> + def test_projectgroup(self):
> + projectgroup = self.factory.makeProject()
> + self.check_omissions(projectgroup, IProjectGroup)
> +
> +
> def test_suite():
> """Return the test suite for the tests in this module."""
> return unittest.TestLoader().loadTestsFromName(__name__)

We no longer need test_suite.

> === modified file 'lib/lp/registry/tests/test_productseries.py'
> --- lib/lp/registry/tests/test_productseries.py 2010-10-18 20:40:05 +0000
> +++ lib/lp/registry/tests/test_productseries.py 2011-01-12 13:35:47 +0000

> @@ -301,5 +305,32 @@
> self.productseries.getLatestRelease())
>
>
> +class ProductSeriesSnapshotTestCase(TestCaseWithFactory):
> + """A TestCase for snapshots of productseries."""
> +
> + layer = DatabaseFunctionalLayer
> +
> + def setUp(self):
> + super(ProductSeriesSnapshotTestCase, self).setUp()

Redundant again, I think. Or am I missing something?

> + def test_productseries(self):
> + """snapshots of objects with IHasSpecification should not snapshot
> + marked attributes.
> +
> + wrap an export with 'donotsnapshot' to force the snapshot to not
> + include that attribute.
> + """

For a moment there you had me wondering where the test methods were! :-)

Anyway, for completeness' sake: comments as above.

> + productseries = self.factory.makeProductSeries()
> + snapshot = Snapshot(productseries, providing=IProductSeries)
> + omitted = [
> + 'milestones',
> + 'all_milestones',
> + ]
> + for attribute in omitted:
> + self.assertFalse(
> + hasattr(snapshot, attribute),
> + "snapshot should not include %s." % attribute)
> +
> +
> def test_suite():
> return TestLoader().loadTestsFromName(__name__)

Again for completeness' sake: this bit of boilerplate is now thankfully obsolete.

Jeroen