Merge lp:~ivo-kracht/launchpad/bug-728129 into lp:launchpad

[Ivo Kracht]

I added the pre_iter_hook to the methods (first(), last(), any(), one()) as well as adding a new test file to make the original test work properly. Finally I adjusted the original test to fit the new conditions and added a test for the case that a method returns None.

Pre-imp call with adeuring

test:
./bin/test services -vvt decoratedresultset

= Launchpad lint =

Checking for conflicts and issues in changed files.

Linting changed files:
  lib/lp/services/database/decoratedresultset.py
  lib/lp/services/database/tests/decoratedresultset.txt
  lib/lp/services/database/tests/test_decoratedresultset.py

./lib/lp/services/database/tests/decoratedresultset.txt
       1: narrative uses a moin header.
      36: narrative uses a moin header.
      60: narrative uses a moin header.
      78: narrative uses a moin header.
      86: narrative uses a moin header.
      96: narrative uses a moin header.
     113: narrative uses a moin header.
     122: narrative uses a moin header.
     137: narrative uses a moin header.
     149: narrative uses a moin header.
     156: narrative uses a moin header.
     173: narrative uses a moin header.

Since it is nearly end of the work day i didn't have time to fix these errors

[j.c.sackett (jcsackett)]

Thanks, Ivo. This looks good.

It looks like you've copy pasted test_decoratedresultset from another file; the headings are all wrong (it's talking about CVEs, buglinktarget, &c).

That said, I'm not sure that file is even needed--I gather from reading it it's meant to gather the doctest you modified, but that test is already gathered and run without it. Can you try deleting it and making sure your tests are run?

review: Needs Information

[Abel Deuring (adeuring)]

On 05.07.2012 19:41, j.c.sackett wrote:
> Review: Needs Information
>
> Thanks, Ivo. This looks good.
>
> It looks like you've copy pasted test_decoratedresultset from another file; the headings are all wrong (it's talking about CVEs, buglinktarget, &c).
>
> That said, I'm not sure that file is even needed--I gather from reading it it's meant to gather the doctest you modified, but that test is already gathered and run without it. Can you try deleting it and making sure your tests are run?
>

[playing the "proxy" for Ivo:]

right, the file was copied, and the comments need to be changed. But the
file itself is necessary: Without it, the doctest is simply not
executed. (Or do we have other ways to let the doctest really run?)

Ivo is currently "recovering" from his internship and back in college
tomorrow, so I'll "seize" his branch and fix the comments.

Abel

[j.c.sackett (jcsackett)]

I'll mark this as approved as the other resubmitted branch is now Approved as well. We'll have to manually mark this as Merged, I suspect, when the other lands.

review: Approve

=== modified file 'lib/lp/services/database/decoratedresultset.py'
--- lib/lp/services/database/decoratedresultset.py	2012-03-27 00:15:07 +0000
+++ lib/lp/services/database/decoratedresultset.py	2012-07-05 17:15:27 +0000
@@ -167,12 +167,17 @@
         else:
             return self.decorate_or_none(value)
 
+    def iterhook_one_elem(self, value):
+        if value is not None and self.pre_iter_hook is not None:
+            self.pre_iter_hook([value])
+
     def any(self, *args, **kwargs):
         """See `IResultSet`.
 
         :return: The decorated version of the returned value.
         """
         value = self.result_set.any(*args, **kwargs)
+        self.iterhook_one_elem(value)
         return self.decorate_or_none(value)
 
     def first(self, *args, **kwargs):
@@ -181,6 +186,7 @@
         :return: The decorated version of the returned value.
         """
         value = self.result_set.first(*args, **kwargs)
+        self.iterhook_one_elem(value)
         return self.decorate_or_none(value)
 
     def last(self, *args, **kwargs):
@@ -189,6 +195,7 @@
         :return: The decorated version of the returned value.
         """
         value = self.result_set.last(*args, **kwargs)
+        self.iterhook_one_elem(value)
         return self.decorate_or_none(value)
 
     def one(self, *args, **kwargs):
@@ -197,6 +204,7 @@
         :return: The decorated version of the returned value.
         """
         value = self.result_set.one(*args, **kwargs)
+        self.iterhook_one_elem(value)
         return self.decorate_or_none(value)
 
     def order_by(self, *args, **kwargs):

=== modified file 'lib/lp/services/database/tests/decoratedresultset.txt'
--- lib/lp/services/database/tests/decoratedresultset.txt	2011-12-20 08:55:34 +0000
+++ lib/lp/services/database/tests/decoratedresultset.txt	2012-07-05 17:15:27 +0000
@@ -25,10 +25,13 @@
     >>> def result_decorator(distribution):
     ...     return "Dist name is: %s" % distribution.name
 
+    >>> def pre_iter_hook(values):
+    ...     print len(values), "elements in result set"
+
     >>> from lp.services.database.decoratedresultset import (
     ...     DecoratedResultSet)
     >>> decorated_result_set = DecoratedResultSet(
-    ...     proxied_result_set, result_decorator)
+    ...     proxied_result_set, result_decorator, pre_iter_hook)
 
 == copy() ==
 
@@ -49,6 +52,7 @@
 
     >>> for distro in result_copy:
     ...     distro
+    7 elements in result set
     u'Dist name is: debian'
     ...
     u'Dist name is: ubuntutest'
@@ -86,6 +90,7 @@
 
     >>> orig_result = decorated_result_set.result_set.any()
     >>> decorated_result_set.any() == result_decorator(orig_result)
+    1 elements in result set
     True
 
 == first() ==
@@ -94,14 +99,24 @@
 method and decorates the result:
 
     >>> decorated_result_set.first()
+    1 elements in result set
     u'Dist name is: debian'
 
+pre_iter_hook is not called from methods like first() or one() which return
+at most one row:
+
+    >>> empty_result_set = decorated_result_set.copy()
+    >>> print empty_result_set.config(
+    ...     offset=empty_result_set.count()).first()
+    None
+
 == last() ==
 
 The decorated last() method calls the original result set's last()
 method and decorates the result:
 
     >>> decorated_result_set.last()
+    1 elements in result set
     u'Dist name is: ubuntutest'
 
 == order_by() ==
@@ -114,6 +129,7 @@
     ...     Desc(Distribution.name))
     >>> for dist in ordered_results:
     ...     dist
+    7 elements in result set
     u'Dist name is: ubuntutest'
     ...
     u'Dist name is: debian'
@@ -124,6 +140,7 @@
 method and decorates the result:
 
     >>> decorated_result_set.config(offset=2, limit=1).one()
+    1 elements in result set
     u'Dist name is: redhat'
 
     >>> result_decorator(decorated_result_set.result_set.one())

=== added file 'lib/lp/services/database/tests/test_decoratedresultset.py'
--- lib/lp/services/database/tests/test_decoratedresultset.py	1970-01-01 00:00:00 +0000
+++ lib/lp/services/database/tests/test_decoratedresultset.py	2012-07-05 17:15:27 +0000
@@ -0,0 +1,36 @@
+# Copyright 2009 Canonical Ltd.  This software is licensed under the
+# GNU Affero General Public License version 3 (see the file LICENSE).
+
+"""Test harness for running the buglinktarget.txt interface test
+
+This module will run the interface test against the CVE, Specification and
+Question implementations of that interface.
+"""
+
+__metaclass__ = type
+
+__all__ = []
+
+import unittest
+
+from lp.testing.layers import DatabaseFunctionalLayer
+from lp.testing.systemdocs import (
+    LayeredDocFileSuite,
+    setUp,
+    tearDown,
+    )
+
+
+def test_suite():
+    suite = unittest.TestSuite()
+
+    test = LayeredDocFileSuite(
+        'decoratedresultset.txt',
+        setUp=setUp, tearDown=tearDown,
+        layer=DatabaseFunctionalLayer)
+    suite.addTest(test)
+    return suite
+
+
+if __name__ == '__main__':
+    unittest.main()