Merge lp:~maxb/launchpad/ignored-asserts into lp:launchpad

[Max Bowsher]

Fix assert statements which were not being tested owing to misplaced parentheses.

[Max Bowsher (maxb)]

The Python assert statement is not a function, and treating it as such - i.e.:

   assert (a == b, "The things weren't equal!")

results in evaluating a 2-element tuple as a boolean, thus, it's approximately the same as 'assert True', i.e. a no-op.

Python 2.6 recognizes this flaw, and adds a SyntaxWarning to report this case. Thus, by running:

   find -name \*.py -type f | xargs python2.6 -m py_compile

I've located all instances of the problem in the Launchpad source.

Here's a branch applying the requisite syntax fixes.

[Max Bowsher (maxb)]

AssertionError: Parent must not have PENDING builds is triggering in lib/lp/soyuz/tests/../doc/initialise-from-parent.txt

[Graham Binns (gmb)]

Hi Max,

Nice catch! I like it. You should add this check to bin/lint.sh so that make lint will pick up this error for us.

I'm marking this needs fixing but only because I want the change to the linter in there as well.

review: Needs Fixing (code)

[Max Bowsher (maxb)]

> Nice catch! I like it. You should add this check to bin/lint.sh so that make
> lint will pick up this error for us.

Graham,

Are you sure you really want bin/lint.sh to depend on having a python2.6 installed?

In any case, once Launchpad properly migrates to Python 2.6 in the future, Python itself will be linting this.

Though, I guess this is "Needs fixing" anyway, since we need to resolve that Soyuz test failure before this branch can land. Should we aim to fix the soyuz test first, or to land this branch with the soyuz asserts commented out and XXXed?

[Graham Binns (gmb)]

On 9 February 2010 11:26, Max Bowsher <email address hidden> wrote:
>> Nice catch! I like it. You should add this check to bin/lint.sh so that make
>> lint will pick up this error for us.
>
> Graham,
>
> Are you sure you really want bin/lint.sh to depend on having a python2.6 installed?
>

Ah, good point. I forget sometimes that not everyone runs on Karmic.
However, we could have a conditional that runs the check if 2.6 is
available.

> In any case, once Launchpad properly migrates to Python 2.6 in the future, Python itself will be linting this.
>

True. Do we have a timetable for that? All the ML threads I've seen
are vague about it.

> Though, I guess this is "Needs fixing" anyway, since we need to resolve that Soyuz test failure before this branch can land. Should we aim to fix the soyuz test first, or to land this branch with the soyuz asserts commented out and XXXed?

We should fix the test, certainly. If the test is very broken, you
should speak to Julian about it.

[Max Bowsher (maxb)]

> On 9 February 2010 11:26, Max Bowsher wrote:
> >> Nice catch! I like it. You should add this check to bin/lint.sh so that
> >> make lint will pick up this error for us.
> >
> > Are you sure you really want bin/lint.sh to depend on having a python2.6
> > installed?
>
> Ah, good point. I forget sometimes that not everyone runs on Karmic.
> However, we could have a conditional that runs the check if 2.6 is
> available.
>
> > In any case, once Launchpad properly migrates to Python 2.6 in the future,
> > Python itself will be linting this.
>
> True. Do we have a timetable for that? All the ML threads I've seen
> are vague about it.

Let's not bother with adding anything to lint.sh, since the Python 2.6 migration seems to be getting under way with Salgado doing some work on it, and the impending release of Lucid means it will become a high priority.

> > Though, I guess this is "Needs fixing" anyway, since we need to resolve that
> > Soyuz test failure before this branch can land. Should we aim to fix the soyuz
> > test first, or to land this branch with the soyuz asserts commented out and
> > XXXed?
>
> We should fix the test, certainly. If the test is very broken, you
> should speak to Julian about it.

I have now fixed the test, thanks to guidance from wgrant.

Ready for re-review and landing.

[Abel Deuring (adeuring)]

review: Approve (code)

=== modified file 'lib/lp/soyuz/doc/initialise-from-parent.txt'
--- lib/lp/soyuz/doc/initialise-from-parent.txt	2009-11-14 02:55:15 +0000
+++ lib/lp/soyuz/doc/initialise-from-parent.txt	2010-04-09 11:45:42 +0000
@@ -23,6 +23,19 @@
   ...                                 'The Foobuntu', 'yeck', 'doom',
   ...                                 '888', hoary, hoary.owner)
 
+
+The script will check that there are no NEEDSBUILD builds in the parent
+distroseries' release pocket, so we need to tweak the status of the NEEDSBUILD
+builds that exist in Ubuntu Hoary in the sampledata:
+
+  >>> from lp.buildmaster.interfaces.buildbase import BuildStatus
+  >>> from lp.registry.interfaces.pocket import PackagePublishingPocket
+
+  >>> pending_builds = hoary.getBuildRecords(BuildStatus.NEEDSBUILD,
+  ...     pocket=PackagePublishingPocket.RELEASE)
+  >>> for build in pending_builds:
+  ...     build.buildstate = BuildStatus.FAILEDTOBUILD
+
   >>> import transaction
   >>> transaction.commit()
 

=== modified file 'lib/lp/translations/model/translatablemessage.py'
--- lib/lp/translations/model/translatablemessage.py	2009-08-26 16:24:02 +0000
+++ lib/lp/translations/model/translatablemessage.py	2010-04-09 11:45:42 +0000
@@ -34,7 +34,7 @@
         Both potmsgset and pofile must be related, meaning they refer to the
         same `IPOTemplate` instance.
         """
-        assert (pofile.potemplate.getPOTMsgSetByID(potmsgset.id) != None,
+        assert pofile.potemplate.getPOTMsgSetByID(potmsgset.id) != None, (
           "POTMsgSet and POFile must refer to the same POTemplate.")
 
         self.potmsgset = potmsgset

=== modified file 'lib/lp/translations/utilities/rosettastats.py'
--- lib/lp/translations/utilities/rosettastats.py	2009-07-17 00:26:05 +0000
+++ lib/lp/translations/utilities/rosettastats.py	2010-04-09 11:45:42 +0000
@@ -62,7 +62,7 @@
         """See IRosettaStats."""
         untranslated = self.messageCount() - self.translatedCount(language)
         # Statistics should not be ever less than 0
-        assert (untranslated >= 0,
+        assert untranslated >= 0, (
             'Stats error in %r id %d, %d untranslated' % (
                 self, self.id, untranslated))
         return untranslated

=== modified file 'lib/lp/translations/utilities/translation_import.py'
--- lib/lp/translations/utilities/translation_import.py	2010-02-23 08:25:39 +0000
+++ lib/lp/translations/utilities/translation_import.py	2010-04-09 11:45:42 +0000
@@ -595,7 +595,7 @@
     def __init__(self, translation_import_queue_entry, importer, logger):
         """Construct an Importer for a translation template."""
 
-        assert(translation_import_queue_entry.pofile is None,
+        assert translation_import_queue_entry.pofile is None, (
             "Pofile must be None when importing a template.")
 
         # Call base constructor
@@ -671,7 +671,7 @@
     def __init__(self, translation_import_queue_entry, importer, logger):
         """Construct an Importer for a translation file."""
 
-        assert(translation_import_queue_entry.pofile is not None,
+        assert translation_import_queue_entry.pofile is not None, (
             "Pofile must not be None when importing a translation.")
 
         # Call base constructor

=== modified file 'scripts/ftpmaster-tools/initialise-from-parent.py'
--- scripts/ftpmaster-tools/initialise-from-parent.py	2010-03-19 21:26:13 +0000
+++ scripts/ftpmaster-tools/initialise-from-parent.py	2010-04-09 11:45:42 +0000
@@ -117,7 +117,7 @@
     pending_builds = parentseries.getBuildRecords(
         BuildStatus.NEEDSBUILD, pocket=PackagePublishingPocket.RELEASE)
 
-    assert (pending_builds.count() == 0,
+    assert pending_builds.count() == 0, (
             'Parent must not have PENDING builds')
 
 def check_queue(distroseries):
@@ -143,11 +143,11 @@
         PackageUploadStatus.UNAPPROVED,
         pocket=PackagePublishingPocket.RELEASE)
 
-    assert (new_items.count() == 0,
+    assert new_items.count() == 0, (
             'Parent NEW queue must be empty')
-    assert (accepted_items.count() == 0,
+    assert accepted_items.count() == 0, (
             'Parent ACCEPTED queue must be empty')
-    assert (unapproved_items.count() == 0,
+    assert unapproved_items.count() == 0, (
             'Parent UNAPPROVED queue must be empty')
 
 def copy_architectures(distroseries):