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

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 test using your own assertion which does all the required checks... just a bit more readable.

but this bit:

+ self.assertEqual(
+ pmount_source.sourcepackagerelease.getBuildByArch(
+ foobuntu['i386'], foobuntu.main_archive), None)

looks like it should be a separate test (which could use the same self._initialise_distroseries()), with an appropriate method comment based on the doctest (ie. *why* doesn't this build exist... the doctest takes 3 paragraphs to explain :/)

And then this bit:

+ created_build = pmount_source.sourcepackagerelease.createBuild(
+ foobuntu['i386'], PackagePublishingPocket.RELEASE,
+ foobuntu.main_archive)
+ retrieved_build = pmount_source.sourcepackagerelease.getBuildByArch(
+ foobuntu['i386'], foobuntu.main_archive)
+ self.assertEqual(retrieved_build.id, created_build.id)

looks like a separate test also (again which could use the same self._initialise_distroseries()).

The following isn't commented in the doctest... does it belong with the above? If so, what are we ensuring here that is different from the similar test for foobuntu['i386']... if it's nothing new, I'd remove it:

+ self.assertEqual(
+ pmount_source.sourcepackagerelease.getBuildByArch(
+ foobuntu['hppa'], foobuntu.main_archive), None)

And this last bit looks like it could belong with the self.assertDistroSeriesEqual() (or whatever you call it), but needs the comment from the doctest

+ self.assertTrue(
+ foobuntu.isSourcePackageFormatPermitted(
+ SourcePackageFormat.FORMAT_1_0))

+
+ def test_script(self):
+ foobuntu = self._create_distroseries(self.hoary)
+ self._set_pending_to_failed(self.hoary)
+ transaction.commit()
+ ifp = os.path.join(
+ config.root, 'scripts', 'ftpmaster-tools',
+ 'initialise-from-parent.py')
+ process = subprocess.Popen(
+ [sys.executable, ifp, "-vv", "-d", "ubuntutest", "foobuntu"],
+ stdout=subprocess.PIPE, stderr=subprocess.PIPE)
+ stdout, stderr = process.communicate()
+ self.assertEqual(process.returncode, 0)
+ self.assertTrue(
+ "DEBUG Committing transaction." in stderr.split('\n'))
+ hoary_pmount_pubs = self.hoary.getPublishedReleases(+ foobuntu_pmount_pubs = foobuntu.getPublishedReleases('pmount')
+ self.assertEqual(len(hoary_pmount_pubs), len(foobuntu_pmount_pubs))
'pmount')

I assume you could again use the self.assertDistroSeriesEqual() (with a better name).

Thanks!