Merge lp:~jtv/launchpad/translationtemplatescollection-test into lp:launchpad

[Jeroen T. Vermeulen]

Tests & simplification for TranslationTemplatesCollection.

= TranslationTemplatesCollection test =

This branch provides intermediate testing for TranslationTemplatesCollection, layered between the existing tests for Collection and for the one class implemented in terms of collections, HasTranslationTemplatesMixin. A silly typo in the TranslationTemplatesCollection revealed a gap in test coverage, which was a good excuse for both fixing the bug and testing the class separately. I should have done it from the start.

This also introduces another piece of convenience for Collection: a concrete collection class based on Collection selects, by default, just the class that it is a collection of. So calling TranslationTemplatesCollection.select() without arguments is now allowed, and returns a sequence of POTemplate.

There's also some drive-by lint cleanup. However I left some pre-existing ones in place because what "make lint" is complaining about looks to me an awful lot like our preferred syntax:

    list_comprehension = [
        frobnicate(item)
        for item in items
        if effable(item)
        ]

Jeroen

[Jeroen T. Vermeulen (jtv)]

Oops. I forgot to include the preceding branch as a requirement for this one, and the preceding one hasn't landed yet. (It's been reviewed and approved though).

Incremental diff compared to that branch: http://paste.ubuntu.com/464611/

[Данило Шеган (danilo)]

1. Split test_restrictCurrent() into two: one test should test one thing, and you are testing for both current=True and current=False.

2. I'd like to see more tests for "simple" conditions, i.e. negative tests: nothing returned, or that stuff in other contexts is not returned. For the second we probably don't care about it since it's tested in the base collection tests.

I mostly care about 1, and do 2 only if you agree.

review: Approve

[Jeroen T. Vermeulen (jtv)]

Thanks. I split up test_restrictCurrent into one test for current templates and another for obsolete ones, as discussed. I also added negative tests: condition False matches nothing, and searching for a productseries/distroseries/sourcepackagename excludes all other templates.

=== modified file 'lib/lp/services/database/collection.py'
--- lib/lp/services/database/collection.py	2010-07-15 15:00:52 +0000
+++ lib/lp/services/database/collection.py	2010-07-17 16:22:44 +0000
@@ -26,7 +26,7 @@
     individual columns or other Storm expressions.
     """
 
-    # Default table for this collection that will always be needed.
+    # Default table for this collection that will always be included.
     # Derived collection classes can use this to say what type they are
     # a collection of.
     starting_table = None
@@ -113,13 +113,27 @@
         return self.refine(tables=[LeftJoin(cls, *conditions)])
 
     def select(self, *values):
-        """Return a result set containing the requested `values`."""
+        """Return a result set containing the requested `values`.
+
+        If no values are requested, this selects the type of object that
+        the Collection is a collection of.
+        """
         if len(self.tables) == 0:
             source = self.store
         else:
             source = self.store.using(*self.tables)
 
-        if len(values) == 1:
+        if len(values) > 1:
+            # Selecting a tuple of values.  Pass it to Storm unchanged.
+            pass
+        elif len(values) == 1:
+            # One value requested.  Unpack for convenience.
             values = values[0]
+        else:
+            # Select the starting table by default.
+            assert self.starting_table is not None, (
+                "Collection %s does not define a starting table." %
+                    self.__class__.__name__)
+            values = self.starting_table
 
         return source.find(values, *self.conditions)

=== modified file 'lib/lp/services/database/tests/test_collection.py'
--- lib/lp/services/database/tests/test_collection.py	2010-07-16 10:05:21 +0000
+++ lib/lp/services/database/tests/test_collection.py	2010-07-17 16:22:44 +0000
@@ -5,8 +5,6 @@
 
 __metaclass__ = type
 
-import unittest
-
 from storm.locals import Int, Storm
 from zope.component import getUtility
 
@@ -185,4 +183,4 @@
         collection_with_store = collection.use(store)
         self.assertEqual(store, collection_with_store.store)
 
-        self.assertEqual([], collection_with_store.select())
+        self.assertEqual([], collection_with_store.select(TestTable))

=== modified file 'lib/lp/services/doc/collection.txt'
--- lib/lp/services/doc/collection.txt	2010-07-15 15:00:52 +0000
+++ lib/lp/services/doc/collection.txt	2010-07-17 16:22:44 +0000
@@ -16,7 +16,7 @@
     >>> from storm.locals import Count, Int, Storm
     >>> from canonical.launchpad.webapp.interfaces import (
     ...     IStoreSelector, MAIN_STORE, MASTER_FLAVOR)
- 
+
     >>> store = getUtility(IStoreSelector).get(MAIN_STORE, MASTER_FLAVOR)
     >>> ok = store.execute("CREATE TEMP TABLE Kumquat(id integer UNIQUE)")
     >>> class Kumquat(Storm):
@@ -30,7 +30,8 @@
     >>> obj = store.add(Kumquat(1))
     >>> obj = store.add(Kumquat(2))
 
-A custom KumquatCollection class derives from Kumquat.
+A custom KumquatCollection class derives from Collection.  The
+starting_table attribute tells Collection what it is a collection of.
 
     >>> from lp.services.database.collection import Collection
     >>> class KumquatCollection(Collection):
@@ -41,6 +42,11 @@
 set.
 
     >>> collection = KumquatCollection()
+    >>> print list(collection.select().order_by(Kumquat.id))
+    [Kumquat-1, Kumquat-2]
+
+Actually, select() is just shorthand for select(Kumquat).
+
     >>> print list(collection.select(Kumquat).order_by(Kumquat.id))
     [Kumquat-1, Kumquat-2]
 

=== modified file 'lib/lp/translations/model/potemplate.py'
--- lib/lp/translations/model/potemplate.py	2010-07-16 10:22:00 +0000
+++ lib/lp/translations/model/potemplate.py	2010-07-17 16:22:44 +0000
@@ -1,4 +1,4 @@
-# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# Copyright 2009-2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
 
 # pylint: disable-msg=E0611,W0212
@@ -92,16 +92,15 @@
 '''
 
 standardTemplateHeader = (
-"Project-Id-Version: %(origin)s\n"
-"Report-Msgid-Bugs-To: FULL NAME <EMAIL@ADDRESS>\n"
-"POT-Creation-Date: %(templatedate)s\n"
-"PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
-"Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
-"Language-Team: %(languagename)s <%(languagecode)[e-mail]>\n"
-"MIME-Version: 1.0\n"
-"Content-Type: text/plain; charset=UTF-8\n"
-"Content-Transfer-Encoding: 8bit\n"
-)
+    "Project-Id-Version: %(origin)s\n"
+    "Report-Msgid-Bugs-To: FULL NAME <EMAIL@ADDRESS>\n"
+    "POT-Creation-Date: %(templatedate)s\n"
+    "PO-Revision-Date: YEAR-MO-DA HO:MI+ZONE\n"
+    "Last-Translator: FULL NAME <EMAIL@ADDRESS>\n"
+    "Language-Team: %(languagename)s <%(languagecode)[e-mail]>\n"
+    "MIME-Version: 1.0\n"
+    "Content-Type: text/plain; charset=UTF-8\n"
+    "Content-Transfer-Encoding: 8bit\n")
 
 
 standardPOFileHeader = (standardTemplateHeader +
@@ -1380,7 +1379,6 @@
         self.sourcepackagename = sourcepackagename
         self.product = product
 
-
     def _get_potemplate_equivalence_class(self, template):
         """Return whatever we group `POTemplate`s by for sharing purposes."""
         if template.sourcepackagename is None:
@@ -1588,8 +1586,7 @@
 
     def getTranslationTemplates(self):
         """See `IHasTranslationTemplates`."""
-        return self._orderTemplates(
-            self.getTemplatesCollection().select(POTemplate))
+        return self._orderTemplates(self.getTemplatesCollection().select())
 
     def getTranslationTemplateFormats(self):
         """See `IHasTranslationTemplates`."""

=== added file 'lib/lp/translations/tests/test_translationtemplatescollection.py'
--- lib/lp/translations/tests/test_translationtemplatescollection.py	1970-01-01 00:00:00 +0000
+++ lib/lp/translations/tests/test_translationtemplatescollection.py	2010-07-17 16:22:44 +0000
@@ -0,0 +1,207 @@
+# Copyright 2010 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test `TranslationTemplatesCollection`."""
+
+__metaclass__ = type
+
+from zope.security.proxy import removeSecurityProxy
+from canonical.testing import DatabaseFunctionalLayer
+from lp.testing import TestCaseWithFactory
+
+from lp.translations.model.pofile import POFile
+from lp.translations.model.potemplate import (
+    POTemplate,
+    TranslationTemplatesCollection)
+
+
+class TestSomething(TestCaseWithFactory):
+    layer = DatabaseFunctionalLayer
+
+    def test_baseline(self):
+        # A collection constructed with no arguments selects all
+        # templates.
+        template = self.factory.makePOTemplate()
+
+        collection = TranslationTemplatesCollection()
+        self.assertIn(template, collection.select())
+
+    def test_none_found(self):
+        trunk = self.factory.makeProduct().getSeries('trunk')
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictProductSeries(trunk)
+
+        self.assertContentEqual([], by_series.select())
+
+    def test_restrictProductSeries(self):
+        trunk = self.factory.makeProduct().getSeries('trunk')
+        template = self.factory.makePOTemplate(productseries=trunk)
+
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictProductSeries(trunk)
+
+        self.assertContentEqual([template], by_series.select())
+
+    def test_restrictProductSeries_restricts(self):
+        # restrictProductSeries makes the collection ignore templates
+        # from other productseries and source packages.
+        product = self.factory.makeProduct()
+        trunk = product.getSeries('trunk')
+
+        nontrunk = removeSecurityProxy(product).newSeries(
+            product.owner, 'nontrunk', 'foo')
+        sourcepackage = self.factory.makeSourcePackage()
+
+        self.factory.makePOTemplate(productseries=nontrunk)
+        self.factory.makePOTemplate(
+            distroseries=sourcepackage.distroseries,
+            sourcepackagename=sourcepackage.sourcepackagename)
+
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictProductSeries(trunk)
+
+        self.assertContentEqual([], by_series.select())
+
+    def test_restrictDistroSeries(self):
+        package = self.factory.makeSourcePackage()
+        template = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictDistroSeries(package.distroseries)
+
+        self.assertContentEqual([template], by_series.select())
+
+    def test_restrictDistroSeries_restricts(self):
+        # restrictProductSeries makes the collection ignore templates
+        # from other productseries and distroseries.
+        distribution = self.factory.makeDistribution()
+        series = self.factory.makeDistroSeries(distribution=distribution)
+        other_series = self.factory.makeDistroSeries(
+            distribution=distribution)
+        productseries = self.factory.makeProductSeries()
+        package = self.factory.makeSourcePackageName()
+
+        self.factory.makePOTemplate(
+            distroseries=other_series, sourcepackagename=package)
+        self.factory.makePOTemplate(productseries=productseries)
+
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictDistroSeries(series)
+
+        self.assertContentEqual([], by_series.select())
+
+    def test_restrictSourcePackageName(self):
+        package = self.factory.makeSourcePackage()
+        template = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+
+        assert package.sourcepackagename
+        collection = TranslationTemplatesCollection()
+        by_packagename = collection.restrictSourcePackageName(
+            package.sourcepackagename)
+
+        self.assertContentEqual([template], by_packagename.select())
+
+    def test_restrictSourcePackageName_restricts(self):
+        # restrictSourcePackageName makes the collection ignore
+        # templates from other source package names and productseries.
+        package = self.factory.makeSourcePackage()
+        distroseries = package.distroseries
+        other_package = self.factory.makeSourcePackage(
+            distroseries=distroseries)
+        productseries = self.factory.makeProductSeries()
+
+        self.factory.makePOTemplate(
+            distroseries=distroseries,
+            sourcepackagename=other_package.sourcepackagename)
+        self.factory.makePOTemplate(productseries=productseries)
+
+        collection = TranslationTemplatesCollection()
+        by_packagename = collection.restrictSourcePackageName(
+            package.sourcepackagename)
+
+        self.assertContentEqual([], by_packagename.select())
+
+    def test_restrict_SourcePackage(self):
+        # You can restrict to a source package by restricting both to a
+        # DistroSeries and to a SourcePackageName.
+        package = self.factory.makeSourcePackage()
+        template = self.factory.makePOTemplate(
+            distroseries=package.distroseries,
+            sourcepackagename=package.sourcepackagename)
+
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictDistroSeries(package.distroseries)
+        by_package = by_series.restrictSourcePackageName(
+            package.sourcepackagename)
+
+        self.assertContentEqual([template], by_package.select())
+
+    def test_restrictCurrent_current(self):
+        trunk = self.factory.makeProduct().getSeries('trunk')
+        template = self.factory.makePOTemplate(productseries=trunk)
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictProductSeries(trunk)
+
+        current_templates = by_series.restrictCurrent(True)
+
+        removeSecurityProxy(template).iscurrent = True
+        self.assertContentEqual(
+            [template], current_templates.select())
+
+        removeSecurityProxy(template).iscurrent = False
+        self.assertContentEqual([], current_templates.select())
+
+    def test_restrictCurrent_obsolete(self):
+        trunk = self.factory.makeProduct().getSeries('trunk')
+        template = self.factory.makePOTemplate(productseries=trunk)
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictProductSeries(trunk)
+
+        obsolete_templates = by_series.restrictCurrent(False)
+
+        removeSecurityProxy(template).iscurrent = True
+        self.assertContentEqual([], obsolete_templates.select())
+
+        removeSecurityProxy(template).iscurrent = False
+        self.assertContentEqual(
+            [template], obsolete_templates.select())
+
+    def test_joinPOFile(self):
+        trunk = self.factory.makeProduct().getSeries('trunk')
+        translated_template = self.factory.makePOTemplate(productseries=trunk)
+        untranslated_template = self.factory.makePOTemplate(
+            productseries=trunk)
+        nl = translated_template.newPOFile('nl')
+        de = translated_template.newPOFile('de')
+
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictProductSeries(trunk)
+        joined = by_series.joinPOFile()
+
+        self.assertContentEqual(
+            [(translated_template, nl), (translated_template, de)],
+            joined.select(POTemplate, POFile))
+
+    def test_joinOuterPOFile(self):
+        trunk = self.factory.makeProduct().getSeries('trunk')
+        translated_template = self.factory.makePOTemplate(productseries=trunk)
+        untranslated_template = self.factory.makePOTemplate(
+            productseries=trunk)
+        nl = translated_template.newPOFile('nl')
+        de = translated_template.newPOFile('de')
+
+        collection = TranslationTemplatesCollection()
+        by_series = collection.restrictProductSeries(trunk)
+        joined = by_series.joinOuterPOFile()
+
+        expected_outcome = [
+            (translated_template, nl),
+            (translated_template, de),
+            (untranslated_template, None),
+            ]
+        self.assertContentEqual(
+            expected_outcome, joined.select(POTemplate, POFile))