Merge pull request #82 from mattip/source2

more source fixes

Author: mattip

codespeed/admin.py

@@ -85,11 +85,17 @@ class EnvironmentAdmin(admin.ModelAdmin):
 
 @admin.register(Result)
 class ResultAdmin(admin.ModelAdmin):
-    list_display = ('revision', 'benchmark', 'executable', 'environment',
-                    'value', 'date')
-    list_filter = ('environment', 'executable', 'date', 'benchmark')
+    list_display = ('revision', 'benchmark', 'source', 'executable',
+                    'environment', 'value', 'date')
+    list_filter = ('environment', 'executable', 'date', 'benchmark__source',
+                   'benchmark')
+    list_select_related = ('revision', 'benchmark', 'executable', 'environment')
     raw_id_fields = ('revision', 'benchmark', 'executable', 'environment')
 
+    @admin.display(ordering='benchmark__source', description='Source')
+    def source(self, obj):
+        return obj.benchmark.source
+
 
 def recalculate_report(modeladmin, request, queryset):
     for report in queryset:

codespeed/management/commands/import_sqlite_dump.py

@@ -12,7 +12,7 @@
 from datetime import datetime
 
 from django.core.management.base import BaseCommand, CommandError
-from django.db import connection
+from django.db import connection, transaction
 
 # Columns that are stored as 0/1 integers in SQLite but need Python bools
 # for PostgreSQL's boolean type.
@@ -66,7 +66,7 @@ def handle(self, *args, **options):
 
         src.row_factory = sqlite3.Row
 
-        with connection.cursor() as cur:
+        with transaction.atomic(), connection.cursor() as cur:
             for table in _TABLES:
                 bool_cols = _BOOL_COLS.get(table, set())
                 dt_cols = _DT_COLS.get(table, set())

codespeed/models.py

@@ -196,17 +196,9 @@ class Benchmark(models.Model):
         "Default on comparison page", default=True)
 
     class Meta:
-        # The same benchmark name can exist in more than one suite
-        # (e.g. 'nbody' in both the legacy and pyperformance suites);
-        # source is part of the identity so results don't get merged.
         unique_together = (('name', 'source'),)
 
     def ident(self):
-        """Stable identifier used in timeline URLs/permalinks.
-
-        A bare name (no ``.source`` suffix) is treated as 'legacy' when
-        parsed back, so old ``?ben=<name>`` permalinks keep working.
-        """
         return "%s.%s" % (self.name, self.source)
 
     def __str__(self):

codespeed/results.py

@@ -12,6 +12,9 @@
 
 logger = logging.getLogger(__name__)
 
+# failed runs are recorded with a huge value; anything this big is bogus
+SENTINEL_THRESHOLD = 1000000
+
 
 def validate_result(item):
     """
@@ -38,12 +41,18 @@ def validate_result(item):
         elif key in item and item[key] == "":
             return 'Value for key "' + key + '" empty in request', error
 
-    # source is optional but, when given, must be a known suite. It is part
-    # of the Benchmark identity, so an unvalidated value would silently
-    # create a bogus benchmark row via get_or_create.
     if 'source' in item and item['source'] not in dict(Benchmark.S_TYPES):
         return 'Invalid source "%s"' % item['source'], error
 
+    try:
+        result_value = float(item['result_value'])
+    except (TypeError, ValueError):
+        return 'Value for key "result_value" is not a number', error
+    if result_value >= SENTINEL_THRESHOLD:
+        return ('Result value %s rejected: at or above the failed-run '
+                'sentinel threshold (%s)' % (
+                    item['result_value'], SENTINEL_THRESHOLD), error)
+
     # Check that the Environment exists
     try:
         e = Environment.objects.get(name=item['environment'])
@@ -64,8 +73,6 @@ def save_result(data, update_repo=True):
     p, created = Project.objects.get_or_create(name=data["project"])
     branch, created = Branch.objects.get_or_create(name=data["branch"],
                                                    project=p)
-    # source is part of the benchmark identity: the same name in a different
-    # suite is a distinct benchmark, so results are never merged across suites.
     source = data.get("source", "legacy")
     b, created = Benchmark.objects.get_or_create(
         name=data["benchmark"], source=source)

codespeed/tests/test_views.py

@@ -197,8 +197,7 @@ def test_source_defaults_to_legacy(self):
         self.assertEqual(b.source, 'legacy')
 
     def test_same_name_different_source_are_distinct(self):
-        """The same name in a different suite is a separate Benchmark, so
-        results are not merged across suites."""
+        """The same name in a different suite is a separate Benchmark"""
         self.client.post(self.path, self.data)
         modified_data = copy.deepcopy(self.data)
         modified_data['source'] = 'pyperformance'
@@ -207,7 +206,6 @@ def test_same_name_different_source_are_distinct(self):
         legacy = Benchmark.objects.get(name='float', source='legacy')
         pyperf = Benchmark.objects.get(name='float', source='pyperformance')
         self.assertNotEqual(legacy.pk, pyperf.pk)
-        # each benchmark owns its own result, nothing merged onto the other
         self.assertEqual(legacy.results.count(), 1)
         self.assertEqual(pyperf.results.count(), 1)
 
@@ -219,6 +217,14 @@ def test_invalid_source_rejected(self):
         self.assertEqual(response.status_code, 400)
         self.assertFalse(Benchmark.objects.filter(name='float').exists())
 
+    def test_sentinel_value_rejected(self):
+        """A failed-run sentinel value is rejected"""
+        modified_data = copy.deepcopy(self.data)
+        modified_data['result_value'] = 1000000
+        response = self.client.post(self.path, modified_data)
+        self.assertEqual(response.status_code, 400)
+        self.assertFalse(Result.objects.filter(benchmark__name='float').exists())
+
 
 @override_settings(ALLOW_ANONYMOUS_POST=True)
 class TestAddJSONResults(TestCase):

codespeed/views.py

@@ -388,9 +388,9 @@ def comparison(request):
             units = unit_qs[0].units
             lessisbetter = (unit_qs[0].lessisbetter and
                             ' (less is better)' or ' (more is better)')
-            bench_units[unit] = [
-                [b.id for b in unit_qs], lessisbetter, units
-            ]
+            # a units_title (e.g. 'Time') can span sources: accumulate, don't overwrite
+            entry = bench_units.setdefault(unit, [[], lessisbetter, units])
+            entry[0].extend(b.id for b in unit_qs)
     checkedbenchmarks = []
     if 'ben' in data:
         checkedbenchmarks = _parse_ben_param(data['ben'])
@@ -717,8 +717,7 @@ def timeline(request):
     baseline = getbaselineexecutables()
     defaultbaseline = None
     if len(baseline) > 1:
-        # must match the option keys built in getbaselineexecutables()
-        # ("<exe.id>:<rev.id>"), which gettimelinedata splits on ":"
+        # must match the "<exe.id>:<rev.id>" option keys from getbaselineexecutables()
         defaultbaseline = str(baseline[1]['executable'].id) + ":"
         defaultbaseline += str(baseline[1]['revision'].id)
     if "base" in data and data['base'] != "undefined":
@@ -739,8 +738,6 @@ def timeline(request):
                 lastrevisions.append(revs_int)
             defaultlast = revs_int
 
-    # order by source so the timeline sidebar can {% regroup %} into
-    # per-suite accordion sections
     benchmarks = Benchmark.objects.all().order_by('source', 'name')
 
     defaultbenchmark = "grid"
@@ -792,8 +789,6 @@ def timeline(request):
     for proj in Project.objects.filter(track=True):
         executables[proj] = Executable.objects.filter(project=proj)
     use_median_bands = hasattr(settings, 'USE_MEDIAN_BANDS') and settings.USE_MEDIAN_BANDS
-    # The radio buttons carry 'name.source' idents, so the JS default must
-    # match that form (the 'grid'/'show_none' sentinels are passed through).
     if isinstance(defaultbenchmark, Benchmark):
         defaultbenchmark_value = defaultbenchmark.ident()
     else:
@@ -937,8 +932,7 @@ def changes(request):
     baseline = getbaselineexecutables()
     defaultbaseline = "none"
     if len(baseline) > 1:
-        # must match the "<exe.id>:<rev.id>" option keys from
-        # getbaselineexecutables()
+        # must match the "<exe.id>:<rev.id>" option keys from getbaselineexecutables()
         defaultbaseline = str(baseline[1]['executable'].id) + ":"
         defaultbaseline += str(baseline[1]['revision'].id)
     if "base" in data and data['base'] != "undefined":

codespeed/views_data.py

@@ -11,13 +11,7 @@
 
 
 def parse_benchmark_ident(ben):
-    """Split a timeline ``ben`` value into (name, source).
-
-    Accepts ``<name>.<source>`` (e.g. 'nbody.pyperformance') and, for
-    backwards compatibility, a bare ``<name>`` which defaults to the
-    'legacy' source. Benchmark names may themselves contain dots, so only
-    a trailing segment that is a known source slug is treated as the source.
-    """
+    """Split a '<name>.<source>' (or bare '<name>') into (name, source='legacy')."""
     name, _, suffix = ben.rpartition('.')
     if name and suffix in dict(Benchmark.S_TYPES):
         return name, suffix