Code review comment for lp://staging/~julian-edwards/launchpad/schema-distro-parents

Thanks for the review Henning!

On Monday 18 April 2011 11:22:26 you wrote:
> Review: Needs Fixing
> Hi Julian,
> thanks for adding this linking table. You indeed almost had me bored but I
> did find a few of my pet issues in your branch ... ;-)
>
> I'd really like to see the tests cleaned up before I approve this.

Not sure I agree with all of your comments there :-) But read on ...

> >
> > +class EditDistroSeriesParent(AuthorizationBase):
> > + """DistroSeriesParent can be edited by the same people who can edit
> > + the derived_distroseries."""
> > + permission = "launchpad.Edit"
> > + usedfor = IDistroSeriesParent
> > +
> > + def checkAuthenticated(self, user):
> > + return check_permission(self.permission,
> > self.obj.derived_series)
>
> Using check_permission in security adapters is flawed and needs to be
> stopped. Please see bug 764406 as to why. You can fix this by referencing
> EditDistroSeriesByReleaseManagerOrDistroOwnersOrAdmins directly or leave it
> in to be fixed when that bug is fixed. ;)

Woops, fixed :-)

> > === modified file 'lib/lp/registry/configure.zcml'
> > === added file 'lib/lp/registry/interfaces/distroseriesparent.py'
> > === added file 'lib/lp/registry/model/distroseriesparent.py'
>
> Yeah, this is the boring bit ... ;) Thanks for a solid implementation.

I've done it too many times now!

> > + def setUp(self):
> > + TestCaseWithFactory.setUp(self)
> > + #self.distribution =
> > self.factory.makeDistribution(name='conftest')
>
> Oh, an unused setUp!

Bugger, forgot to remove it :-)

> > +
> > + def test_verify_interface(self):
> > + # Test the interface for the model.
> > + dsp = self.factory.makeDistroSeriesParent()
> > + verified = verifyObject(IDistroSeriesParent, dsp)
> > + self.assertTrue(verified)
> > +
> > + def test_properties(self):
> > + # Test the model properties.
> > + parent_series = self.factory.makeDistroSeries()
> > + derived_series = self.factory.makeDistroSeries()
> > + dsp = self.factory.makeDistroSeriesParent(
> > + derived_series=derived_series,
> > + parent_series=parent_series,
> > + initialized=True
> > + )
> > +
> > + self.assertThat(
> > + dsp,
> > + MatchesStructure(
> > + derived_series=Equals(derived_series),
> > + parent_series=Equals(parent_series),
> > + initialized=Equals(True)
> > + ))
> > +
> > + def test_getByDerivedSeries(self):
> > + parent_series = self.factory.makeDistroSeries()
> > + derived_series = self.factory.makeDistroSeries()
> > + self.factory.makeDistroSeriesParent(
> > + derived_series, parent_series)
>
> Hm, why not let makeDistroSeriesParent create the series? Just pick them
> from the dsp object you get from the factory.

I've re-worked these tests entirely so it's actually testing *editing*
(previously it was doing nothing :/) and they are much simpler now.

>
> > + results = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
> > + derived_series)
> > + self.assertEqual(1, results.count())
> > + self.assertEqual(parent_series, results[0].parent_series)
> > +
> > + # Making a second parent should add it to the results.
>
> Uh-oh, two-stage test. I would prefer this in two tests.

I know a few people don't like this pattern, but IMHO that would be bad here
where the second part depends on the first. Splitting tests up makes our test
suite slower and that pisses me off more than multi-stage tests.

>
> > + self.factory.makeDistroSeriesParent(
> > + derived_series, self.factory.makeDistroSeries())
> > + results = getUtility(IDistroSeriesParentSet).getByDerivedSeries(
> > + derived_series)
> > + self.assertEqual(2, results.count())
> > +
> > + def test_getByParentSeries(self):
> > + parent_series = self.factory.makeDistroSeries()
> > + derived_series = self.factory.makeDistroSeries()
> > + dsp = self.factory.makeDistroSeriesParent(
> > + derived_series, parent_series)
>
> Same comment about the factory method here.

Same reply :)

>
> > + results = getUtility(IDistroSeriesParentSet).getByParentSeries(
> > + parent_series)
> > + self.assertEqual(1, results.count())
> > + self.assertEqual(derived_series, results[0].derived_series)
> > +
> > + # Making a second child should add it to the results.
>
> And here about splitting the test. ;-)

And .... same :)

>
> > + self.factory.makeDistroSeriesParent(
> > + self.factory.makeDistroSeries(), parent_series)
> > + results = getUtility(IDistroSeriesParentSet).getByParentSeries(
> > + parent_series)
> > + self.assertEqual(2, results.count())
> > +
> > +
> > +class TestDistroSeriesParentSecurity(TestCaseWithFactory):
> > +
> > + layer = DatabaseFunctionalLayer
> > +
> > + def setUp(self):
> > + super(TestDistroSeriesParentSecurity, self).setUp()
> > + self.parent_series = self.factory.makeDistroSeries()
> > + self.derived_series = self.factory.makeDistroSeries()
> > + self.person = self.factory.makePerson()
>
> In case you didn't notice, I don't think using setUp for sample data
> creation is a good idea. In this case, self.person is only used in two
> tests, why not just create them there? And the series could be created by
> makeDistroSeriesParent, just like mentioned in the previous test case.
> Please remove setUp from this test case.

I was someone who disagreed with your email about this pattern as I take each
case on its own merits ;)

I've re-written the testst a bit anyway and negated the need for a setup at
all. See what you think.

> > +
> > + def test_random_person_is_unauthorized(self):
> > + dsp_set = getUtility(IDistroSeriesParentSet)
> > + dsp = dsp_set.new(self.derived_series, self.parent_series,
> > False) + with person_logged_in(self.person):
> > + self.assertRaises(
> > + Unauthorized,
> > + setattr, dsp, "derived_series", self.parent_series)
> > +
> > + def assertCanEdit(self):
> > + dsp_set = getUtility(IDistroSeriesParentSet)
> > + dsp = dsp_set.new(
> > + self.derived_series, self.parent_series, False)
> > + self.assertThat(
> > + dsp,
> > + MatchesStructure(
> > + derived_series=Equals(self.derived_series),
> > + parent_series=Equals(self.parent_series),
> > + initialized=Equals(False)
> > + ))
> > +
> > + def test_distro_drivers_can_edit(self):
> > + # Test that distro drivers can edit the data.
> > + login(LAUNCHPAD_ADMIN)
> > + self.derived_series.distribution.driver = self.person
> > + with person_logged_in(self.person):
> > + self.assertCanEdit()
> > +
> > + def test_admins_can_edit(self):
> > + login(LAUNCHPAD_ADMIN)
> > + self.assertCanEdit()
> > +
> > + def test_distro_owners_can_edit(self):
> > + login(LAUNCHPAD_ADMIN)
> > + self.derived_series.distribution.owner = self.person
> > + with person_logged_in(self.person):
> > + self.assertCanEdit()
> >
> > === modified file 'lib/lp/testing/factory.py'
>
> Thanks for adding the factory to go with this new class.

My pleasure. Thanks for reviewing, let me know what you think.

Cheers
J