Merge lp:~stevenk/launchpad/switch-ifp-to-unittests into lp:launchpad

Hi Steve,

So, your new tests look okay, though I'd like to get a second opinion on whether they completely replace the existing doctests since I find the doctests quite torturous to read.

However, the new tests are sorely in need of comments. Firstly, each test_ method should have a comment or docstring (I prefer comments for tests, FTR) explaining what's being tested, in the form:

    def test_frobnob_goes_boing(self):
        # The frobnob will go boing if pushed.

Secondly, I can't tell (because there are no comments to tell me) whether test_initialise() is huge for a reason or whether it can be split into individual tests. I'm guessing that it's huge for a reason, because all the asserts seem to be of the nature of

    foo = bar.foo
    self.assertEqual(expected_foo, foo)

Which is fine, but given the length of the method they should have some explanation with them:

    # The bar's foo will have been updated.
    foo = bar.foo
    self.assertEqual(expected_foo, foo)

As I said, I don't have the context for knowing if the test conversion is accurate, since the old test seems to rely a lot on the output from the script. I'd appreciate it if you could find someone else (Jools maybe) who can confirm that (I'm aware that this is overkill but I'm also aware that this is a Soyuz branch).

I'm marking the branch as needs fixing pending the changes above.

Hi Steven,

It's great to see the doctest being replaced!

In addition to the method comment mentioned by gmb, I'd also be keen to see all the test_failure_* methods be a bit more specific... the easiest way to do this might be to use self.assertRaisesWithContent() which will ensure that the cause of the raised exception is obvious.

There is generally a lot of info in the doctest which is currently removed... it'd be great to include all the pertinent bits (imagine you knew nothing about soyuz and were looking at this test).

I can't see anything missing from the conversion... perhaps one or two things could be removed (see below). Also, regarding the length of test_initialise,

=== added file 'lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py'
--- lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py 1970-01-01 00:00:00 +0000
+++ lib/lp/soyuz/scripts/tests/test_initialise_distroseries.py 2010-08-10 14:55:27 +0000
@@ -0,0 +1,144 @@
<snip>
+
+ def test_initialise(self):

From here...
+ foobuntu = self._create_distroseries(self.hoary)
+ self._set_pending_to_failed(self.hoary)
+ transaction.commit()
+ ids = InitialiseDistroSeries(foobuntu)
+ ids.check()
+ ids.initialise()
to here could be extracted into self._initialise_distroseries() ?

and then from here:
+ hoary_pmount_pubs = self.hoary.getPublishedReleases('pmount')
+ foobuntu_pmount_pubs = foobuntu.getPublishedReleases('pmount')
+ self.assertEqual(len(hoary_pmount_pubs), len(foobuntu_pmount_pubs))
+ hoary_i386_pmount_pubs = self.hoary['i386'].getReleasedPackages(
+ 'pmount')
+ foobuntu_i386_pmount_pubs = foobuntu['i386'].getReleasedPackages(
+ 'pmount')
+ self.assertEqual(
+ len(hoary_i386_pmount_pubs), len(foobuntu_i386_pmount_pubs))
+ pmount_binrel = (
+ foobuntu['i386'].getReleasedPackages(
+ 'pmount')[0].binarypackagerelease)
+ self.assertEqual(pmount_binrel.title, u'pmount-0.1-1')
+ self.assertEqual(pmount_binrel.build.id, 7)
+ self.assertEqual(
+ pmount_binrel.build.title,
+ u'i386 build of pmount 0.1-1 in ubuntu hoary RELEASE')
+ pmount_srcrel = pmount_binrel.build.source_package_release
+ self.assertEqual(pmount_srcrel.title, u'pmount - 0.1-1')
+ foobuntu_pmount = pmount_srcrel.getBuildByArch(
+ foobuntu['i386'], foobuntu.main_archive)
+ hoary_pmount = pmount_srcrel.getBuildByArch(
+ self.hoary['i386'], self.hoary.main_archive)
+ self.assertEqual(foobuntu_pmount.id, hoary_pmount.id)
+ pmount_source = self.hoary.getSourcePackage(
+ 'pmount').currentrelease
+ self.assertEqual(
+ pmount_source.title,
+ '"pmount" 0.1-2 source package in The Hoary Hedgehog Release')
+ pmount_source = foobuntu.getSourcePackage('pmount').currentrelease
+ self.assertEqual(
+ pmount_source.title,
+ '"pmount" 0.1-2 source package in The Foobuntu')

...to here into self.assertDistroSeriesEqual() (hrm.. with a more accurate method name), so you have a nice small tes...

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

@Michael, re custom assertions - have you seen the matcher stuff that
James Westby has been emailing about on the dev list? I'm biased ;),
but I think its really nice and cleaner than custom assertions.

-Rob

> @Michael, re custom assertions - have you seen the matcher stuff that
> James Westby has been emailing about on the dev list? I'm biased ;),
> but I think its really nice and cleaner than custom assertions.
>
> -Rob

Yeah I did... they do look great :) I'd be happy if StevenK wanted to use them, but didn't want to request even more changes. I'll leave that up to StevenK.

Thanks again Steven. It looks good now... there are just some inconsistencies in the comments which I've highlighted with a diff (to save confusion):

http://pastebin.ubuntu.com/476903/

With those fixed up as you see fit, r=me.

I think Michael's review is more than comprehensive enough to cover my original concerns.