Code review comment for lp:~stevenk/launchpad/switch-ifp-to-unittests

Hi Steve,

As per our IRC conv., unless there's a reason for not creating a custom assertion such as assertDistroSeriesInitializdCorrectly(), please rename _check_distroseries() to follow normal unit-test assertion names.

Also, this method (whether it is renamed or not) should definitely not be creating builds... it should only be checking/asserting things. That's why I really think it should be a separate test (see comment above where it's identified).

And lastly, you didn't comment on the _initiale_distroseries() - ah, it'd only be necessary/useful to avoid duplicate code if you had the separate test as above.

{{{
12:15 < noodles> StevenK: I assumed when you said earlier that it adds nothing to the test run, that you'd isolated the positive tests, but the diff on the MP still only has test_initialize? Also any reason for not calling _check_distroseries assertDistroSeriesInitialized(distro1, distro2) or similar? (it is a custom assertion right?)
12:19 < jelmer> bigjools: While I'm changing the BinaryPackageRelease table, would it be ok if I also added the field for Homepage?
12:19 < bigjools> jelmer: what's that for?
12:19 < jelmer> bigjools: I have to touch the code that fills it in, and it would help for bug 319196 and bug 602385
12:19 < edbot> Bug #319196: On the source+ page, provide external links <https://launchpad.net/bugs/319196> <soyuz [Triaged/Low] No assignee>
12:19 < edbot> Bug #602385: Allow SPs to register an upstream project <https://launchpad.net/bugs/602385> <launchpad-registry [In Progress/High] Edwin
               Grubbs>
12:20 < bigjools> jelmer: that would need to be on SourcePackageRelease woundn't it?
12:20 < bigjools> wouldn't, even
12:22 < StevenK> noodles: It shouldn't ...
12:22 < StevenK> noodles: Both test_initialise() and test_script() call self._check_distroseries(foobuntu)
12:22 < bigjools> jelmer: also, I'm not sure about that first bug any more, I think we should do it through packaging links
12:22 < bigjools> as per the 2nd bug
12:23 < bigjools> it might be worth talking to Curtis
12:23 < noodles> StevenK: yes, but they both use it like an assertion, so why not a custom assertion as above?
12:23 < jelmer> bigjools: It would need to be on both spr and bpr I guess, since its in both their control fields
12:23 < jelmer> bigjools: I'll ask him, thanks
12:24 < StevenK> noodles: I can rename the method if you wish, but the intent is clear
12:24 < bigjools> jelmer: I can't see the actual use for it in LP if it's on BPR though
12:24 < noodles> StevenK: I don't mind... I was just wondering if there was a reason for not using standard unit-test conventions?
12:24 < StevenK> noodles: I was trying to get away with changing as little code as possible
12:25 < StevenK> It worked, too.
}}}