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

On Thu, Oct 7, 2010 at 5:58 PM, Gavin Panella
<email address hidden> wrote:
> Review: Approve
> Looks great :)
>
> Three fairly trivial comments.

Thanks for the review Gavin, comments below.

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

Done.

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

Given that we know the base version was published in the distroseries'
main archive, we can be sure that we can get it back... however, you
prompted me to check getPublishedSources() which only returns
currently PUBLISHED or PENDING sources, which was fine for my tests,
but in reality the base version will be superseded. So I've updated my
tests and refactored this to ensure we return the specific
base-version irrespective of its status. I added a comment too,
explaining why pubs[0] is ok (or why I'd use pubs.one() if it was a
storm result set).

>
>
> [3]
>
> +        base_version = versions.get('base')
> +        if base_version:
>
> s/:/ is not None:/

Done. Thanks! Incremental here: http://pastebin.ubuntu.com/508671/