Merge lp:~james-w/udd/fix-auto-retry into lp:udd

[James Westby]

Fix auto retry to only retry things that are marked to be auto retried.

[lp:~james-w/udd/fix-auto-retry]

605. By James Westby on 2012-07-04

Change the variable name to make the purpose clearer.

[Vincent Ladeuil (vila)]

Shame I didn't write those tests in the first place :-/

review: Approve

[Jonathan Lange (jml)]

On 4 July 2012 15:37, James Westby <email address hidden> wrote:
> James Westby has proposed merging lp:~james-w/udd/fix-auto-retry into lp:udd.
>
> Requested reviews:
> Ubuntu Distributed Development Developers (udd)
>
> For more details, see:
> https://code.launchpad.net/~james-w/udd/fix-auto-retry/+merge/113409
>
>
> --
> https://code.launchpad.net/~james-w/udd/fix-auto-retry/+merge/113409
> Your team Ubuntu Distributed Development Developers is requested to review the proposed merge of lp:~james-w/udd/fix-auto-retry into lp:udd.
>
> === modified file 'udd/icommon.py'
> --- udd/icommon.py 2012-07-04 11:27:18 +0000
> +++ udd/icommon.py 2012-07-04 14:36:22 +0000
> @@ -768,7 +768,7 @@
> return sig
>
> def _known_auto_retry(self, c, sig):
> - return c.execute(self.RETRY_TABLE_SELECT, (sig,)).get_one()
> + return c.execute(self.RETRY_TABLE_SELECT, (sig,)).get_one() is not None
>

Since this caught us out once, it's probably a good idea to add a
docstring to this method explaining its return type so we aren't
caught out again.

> === modified file 'udd/tests/test_status_database.py'
> --- udd/tests/test_status_database.py 2012-07-02 19:43:28 +0000
> +++ udd/tests/test_status_database.py 2012-07-04 14:36:22 +0000
> @@ -112,7 +112,7 @@
> class TestRetry(TestStatusDb):
>
> def test_known_auto_retry_empty(self):
> - self.assertEquals(False, self.db.known_auto_retry('whatever'))
> + self.assertEquals(False, self.db.known_auto_retry(u'whatever'))
>
> def test_known_auto_retry_known(self):
> sig = u'whatever'
> @@ -123,12 +123,47 @@
> self.db.add_known_auto_retry(u'something')
> self.assertEquals(False, self.db.known_auto_retry(u'something else'))
>
> + def test_attempt_retry_no_auto_retry(self):
> + info = icommon.PackageStatus(u'foo')
> + info.signature = u'aaaa'
...

Good tests, thanks!

If the signature u'aaaa' is special at all, then it should become a
constant or something. If udd already depends on testtools or similar
then it should probably use a factory method.

Thanks,
jml

[James Westby (james-w)]

On Thu, 05 Jul 2012 09:04:24 -0000, Jonathan Lange <email address hidden> wrote:
> Since this caught us out once, it's probably a good idea to add a
> docstring to this method explaining its return type so we aren't
> caught out again.

I've added a reference to the public version of the method which is
documented.

> Good tests, thanks!
>
> If the signature u'aaaa' is special at all, then it should become a
> constant or something. If udd already depends on testtools or similar
> then it should probably use a factory method.

It's not special, and the code doesn't currently depend on testtools.

Thanks,

James

[Vincent Ladeuil (vila)]

> It's not special,

/me nods

> and the code doesn't currently depend on testtools.

Well, the tests use bzrlib.tests which use testtools, so you can use testtools features if/when needed.

[James Westby (james-w)]

On Tue, 10 Jul 2012 12:22:24 -0000, Vincent Ladeuil <email address hidden> wrote:
> Well, the tests use bzrlib.tests which use testtools, so you can use testtools features if/when needed.

Ah, good point.

Thanks,

James

=== modified file 'udd/icommon.py'
--- udd/icommon.py	2012-07-04 11:27:18 +0000
+++ udd/icommon.py	2012-07-04 14:41:20 +0000
@@ -768,7 +768,7 @@
         return sig
 
     def _known_auto_retry(self, c, sig):
-        return c.execute(self.RETRY_TABLE_SELECT, (sig,)).get_one()
+        return c.execute(self.RETRY_TABLE_SELECT, (sig,)).get_one() is not None
 
     def known_auto_retry(self, sig):
         """Is the signature marked for auto-retry?
@@ -779,11 +779,11 @@
         :rtype: boolean
         """
         with Cursor(self.conn) as c:
-            return self._known_auto_retry(c, sig) is not None
+            return self._known_auto_retry(c, sig)
 
     def _add_known_auto_retry(self, c, sig):
-        row = self._known_auto_retry(c, sig)
-        if not row:
+        already_added = self._known_auto_retry(c, sig)
+        if not already_added:
             c.execute(self.RETRY_TABLE_INSERT, (sig,))
 
     def add_known_auto_retry(self, sig):
@@ -870,8 +870,8 @@
         self._add_job(c, package, job_type)
 
     def _attempt_retry(self, c, info):
-        row = self._known_auto_retry(c, info.signature)
-        if row:
+        auto_retry = self._known_auto_retry(c, info.signature)
+        if not auto_retry:
             # A failure that is not auto-retried
             return False
         info.auto_retry = True
@@ -888,6 +888,7 @@
                 # Already retried MAX_AUTO_RETRY_COUNT times, stop retrying
                 return False
         self._retry(c, info.name, info.signature, info.timestamp)
+        return True
 
     def summarise_failures(self):
         package_info = []

=== modified file 'udd/tests/test_status_database.py'
--- udd/tests/test_status_database.py	2012-07-02 19:43:28 +0000
+++ udd/tests/test_status_database.py	2012-07-04 14:41:20 +0000
@@ -112,7 +112,7 @@
 class TestRetry(TestStatusDb):
 
     def test_known_auto_retry_empty(self):
-        self.assertEquals(False, self.db.known_auto_retry('whatever'))
+        self.assertEquals(False, self.db.known_auto_retry(u'whatever'))
 
     def test_known_auto_retry_known(self):
         sig = u'whatever'
@@ -123,12 +123,47 @@
         self.db.add_known_auto_retry(u'something')
         self.assertEquals(False, self.db.known_auto_retry(u'something else'))
 
+    def test_attempt_retry_no_auto_retry(self):
+        info = icommon.PackageStatus(u'foo')
+        info.signature = u'aaaa'
+        with icommon.Cursor(self.db.conn) as c:
+            self.assertEquals(False, self.db._attempt_retry(c, info))
+
+    def test_attempt_retry_too_recent(self):
+        info = icommon.PackageStatus(u'foo')
+        info.signature = u'aaaa'
+        info.timestamp = datetime.datetime.utcnow()
+        self.db.add_known_auto_retry(info.signature)
+        with icommon.Cursor(self.db.conn) as c:
+            self.assertEquals(False, self.db._attempt_retry(c, info))
+
+    def test_attempt_retry_too_many_times(self):
+        package_name = u'foo'
+        info = icommon.PackageStatus(package_name)
+        info.signature = u'aaaa'
+        info.timestamp = datetime.datetime.utcnow()
+        self.db.add_known_auto_retry(info.signature)
+        with icommon.Cursor(self.db.conn) as c:
+            c.execute('insert into %s values (?, ?, ?, ?, ?)'
+                    % self.db.OLD_FAILURES_TABLE,
+                    (package_name, info.signature, info.timestamp,
+                        info.timestamp, self.db.MAX_AUTO_RETRY_COUNT))
+            self.assertEquals(False, self.db._attempt_retry(c, info))
+
+    def test_attempt_retry_with_auto_retry(self):
+        info = icommon.PackageStatus(u'foo')
+        info.signature = u'aaaa'
+        info.timestamp = (datetime.datetime.utcnow()
+                - datetime.timedelta(seconds=self.db.AUTO_RETRY_SECONDS+1))
+        self.db.add_known_auto_retry(info.signature)
+        with icommon.Cursor(self.db.conn) as c:
+            self.assertEquals(True, self.db._attempt_retry(c, info))
+
 # FIXME: Need more tests for the following but they are too hard to test for
 # now -- vila 20111020
 
 # retry(self, package, force=False, priority=False, auto=False,
 # _retry(self, c, package, signature, timestamp, priority=False):
-# _attempt_retry(self, c, info):
 
 
 class TestSignatures(TestStatusDb):