Launchpad itself

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

  1. ignored-asserts
  2. Merge into devel
Proposed by Max Bowsher
Status: Merged
Merged at revision: not available
Proposed branch: lp:~maxb/launchpad/ignored-asserts
Merge into: lp:launchpad
Diff against target: 99 lines (+21/-8)
5 files modified
lib/lp/soyuz/doc/initialise-from-parent.txt (+13/-0)
lib/lp/translations/model/translatablemessage.py (+1/-1)
lib/lp/translations/utilities/rosettastats.py (+1/-1)
lib/lp/translations/utilities/translation_import.py (+2/-2)
scripts/ftpmaster-tools/initialise-from-parent.py (+4/-4)
To merge this branch: bzr merge lp:~maxb/launchpad/ignored-asserts
Reviewer Review Type Date Requested Status
Abel Deuring (community) code Approve
Graham Binns (community) code Needs Fixing
Review via email: mp+18889@code.launchpad.net

Commit message

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

To post a comment you must log in.
Revision history for this message
Max Bowsher (maxb) wrote :

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.

Revision history for this message
Max Bowsher (maxb) wrote :

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

Revision history for this message
Graham Binns (gmb) wrote :

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)
Revision history for this message
Max Bowsher (maxb) 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?

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?

Revision history for this message
Graham Binns (gmb) wrote :

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.

Revision history for this message
Max Bowsher (maxb) wrote :

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

Revision history for this message
Abel Deuring (adeuring) :
review: Approve (code)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
23 ... 'The Foobuntu', 'yeck', 'doom',23 ... 'The Foobuntu', 'yeck', 'doom',
24 ... '888', hoary, hoary.owner)24 ... '888', hoary, hoary.owner)
2525
26
27The script will check that there are no NEEDSBUILD builds in the parent
28distroseries' release pocket, so we need to tweak the status of the NEEDSBUILD
29builds that exist in Ubuntu Hoary in the sampledata:
30
31 >>> from lp.buildmaster.interfaces.buildbase import BuildStatus
32 >>> from lp.registry.interfaces.pocket import PackagePublishingPocket
33
34 >>> pending_builds = hoary.getBuildRecords(BuildStatus.NEEDSBUILD,
35 ... pocket=PackagePublishingPocket.RELEASE)
36 >>> for build in pending_builds:
37 ... build.buildstate = BuildStatus.FAILEDTOBUILD
38
26 >>> import transaction39 >>> import transaction
27 >>> transaction.commit()40 >>> transaction.commit()
2841
2942
=== 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 @@
34 Both potmsgset and pofile must be related, meaning they refer to the34 Both potmsgset and pofile must be related, meaning they refer to the
35 same `IPOTemplate` instance.35 same `IPOTemplate` instance.
36 """36 """
37 assert (pofile.potemplate.getPOTMsgSetByID(potmsgset.id) != None,37 assert pofile.potemplate.getPOTMsgSetByID(potmsgset.id) != None, (
38 "POTMsgSet and POFile must refer to the same POTemplate.")38 "POTMsgSet and POFile must refer to the same POTemplate.")
3939
40 self.potmsgset = potmsgset40 self.potmsgset = potmsgset
4141
=== 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 @@
62 """See IRosettaStats."""62 """See IRosettaStats."""
63 untranslated = self.messageCount() - self.translatedCount(language)63 untranslated = self.messageCount() - self.translatedCount(language)
64 # Statistics should not be ever less than 064 # Statistics should not be ever less than 0
65 assert (untranslated >= 0,65 assert untranslated >= 0, (
66 'Stats error in %r id %d, %d untranslated' % (66 'Stats error in %r id %d, %d untranslated' % (
67 self, self.id, untranslated))67 self, self.id, untranslated))
68 return untranslated68 return untranslated
6969
=== 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 @@
595 def __init__(self, translation_import_queue_entry, importer, logger):595 def __init__(self, translation_import_queue_entry, importer, logger):
596 """Construct an Importer for a translation template."""596 """Construct an Importer for a translation template."""
597597
598 assert(translation_import_queue_entry.pofile is None,598 assert translation_import_queue_entry.pofile is None, (
599 "Pofile must be None when importing a template.")599 "Pofile must be None when importing a template.")
600600
601 # Call base constructor601 # Call base constructor
@@ -671,7 +671,7 @@
671 def __init__(self, translation_import_queue_entry, importer, logger):671 def __init__(self, translation_import_queue_entry, importer, logger):
672 """Construct an Importer for a translation file."""672 """Construct an Importer for a translation file."""
673673
674 assert(translation_import_queue_entry.pofile is not None,674 assert translation_import_queue_entry.pofile is not None, (
675 "Pofile must not be None when importing a translation.")675 "Pofile must not be None when importing a translation.")
676676
677 # Call base constructor677 # Call base constructor
678678
=== 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 @@
117 pending_builds = parentseries.getBuildRecords(117 pending_builds = parentseries.getBuildRecords(
118 BuildStatus.NEEDSBUILD, pocket=PackagePublishingPocket.RELEASE)118 BuildStatus.NEEDSBUILD, pocket=PackagePublishingPocket.RELEASE)
119119
120 assert (pending_builds.count() == 0,120 assert pending_builds.count() == 0, (
121 'Parent must not have PENDING builds')121 'Parent must not have PENDING builds')
122122
123def check_queue(distroseries):123def check_queue(distroseries):
@@ -143,11 +143,11 @@
143 PackageUploadStatus.UNAPPROVED,143 PackageUploadStatus.UNAPPROVED,
144 pocket=PackagePublishingPocket.RELEASE)144 pocket=PackagePublishingPocket.RELEASE)
145145
146 assert (new_items.count() == 0,146 assert new_items.count() == 0, (
147 'Parent NEW queue must be empty')147 'Parent NEW queue must be empty')
148 assert (accepted_items.count() == 0,148 assert accepted_items.count() == 0, (
149 'Parent ACCEPTED queue must be empty')149 'Parent ACCEPTED queue must be empty')
150 assert (unapproved_items.count() == 0,150 assert unapproved_items.count() == 0, (
151 'Parent UNAPPROVED queue must be empty')151 'Parent UNAPPROVED queue must be empty')
152152
153def copy_architectures(distroseries):153def copy_architectures(distroseries):