Code review comment for lp:~michael.nelson/launchpad/656166-expose-dsd-diffs

Looks great :)

Three fairly trivial comments.

+1

[1]

+ def test_package_diffs(self):
+ # The package diff urls exposed.
+ ds_diff = self.factory.makeDistroSeriesDifference()
+ naked_dsdiff = removeSecurityProxy(ds_diff)
+ naked_dsdiff.package_diff = self.factory.makePackageDiff(
+ status=PackageDiffStatus.PENDING)
+ naked_dsdiff.parent_package_diff = self.factory.makePackageDiff()
+
+ ws_diff = ws_object(self.factory.makeLaunchpadService(
+ ds_diff.owner), ds_diff)
+
+ self.assertEqual(None, ws_diff.package_diff_url)
+ self.assertTrue(ws_diff.parent_package_diff_url.startswith(
+ 'http://localhost:58000/'))

What does this demonstrate? Is that a link to the librarian? If so can
you add a comment explaining what it is.

I'm a little bit worried that this test is fragile, after reading some
of the discussions on launchpad-dev about just-in-time configuration
of the librarian, amongst other things. Maybe I got the wrong end of
the stick. Anyway, it seems to me that it's enough just to show that
parent_package_diff_url is not None.

On that note, assertIs(None, ...) and assertIsNot(None, ...) might be
more appropriate for testing against None, but I don't think it
actually matters. Ah, I see you've used it later.

[2]

+ pubs = self.derived_series.getPublishedSources(
+ self.source_package_name, version=self.base_version,
+ include_pending=True)
+ return pubs[0]

Could getPublishedSources() ever return an empty list?

[3]

+ base_version = versions.get('base')
+ if base_version:

s/:/ is not None:/