Merge lp:~matsubara/lp-devops-dashboard/929538-incident-reports-record into lp:lp-devops-dashboard

[Diogo Matsubara]

fix bug #929538: Wrong calculation of number of days without incident reports

This branch fixes how the day since the last incident report is calculated. The current code does a delta between incident records to find out how many days we had without incidents. The fix take into account the delta between today and the day of the last incident.

Also added tests for the incidentreports.py module and updated some dependencies with the dependencies available in the download-cache.

[lp:~matsubara/lp-devops-dashboard/929538-incident-reports-record]

50. By Diogo Matsubara on 2012-02-09

oops, forgot to add the test file and sample data

[Ian Booth (wallyworld)]

Hi there

I have perhaps a dumb question - if we are intending to show the days since the last incident report, is the comparison used in the code the wrong way around?

89 + if current and ((today() - current) > overall_delta):
90 + overall_delta = today() - current

The above code will end up finding the number of days since the oldest incident report I think? Where as we want the number of days since the latest incident report, so it should be:

if current and ((today() - current) < overall_delta):

The attribute used to hold the delta value, "most_days_with_no_incidents" seems incorrectly named now that we are changing the semantics of the calculation. It should now really be called "days_since_last_incident".

If what I say above is correct, then the tests are incorrect, eg:

183 + def test_get_last_reports_since(self):
184 + incidentreports._today = datetime(2011, 11, 9)
185 + self.ir.get_last_reports_since()
186 + self.assertEqual(self.ir.total_incidents, 7)
187 + self.assertEqual(self.ir.most_days_with_no_incidents, 89)
188 + self.assertEqual(self.ir.last_report, '2011-11-07-LP-some-error')

The latest incident report in the sample data is 2011-11-07 and "today" is 2011-11-09, so I would expect the days since last incident report to be 2.

review: Needs Information (code)

[Diogo Matsubara (matsubara)]

I replied by email but it didn't show up here.

On Thu, Feb 9, 2012 at 11:16 PM, Ian Booth <email address hidden> wrote:
> Review: Needs Information code
>
> Hi there

Hi Ian, thanks for the review!

>
> I have perhaps a dumb question - if we are intending to show the days since the last incident report, is the comparison used in the code the wrong way around?
>

We don't want to show the days since the last incident report, we want
to show how many days we managed to stay with no incidents. Kinda like
those signs on heavy traffic roads where they say: XX days without any
fatal accidents/crashes.

The days since the last incident report is already in the page.

> 89 + if current and ((today() - current) > overall_delta):
> 90 + overall_delta = today() - current
>
> The above code will end up finding the number of days since the oldest incident report I think? Where as we want the number of days since the latest incident report, so it should be:
>
> if current and ((today() - current) < overall_delta):
>
> The attribute used to hold the delta value, "most_days_with_no_incidents" seems incorrectly named now that we are changing the semantics of the calculation. It should now really be called "days_since_last_incident".
>

Given the reason above, I think the attribute is correctly named.

> If what I say above is correct, then the tests are incorrect, eg:
>
> 183 + def test_get_last_reports_since(self):
> 184 + incidentreports._today = datetime(2011, 11, 9)
> 185 + self.ir.get_last_reports_since()
> 186 + self.assertEqual(self.ir.total_incidents, 7)
> 187 + self.assertEqual(self.ir.most_days_with_no_incidents, 89)
> 188 + self.assertEqual(self.ir.last_report, '2011-11-07-LP-some-error')
>
> The latest incident report in the sample data is 2011-11-07 and "today" is 2011-11-09, so I would expect the days since last incident report to be 2.
>
> --
> https://code.launchpad.net/~matsubara/lp-devops-dashboard/929538-incident-reports-record/+merge/92359
> You are the owner of lp:~matsubara/lp-devops-dashboard/929538-incident-reports-record.

Is this ok to land?

Cheers,

Diogo

[Ian Booth (wallyworld)]

Hi

I understand now, thanks for the explanation. I was initially unclear what was being calculated.

>
> We don't want to show the days since the last incident report, we want
> to show how many days we managed to stay with no incidents. Kinda like
> those signs on heavy traffic roads where they say: XX days without any
> fatal accidents/crashes.
>
> The days since the last incident report is already in the page.
>

The XX days example above - I now understand that we want the greatest XX value over the time period of the report, including the time between today and the last incident.

review: Approve (code)

=== modified file 'buildout.cfg'
--- buildout.cfg	2011-04-22 20:07:49 +0000
+++ buildout.cfg	2012-02-09 20:17:56 +0000
@@ -19,8 +19,6 @@
 
 extends = versions.cfg
 
-allow-picked-versions = false
-
 prefer-final = true
 
 versions = versions

=== modified file 'src/devops/incidentreports.py'
--- src/devops/incidentreports.py	2011-04-21 23:02:24 +0000
+++ src/devops/incidentreports.py	2012-02-09 20:17:56 +0000
@@ -3,10 +3,22 @@
 from BeautifulSoup import BeautifulSoup
 from devops.utils import days_ago, last_date_delta
 
-### Find last LP incident report
+
+_today = None
+
+def today():
+    """Return a date time object from the cache.
+
+    This function makes it easy for test to use a cached date rather than the
+    real date.
+    """
+    if _today is not None:
+        return _today
+    return datetime.today()
 
 
 class Wiki:
+
     def __init__(self, url):
         self.url = url
         self.contents = []
@@ -20,7 +32,11 @@
             self.read_contents(moinfile)
             return 1
 
-        if not editshortcut(self.url, action="print", editfile_func=editfunc,
+        # If it's a local file, just read it rather than trying to open it
+        # with editmoin.
+        if self.url.startswith('/'):
+            self.data = open(self.url).read()
+        elif not editshortcut(self.url, action="print", editfile_func=editfunc,
             change_page=False):
             # Someone else was editing the page. Raise an exception
             raise Exception("Couldn't get write lock for the wiki page. "
@@ -30,19 +46,11 @@
         self.data = moinfile._unescape(moinfile.data)
 
 
-class Reports:
+class IncidentReports:
 
-    def __init__(sel, source_url):
+    def __init__(self, since=datetime(2011, 01, 01),
+                 source_url="https://wiki.canonical.com/IncidentReports"):
         self.source_url = source_url
-
-    def fetch_data(self):
-        raise NotImplementedError
-
-
-class IncidentReports:
-
-    def __init__(self, since=datetime(2011, 01, 01)):
-        self.source_url = "https://wiki.canonical.com/IncidentReports"
         self.all_reports = []
         # { Date : report}
         self.lp_reports = {}
@@ -111,6 +119,7 @@
 
         overall_delta = timedelta(0)
 
+        current = None
         for i, report in enumerate(sorted(last_reports)):
             current = report
             if i == 0:
@@ -121,6 +130,9 @@
                 overall_delta = current_delta
             previous = current
 
+        if current and ((today() - current) > overall_delta):
+            overall_delta = today() - current
+
         self.most_days_with_no_incidents = overall_delta.days
         self.total_incidents = len(last_reports)
         self.last_report = self.lp_reports[current][0]

=== added file 'src/devops/tests/sample_data/incident-reports.html'
--- src/devops/tests/sample_data/incident-reports.html	1970-01-01 00:00:00 +0000
+++ src/devops/tests/sample_data/incident-reports.html	2012-02-09 20:17:56 +0000
@@ -0,0 +1,42 @@
+<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
+<html>
+<head>
+<title>IncidentReports - Canonical Wiki</title>
+</head>
+
+<body>
+  <ol>
+    <li>
+      <a href="/IncidentReports/2011-01-16-LP-Rollout"><strong>IncidentReports/</strong>2011-01-16-LP-Rollout</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-01-24-U1-Borkness"><strong>IncidentReports/</strong>2011-01-24-U1-Borkness</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-02-13-LP-Foobar"><strong>IncidentReports/</strong>2011-02-13-LP-Foobar</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-02-25-ISD-Snafu"><strong>IncidentReports/</strong>2011-02-25-ISD-Snafu</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-03-03-Linaro-Doom"><strong>IncidentReports/</strong>2011-03-03-Linaro-Doom</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-03-22-LP-Everything-on-fire"><strong>IncidentReports/</strong>2011-03-22-LP-Everything-on-fire</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-04-21-LP-Chaos"><strong>IncidentReports/</strong>2011-04-21-LP-Chaos</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-06-18-LP-Armaggedon"><strong>IncidentReports/</strong>2011-06-18-LP-Armaggedon</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-08-10-LP-Apocalypse"><strong>IncidentReports/</strong>2011-08-10-LP-Apocalypse</a>
+    </li>
+    <li>
+      <a href="/IncidentReports/2011-11-07-LP-some-error"><strong>IncidentReports/</strong>2011-11-07-LP-some-error</a>
+    </li>
+  </ol>
+</body>
+</html>
+

=== modified file 'src/devops/tests/test_fetchbugs.py'
--- src/devops/tests/test_fetchbugs.py	2011-06-08 02:23:04 +0000
+++ src/devops/tests/test_fetchbugs.py	2012-02-09 20:17:56 +0000
@@ -50,14 +50,3 @@
 
         self.assertEqual(len(self.lp_bugs.fetch_critical_bugs()), 4)
         self.assertEqual(len(self.lp_bugs.fetch_open_critical_bugs()), 3)
-
-
-
-
-
-
-
-
-
-
-

=== added file 'src/devops/tests/test_incidentreports.py'
--- src/devops/tests/test_incidentreports.py	1970-01-01 00:00:00 +0000
+++ src/devops/tests/test_incidentreports.py	2012-02-09 20:17:56 +0000
@@ -0,0 +1,31 @@
+from datetime import datetime
+from os.path import dirname, join
+from unittest import TestCase
+from urlparse import urljoin
+
+from devops import incidentreports
+from devops.incidentreports import IncidentReports
+
+
+class TestIncidentReports(TestCase):
+
+
+    def setUp(self):
+        source_url = join(
+            dirname(__file__), "sample_data", "incident-reports.html")
+        self.ir = IncidentReports(source_url=source_url)
+
+    def test_get_last_reports_since(self):
+        incidentreports._today = datetime(2011, 11, 9)
+        self.ir.get_last_reports_since()
+        self.assertEqual(self.ir.total_incidents, 7)
+        self.assertEqual(self.ir.most_days_with_no_incidents, 89)
+        self.assertEqual(self.ir.last_report, '2011-11-07-LP-some-error')
+
+    def test_no_incidents_for_a_long_time(self):
+        # Test when the last incident report is older than the delta between
+        # incident reports. Bug 929538
+        incidentreports._today = datetime(2012, 2, 9)
+        self.ir.get_last_reports_since()
+        self.assertEqual(self.ir.most_days_with_no_incidents, 94)
+        self.assertEqual(self.ir.last_report, '2011-11-07-LP-some-error')

=== modified file 'versions.cfg'
--- versions.cfg	2011-04-22 20:11:46 +0000
+++ versions.cfg	2012-02-09 20:17:56 +0000
@@ -11,13 +11,13 @@
 setuptools = 0.6c11
 testtools = 0.9.4
 z3c.recipe.scripts = 1.0.1
-z3c.recipe.tag = 0.2.0
+z3c.recipe.tag = 0.4.0
 zc.buildout = 1.5.1
 zc.recipe.egg = 1.3.2
 zc.recipe.testrunner = 1.4.0
 zc.sourcerelease = 0.3.1
 zope.exceptions = 3.5.2
-zope.interface = 3.5.1
+zope.interface = 3.8.0
 zope.testing = 3.10.2
 zope.testrunner = 4.0.3
 
@@ -25,7 +25,7 @@
 BeautifulSoup = 3.2.0
 
 # bzr
-bzr = 2.1.0
+bzr = 2.5.0dev5-r6402
 
 # genshi
 genshi = 0.6
@@ -33,13 +33,13 @@
 # launchpadlib
 elementtree = 1.2.6-20050316
 httplib2 = 0.6.0
-launchpadlib = 1.6.4
-lazr.authentication = 0.1.2
-lazr.restfulclient = 0.11.1
+launchpadlib = 1.9.12
+lazr.authentication = 0.1.1
+lazr.restfulclient = 0.12.0
 lazr.uri = 1.0.2
 oauth = 1.0.1
-simplejson = 2.1.1
-wadllib = 1.1.8
+simplejson = 2.2.1
+wadllib = 1.2.0
 wsgi-intercept = 0.4
 
 # mocker