Merge lp:~bigkevmcd/offspring/add-metrics into lp:offspring

[Kevin McDermott]

This adds the metrics per the feature request.

[David Murphy (schwuk)]

The (calculated) number of days is consistently one out: https://pastebin.canonical.com/92199/ and https://pastebin.canonical.com/92201/

Trying to specify the same start and end month of February results in an error: https://pastebin.canonical.com/92200/. I know the bug states "the end date is greater than the start date" but what was actually meant by that is that --start=2013-02 --end=2013-01 would be invalid, however due to date expansion rules laid out in the bug, --start=2013-02 --end=2013-02 would be expanded to --start=2013-02-01 00:00:00 --end=2013-02-28 23:59:59 and therefore be valid.

review: Needs Fixing

[David Murphy (schwuk)]

...and the validation for end date being greater than start date doesn't appear to be working: https://pastebin.canonical.com/92208/

review: Needs Fixing

[Kevin McDermott (bigkevmcd)]

> The (calculated) number of days is consistently one out:
> https://pastebin.canonical.com/92199/ and
> https://pastebin.canonical.com/92201/

As explained, this isn't quite right...the bug explicitly asks for less than a day...

> Trying to specify the same start and end month of February results in an
> error: https://pastebin.canonical.com/92200/. I know the bug states "the end
> date is greater than the start date" but what was actually meant by that is
> that --start=2013-02 --end=2013-01 would be invalid, however due to date
> expansion rules laid out in the bug, --start=2013-02 --end=2013-02 would be
> expanded to --start=2013-02-01 00:00:00 --end=2013-02-28 23:59:59 and
> therefore be valid.

So, should we fix this or not?

[David Murphy (schwuk)]

On the other hand, I like that the date parsing is not strict (despite the request for handling invalid formats in the bug): https://pastebin.canonical.com/92209/

[Kevin McDermott (bigkevmcd)]

> On the other hand, I like that the date parsing is not strict (despite the
> request for handling invalid formats in the bug):
> https://pastebin.canonical.com/92209/

Ummm...you do know that , is valid in an ISO 8601 date? as is / :-)

[lp:~bigkevmcd/offspring/add-metrics]

165. By Kevin McDermott on 2013-06-05

Fix the start less than end bug.

166. By Kevin McDermott on 2013-06-06

Implement calculate_days.

167. By Kevin McDermott on 2013-06-06

Change the method of calcuating the date-diff to round up.

168. By Kevin McDermott on 2013-06-06

Failing tests for pseudo_iso8601.

169. By Kevin McDermott on 2013-06-06

Implement parsing with the pseudo iso8601 parser.

170. By Kevin McDermott on 2013-06-06

Rename year variable.

[David Murphy (schwuk)]

Looks good. Thanks.

review: Approve

[David Murphy (schwuk)]

Damn - wrong MP.

Unmerged revisions

=== modified file 'lib/offspring/web/queuemanager/management/commands/build_metrics.py'
--- lib/offspring/web/queuemanager/management/commands/build_metrics.py	2011-12-14 07:58:45 +0000
+++ lib/offspring/web/queuemanager/management/commands/build_metrics.py	2013-06-06 09:47:40 +0000
@@ -1,15 +1,37 @@
-# Copyright 2010-2011 Canonical Ltd.
-
-from django.core.management.base import NoArgsCommand
-
-from offspring.web.queuemanager.metrics import get_build_result_metrics
-
-
-class Command(NoArgsCommand):
+# Copyright 2010-2013 Canonical Ltd.
+from optparse import make_option
+
+from django.core.management.base import BaseCommand, CommandError
+from django.db import transaction
+
+from offspring.web.queuemanager.metrics import (
+    get_build_result_metrics, calculate_start_and_end)
+
+
+class Command(BaseCommand):
     help = "Outputs some basic metrics."
 
-    def handle_noargs(self, **options):
-        metrics = get_build_result_metrics()
+    option_list = BaseCommand.option_list + (
+        make_option(
+            "--end", dest="end",
+            help="End date e.g. 2013-06-05"),
+        make_option(
+            "--start", dest="start",
+            help="Start date e.g. 2013-06-20"),
+        )
+
+    def handle(self, *args, **options):
+        if options.get("start") and options.get("end"):
+            start, end = calculate_start_and_end(
+                options.get("start"), options.get("end"))
+            if not end > start:
+                raise CommandError("start date must be before end date")
+            metrics = get_build_result_metrics(start, end)
+        elif (options.get("start") and not options.get("end")) or (
+            options.get("end") and not options.get("start")):
+            raise CommandError("--start and --end must be supplied")
+        else:
+            metrics = get_build_result_metrics()
 
         for item, value in metrics:
             self.stdout.write("%s = %s\n" % (item, value))

=== modified file 'lib/offspring/web/queuemanager/management/commands/tests/test_build_metrics.py'
--- lib/offspring/web/queuemanager/management/commands/tests/test_build_metrics.py	2011-12-15 08:50:53 +0000
+++ lib/offspring/web/queuemanager/management/commands/tests/test_build_metrics.py	2013-06-06 09:47:40 +0000
@@ -1,7 +1,9 @@
 # Copyright 2010-2011 Canonical Ltd.
 from cStringIO import StringIO
+from datetime import datetime
 
 from mocker import MockerTestCase
+from django.core.management.base import CommandError
 
 from offspring.web.queuemanager.metrics import get_build_result_metrics
 from offspring.web.queuemanager.management.commands.build_metrics import (
@@ -23,7 +25,72 @@
 
         command = Command()
         stdout = StringIO()
-        command.stdout = stdout
-        command.handle_noargs()
-
-        self.assertEqual("Description of value = 200\n", stdout.getvalue())
+        command.execute(stdout=stdout)
+
+        self.assertEqual("Description of value = 200\n", stdout.getvalue())
+
+    def test_calls_get_build_result_metrics_with_start_and_end(self):
+        """
+        If --start and --end are provided, we should use them to calculate the
+        period of the metrics.
+        """
+        build_result_metrics_mock = self.mocker.replace(
+            get_build_result_metrics)
+        build_result_metrics_mock(
+            datetime(2013, 06, 05, 0, 0, 0),
+            datetime(2013, 06, 05, 23, 59, 59))
+
+        self.mocker.result([("Description of value", 200)])
+        self.mocker.replay()
+
+        command = Command()
+        stdout = StringIO()
+        command.execute(start="2013-06-05", end="2013-06-05", stdout=stdout)
+        self.assertEqual("Description of value = 200\n", stdout.getvalue())
+
+    def test_error_with_missing_end_parameters(self):
+        """
+        If --start is provided without --end, then we should get an
+        appropriate error.
+        """
+        exit_mock = self.mocker.replace("sys.exit")
+        exit_mock(1)
+        self.mocker.replay()
+
+        command = Command()
+        stderr = StringIO()
+        command.execute(start="2013-06-05", stderr=stderr)
+
+        self.assertTrue(
+            "Error: --start and --end must be supplied" in stderr.getvalue())
+
+    def test_error_with_missing_start_parameters(self):
+        """
+        If --end is provided without --start, then we should get an
+        appropriate error.
+        """
+        exit_mock = self.mocker.replace("sys.exit")
+        exit_mock(1)
+        self.mocker.replay()
+
+        command = Command()
+        stderr = StringIO()
+        command.execute(end="2013-06-05", stderr=stderr)
+
+        self.assertTrue(
+            "Error: --start and --end must be supplied" in stderr.getvalue())
+
+    def test_error_with_end_before_start(self):
+        """
+        If the end date is not before the start datae we should get an error.
+        """
+        exit_mock = self.mocker.replace("sys.exit")
+        exit_mock(1)
+        self.mocker.replay()
+
+        command = Command()
+        stderr = StringIO()
+        command.execute(start="2013-06-05", end="2013-05-05", stderr=stderr)
+
+        self.assertTrue(
+            "Error: start date must be before end date" in stderr.getvalue())

=== modified file 'lib/offspring/web/queuemanager/metrics.py'
--- lib/offspring/web/queuemanager/metrics.py	2012-01-12 08:29:41 +0000
+++ lib/offspring/web/queuemanager/metrics.py	2013-06-06 09:47:40 +0000
@@ -1,11 +1,14 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
-from datetime import datetime, timedelta
+import re
+from datetime import datetime, timedelta, time
+
+import dateutil.parser
 
 from offspring.web.queuemanager.models import BuildResult
 
 
-def get_build_result_metrics():
+def get_build_result_metrics(start=None, end=None):
     """
     Return some simple metrics, in a format suitable for iterating over and
     displaying in a command-line tool.
@@ -18,8 +21,13 @@
     ninety_days = (start_date - timedelta(days=90), start_date)
 
     build_types = [("scheduled", True), ("requested", False)]
-    build_periods = [("7 days", seven_days), ("30 days", thirty_days),
-                     ("90 days", ninety_days)]
+
+    if start and end:
+        days = calculate_days(start, end)
+        build_periods = [("%d days" % days, (start, end))]
+    else:
+        build_periods = [("7 days", seven_days), ("30 days", thirty_days),
+                         ("90 days", ninety_days)]
     metrics = [(BuildResult.metrics.get_average_build_time, "build"),
                (BuildResult.metrics.get_average_queue_time, "queue")]
 
@@ -32,3 +40,55 @@
                                 function(period=period, automated=automated)))
 
     return results
+
+
+def calculate_days(start, end):
+    """
+    Returns the number of days between two datetimes, rounded up.
+    """
+    delta = end - start
+    seconds = 86400 * delta.days
+    seconds += delta.seconds
+    return round(seconds / 86400.0, 0)
+
+
+def calculate_start_and_end(start, end):
+    """
+    Given up to two date strings, return a tuple of start/end date to query
+    metrics from.
+
+    The start date defaults to YYYY-01-01:00:00:00 of the current year.
+    The end date defaults to YYYY-12-31:23:59:59 of the current year.
+    """
+    try:
+        start_date = parse_pseudo_iso8601(start)
+    except ValueError:
+        raise ValueError("Invalid start date %s" % start)
+    try:
+        end_date = parse_pseudo_iso8601(end, end=True)
+    except ValueError:
+        raise ValueError("Invalid end date %s" % end)
+    return (start_date, end_date)
+
+
+def parse_pseudo_iso8601(date, end=False):
+    """
+    Parses a date from Murphy's pseudo-ISO 8601 format.
+    """
+    parsed_date = dateutil.parser.parse(date)
+    element_count = len(re.split("[,-]", date))
+    if element_count == 2:
+        if end:
+            start_of_next_month = datetime(parsed_date.year, parsed_date.month+1, 1)
+            parsed_date = start_of_next_month - timedelta(days=1)
+        else:
+            parsed_date = datetime(parsed_date.year, parsed_date.month, 1)
+    elif element_count == 1:
+        if end:
+            start_of_next_month = datetime(parsed_date.year + 1, 1, 1)
+            parsed_date = start_of_next_month - timedelta(days=1)
+        else:
+            parsed_date = datetime(parsed_date.year, 1, 1)
+    if end:
+        parsed_date = datetime.combine(parsed_date.date(), time(23, 59, 59))
+    return parsed_date

=== modified file 'lib/offspring/web/queuemanager/tests/test_metrics.py'
--- lib/offspring/web/queuemanager/tests/test_metrics.py	2012-03-14 15:21:44 +0000
+++ lib/offspring/web/queuemanager/tests/test_metrics.py	2013-06-06 09:47:40 +0000
@@ -1,11 +1,13 @@
 # Copyright 2010 Canonical Ltd.  This software is licensed under the
 # GNU Affero General Public License version 3 (see the file LICENSE).
+import unittest
 from datetime import datetime, timedelta
 
 from django.test import TestCase
 
 from offspring.web.queuemanager.metrics import (
-    get_build_result_metrics)
+    get_build_result_metrics, calculate_start_and_end,
+    calculate_days, parse_pseudo_iso8601)
 
 from offspring.web.queuemanager.models import BuildResult
 from offspring.enums import ProjectBuildStates
@@ -19,51 +21,54 @@
 
     def setUp(self):
         self.project = factory.make_project()
+        self.user = factory.make_user()
+
+    def create_test_data(self, user=None, offset=0):
+        """
+        Creates tests data with the specified requestor user and datetime offset.
+        """
+        start_date = datetime.utcnow()
+        last_week = start_date - timedelta(days=8)
+        last_month = start_date - timedelta(days=35)
+        # Two this week, average = 10 + 20/2 = 15
+        create_build_results_with_durations(
+            self.project, [10 + offset, 20],
+            build_date=start_date - timedelta(days=1),
+            requestor=user)
+
+        # Two this week, average = 30 + 18/2 = 24.0
+        create_build_results_with_queue_times(
+            self.project, [30 + offset, 18],
+            build_date=start_date - timedelta(days=1),
+            requestor=user)
+
+        # Two last week, 10 + 10 + 10 + 20/4 = 12.5
+        create_build_results_with_durations(
+            self.project, [10 + offset, 10], build_date=last_week,
+            requestor=user)
+
+        # Two last week, average = 30 + 18 + 14  11/4 = 73
+        create_build_results_with_queue_times(
+            self.project, [14 + offset, 11], build_date=last_week,
+            requestor=user)
+
+        # Two last month, 10 + 10 + 10 + 20 + 30 + 40/6 = 20
+        create_build_results_with_durations(
+            self.project, [30 + offset, 40], build_date=last_month,
+            requestor=user)
+
+        # Two last month, average = 30 + 18 + 14  11 + 30 + 20/6 = 82
+        create_build_results_with_queue_times(
+            self.project, [30 + offset, 20], build_date=last_month,
+            requestor=user)
 
     def test_get_build_result_metrics(self):
         """
         get_build_result_metrics should return a list of tuples with the metric
         being measured, and the time in minutes.
         """
-        def create_test_data(user=None, offset=0):
-            start_date = datetime.utcnow()
-            last_week = start_date - timedelta(days=8)
-            last_month = start_date - timedelta(days=35)
-            # Two this week, average = 10 + 20/2 = 15
-            create_build_results_with_durations(
-                self.project, [10 + offset, 20],
-                build_date=start_date - timedelta(days=1),
-                requestor=user)
-
-            # Two this week, average = 30 + 18/2 = 24.0
-            create_build_results_with_queue_times(
-                self.project, [30 + offset, 18],
-                build_date=start_date - timedelta(days=1),
-                requestor=user)
-
-            # Two last week, 10 + 10 + 10 + 20/4 = 12.5
-            create_build_results_with_durations(
-                self.project, [10 + offset, 10], build_date=last_week,
-                requestor=user)
-
-            # Two last week, average = 30 + 18 + 14  11/4 = 73
-            create_build_results_with_queue_times(
-                self.project, [14 + offset, 11], build_date=last_week,
-                requestor=user)
-
-            # Two last month, 10 + 10 + 10 + 20 + 30 + 40/6 = 20
-            create_build_results_with_durations(
-                self.project, [30 + offset, 40], build_date=last_month,
-                requestor=user)
-
-            # Two last month, average = 30 + 18 + 14  11 + 30 + 20/6 = 82
-            create_build_results_with_queue_times(
-                self.project, [30 + offset, 20], build_date=last_month,
-                requestor=user)
-
-        create_test_data()
-        user = factory.make_user()
-        create_test_data(user=user, offset=10)
+        self.create_test_data()
+        self.create_test_data(user=self.user, offset=10)
 
         self.assertEqual(
             [("Average scheduled build time (7 days)", 15.0 * 60),
@@ -80,6 +85,24 @@
              ("Average requested queue time (90 days)", 25.5 * 60)],
             get_build_result_metrics())
 
+    def test_get_build_result_metrics_with_start_and_end_date(self):
+        """
+        get_build_result_metrics should return a list of tuples with the metric
+        being measured, and the time in minutes.
+        """
+        self.create_test_data()
+        self.create_test_data(user=self.user, offset=10)
+
+        end_date = datetime.utcnow()
+        start_date = end_date - timedelta(days=10)
+
+        self.assertEqual(
+            [("Average scheduled build time (10 days)", 12.5 * 60),
+             ("Average scheduled queue time (10 days)", 18.25 * 60),
+             ("Average requested build time (10 days)", 17.5 * 60),
+             ("Average requested queue time (10 days)", 23.25 * 60),],
+            get_build_result_metrics(start_date, end_date))
+
 
 class ProjectGroupBuildMetricsTests(TestCase):
 
@@ -141,3 +164,79 @@
             15 * 60,
             BuildResult.metrics.get_average_queue_time(
                 project_group=self.project_group))
+
+
+class MetricsDateCalculatorTestCase(unittest.TestCase):
+
+    def test_start_end_dates(self):
+        """
+        * with a start value of YYYY-MM-DD, the actual value will be YYYY-MM-DD
+          00:00:00
+        * with a start value of YYYY, the actual value will be YYYY-01-01 00:00:00
+        * with an end value of YYYY-MM-DD, the actual value will be YYYY-MM-DD
+          23:59:59
+        * with a end value of YYYY, the actual value will be YYYY-12-31 23:59:59
+        """
+        self.assertEqual(
+            (datetime(2013, 06, 05, 0, 0, 0), datetime(2013, 06, 20, 23, 59, 59)),
+            calculate_start_and_end("2013-06-05", "2013-06-20"))
+        self.assertEqual(
+            (datetime(2013, 01, 01, 0, 0, 0), datetime(2013, 12, 31, 23, 59, 59)),
+            calculate_start_and_end("2013", "2013"))
+        self.assertEqual(
+            (datetime(2013, 05, 01, 0, 0, 0), datetime(2013, 10, 31, 23, 59, 59)),
+            calculate_start_and_end("2013-05", "2013-10"))
+        self.assertEqual(
+            (datetime(2013, 06, 05, 0, 0, 0), datetime(2013, 06, 05, 23, 59, 59)),
+            calculate_start_and_end("2013-06-05", "2013-06-05"))
+
+    def test_parse_errors(self):
+        """
+        calculate_start_and_end should return a sensible message if an error
+        occurs during parsing.
+        """
+        with self.assertRaises(ValueError) as cm:
+            calculate_start_and_end("TESTING", "2013-06-05")
+        self.assertEqual(cm.exception.message, "Invalid start date TESTING")
+
+        with self.assertRaises(ValueError) as cm:
+            calculate_start_and_end("2013-06-05", "TESTING")
+        self.assertEqual(cm.exception.message, "Invalid end date TESTING")
+
+    def test_calculate_days(self):
+        """
+        calculate_days should calculate the number of days between two
+        timestamps, rounding up, rather than rounding down as timedeltas do.
+        """
+        start = datetime(2013, 01, 01, 00, 00, 00)
+        end = datetime(2013, 12, 31, 23, 59, 59)
+        self.assertEqual(365, calculate_days(start, end))
+
+
+class ParsePseudoISO8601TestCase(unittest.TestCase):
+
+    def test_parse_pseudo_iso8601_start(self):
+      """
+      We should calculate the date in various permutations.
+      """
+      self.assertEqual(
+          datetime(2013, 06, 05, 0, 0, 0), parse_pseudo_iso8601("2013-06-05"))
+      self.assertEqual(
+          datetime(2013, 06, 01, 0, 0, 0), parse_pseudo_iso8601("2013-06"))
+      self.assertEqual(
+          datetime(2013, 01, 01, 0, 0, 0), parse_pseudo_iso8601("2013"))
+
+    def test_parse_pseudo_iso8601_end(self):
+      """
+      We should calculate the date in various permutations, if end is true, then
+      the date calculated should be the end of the period specified.
+      """
+      self.assertEqual(
+          datetime(2013, 06, 05, 23, 59, 59),
+          parse_pseudo_iso8601("2013-06-05", True))
+      self.assertEqual(
+          datetime(2013, 06, 30, 23, 59, 59),
+          parse_pseudo_iso8601("2013-06", True))
+      self.assertEqual(
+          datetime(2013, 12, 31, 23, 59, 59),
+          parse_pseudo_iso8601("2013", True))

=== modified file 'requirements/requirements.web.txt'
--- requirements/requirements.web.txt	2013-05-17 13:49:22 +0000
+++ requirements/requirements.web.txt	2013-06-06 09:47:40 +0000
@@ -14,3 +14,5 @@
 celery>=2.3.1
 django-celery>=2.3.0
 django-kombu>=0.9.4
+# Need 1.5 because of Python 2.x compatibility.
+python-dateutil==1.5