Merge pull request #81 from mattip/source

disambiguate benchmarks by source

Author: mattip

codespeed/migrations/0006_alter_benchmark_name_alter_benchmark_unique_together.py

@@ -0,0 +1,22 @@
+# Generated by Django 5.2.13 on 2026-07-17 06:34
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ('codespeed', '0005_benchmark_source_result_suite_version'),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name='benchmark',
+            name='name',
+            field=models.CharField(max_length=100),
+        ),
+        migrations.AlterUniqueTogether(
+            name='benchmark',
+            unique_together={('name', 'source')},
+        ),
+    ]

codespeed/models.py

@@ -181,7 +181,7 @@ class Benchmark(models.Model):
         ('M', 'Median'),
     )
 
-    name = models.CharField(unique=True, max_length=100)
+    name = models.CharField(max_length=100)
     parent = models.ForeignKey(
         'self', on_delete=models.CASCADE, verbose_name="parent",
         help_text="allows to group benchmarks in hierarchies",
@@ -195,6 +195,20 @@ class Benchmark(models.Model):
     default_on_comparison = models.BooleanField(
         "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):
         return self.name
 

codespeed/results.py

@@ -38,6 +38,12 @@ 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
+
     # Check that the Environment exists
     try:
         e = Environment.objects.get(name=item['environment'])
@@ -58,7 +64,11 @@ 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)
-    b, created = Benchmark.objects.get_or_create(name=data["benchmark"])
+    # 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)
 
     if created:
         if "description" in data:
@@ -69,8 +79,6 @@ def save_result(data, update_repo=True):
             b.units_title = data["units_title"]
         if "lessisbetter" in data:
             b.lessisbetter = data["lessisbetter"]
-        if "source" in data:
-            b.source = data["source"]
         b.full_clean()
         b.save()
 

codespeed/static/js/codespeed.js

@@ -43,7 +43,8 @@ $(function() {
 
     $('.togglefold').each(function() {
         var lis = $(this).parent().children("li");
-        var allUnchecked = lis.find("input[type='checkbox']").filter(':checked').length === 0;
+        // count radios too (timeline groups use radio inputs, comparison checkboxes)
+        var allUnchecked = lis.find("input").filter(':checked').length === 0;
         if (allUnchecked) {
             lis.hide();
             $(this).addClass('folded');

codespeed/static/js/timeline.js

@@ -605,7 +605,15 @@ function setValuesOfInputFields(params) {
     });
 
     var benchmark = valueOrDefault(params.ben, defaults.benchmark);
-    $("input:radio[name='benchmark']").filter("[value='" + benchmark + "']").prop('checked', true);
+    var benchRadio = $("input:radio[name='benchmark']").filter("[value='" + benchmark + "']");
+    if (benchRadio.length === 0 && benchmark !== "grid" && benchmark !== "show_none") {
+        // backwards compat: a bare '<name>' permalink defaults to the legacy suite
+        benchmark = benchmark + ".legacy";
+        benchRadio = $("input:radio[name='benchmark']").filter("[value='" + benchmark + "']");
+    }
+    benchRadio.prop('checked', true);
+    // reveal the suite accordion section that holds the selected benchmark
+    benchRadio.closest("ul").show().children("a.togglefold").removeClass('folded');
 
     var envDefault = (defaults.environments || []).map(String).join(',');
     var envIds = valueOrDefault(params.env, envDefault).split(',').filter(Boolean);

codespeed/templates/codespeed/timeline.html

@@ -65,13 +65,16 @@
       <label for="b_show_none">Display none</label>
     </li>
   </ul>{% endif %}
-  <ul>{% for bench in benchmarks|dictsort:"name" %}
+  {% regroup benchmarks by get_source_display as benchmark_groups %}
+  {% for group in benchmark_groups %}
+  <ul><a href="#" class="togglefold">{{ group.grouper }}</a>
+    {% for bench in group.list|dictsort:"name" %}
     <li title="{{ bench.description }}">
-      <input id="benchmark_{{ bench.id }}" type="radio" name="benchmark" value="{{ bench.name }}" />
+      <input id="benchmark_{{ bench.id }}" type="radio" name="benchmark" value="{{ bench.ident }}" />
       <label for="benchmark_{{ bench.id }}">{{ bench }}</label>
     </li>
     {% endfor %}
-  </ul>
+  </ul>{% endfor %}
   </div>
 </div>
 </div>
@@ -120,7 +123,7 @@
       baseline: "{{ defaultbaseline }}",
       executables: [{% for exe in checkedexecutables %}{{ exe.id }}, {% endfor %}],
       branches: [{% for b in branch_list %}"{{ branch }}", {% endfor %}],
-      benchmark: "{{ defaultbenchmark }}",
+      benchmark: "{{ defaultbenchmark_value }}",
       environments: [{% for env in defaultenvironments %}{{ env.id }}, {% endfor %}],
       equidistant: "{{ defaultequid }}",
       quartiles: "{{ defaultquarts }}",

codespeed/tests/test_views.py

@@ -190,14 +190,34 @@ def test_source_set_on_new_benchmark(self):
         b = Benchmark.objects.get(name='newbench')
         self.assertEqual(b.source, 'pyperformance')
 
-    def test_source_not_changed_on_existing_benchmark(self):
-        """source in the payload should not overwrite an existing Benchmark"""
+    def test_source_defaults_to_legacy(self):
+        """A payload without a source creates a 'legacy' Benchmark"""
+        self.client.post(self.path, self.data)
+        b = Benchmark.objects.get(name='float')
+        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."""
         self.client.post(self.path, self.data)
         modified_data = copy.deepcopy(self.data)
         modified_data['source'] = 'pyperformance'
         self.client.post(self.path, modified_data)
-        b = Benchmark.objects.get(name='float')
-        self.assertEqual(b.source, 'legacy')
+
+        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)
+
+    def test_invalid_source_rejected(self):
+        """An unknown source is rejected instead of creating a bogus row"""
+        modified_data = copy.deepcopy(self.data)
+        modified_data['source'] = 'bogus'
+        response = self.client.post(self.path, modified_data)
+        self.assertEqual(response.status_code, 400)
+        self.assertFalse(Benchmark.objects.filter(name='float').exists())
 
 
 @override_settings(ALLOW_ANONYMOUS_POST=True)

codespeed/views.py

@@ -25,7 +25,7 @@
 from .views_data import (get_default_environment, getbaselineexecutables,
                          getdefaultexecutable, getcomparisonexes,
                          get_benchmark_results, get_num_revs_and_benchmarks,
-                         get_stats_with_defaults)
+                         get_stats_with_defaults, parse_benchmark_ident)
 from .results import save_result, create_report_if_enough_data
 from . import commits
 from .validators import validate_results_request
@@ -717,7 +717,9 @@ def timeline(request):
     baseline = getbaselineexecutables()
     defaultbaseline = None
     if len(baseline) > 1:
-        defaultbaseline = str(baseline[1]['executable'].id) + "+"
+        # must match the option keys built in getbaselineexecutables()
+        # ("<exe.id>:<rev.id>"), which gettimelinedata splits on ":"
+        defaultbaseline = str(baseline[1]['executable'].id) + ":"
         defaultbaseline += str(baseline[1]['revision'].id)
     if "base" in data and data['base'] != "undefined":
         try:
@@ -737,7 +739,9 @@ def timeline(request):
                 lastrevisions.append(revs_int)
             defaultlast = revs_int
 
-    benchmarks = Benchmark.objects.all()
+    # order by source so the timeline sidebar can {% regroup %} into
+    # per-suite accordion sections
+    benchmarks = Benchmark.objects.all().order_by('source', 'name')
 
     defaultbenchmark = "grid"
     if not len(benchmarks):
@@ -748,9 +752,10 @@ def timeline(request):
         if settings.DEF_BENCHMARK in ['grid', 'show_none']:
             defaultbenchmark = settings.DEF_BENCHMARK
         else:
+            def_name, def_source = parse_benchmark_ident(settings.DEF_BENCHMARK)
             try:
                 defaultbenchmark = Benchmark.objects.get(
-                    name=settings.DEF_BENCHMARK)
+                    name=def_name, source=def_source)
             except Benchmark.DoesNotExist:
                 pass
     elif len(benchmarks) >= get_setting('TIMELINE_GRID_LIMIT', 30):
@@ -760,7 +765,9 @@ def timeline(request):
         if data['ben'] == "show_none":
             defaultbenchmark = data['ben']
         else:
-            defaultbenchmark = get_object_or_404(Benchmark, name=data['ben'])
+            ben_name, ben_source = parse_benchmark_ident(data['ben'])
+            defaultbenchmark = get_object_or_404(
+                Benchmark, name=ben_name, source=ben_source)
 
     if 'equid' in data:
         defaultequid = data['equid']
@@ -785,12 +792,19 @@ 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:
+        defaultbenchmark_value = defaultbenchmark
     return render(request, 'codespeed/timeline.html', {
         'pagedesc': pagedesc,
         'checkedexecutables': checkedexecutables,
         'defaultbaseline': defaultbaseline,
         'baseline': baseline,
         'defaultbenchmark': defaultbenchmark,
+        'defaultbenchmark_value': defaultbenchmark_value,
         'defaultenvironment': defaultenviro,
         'defaultenvironments': defaultenvironments,
         'lastrevisions': lastrevisions,
@@ -921,9 +935,11 @@ def changes(request):
             pass
 
     baseline = getbaselineexecutables()
-    defaultbaseline = "+"
+    defaultbaseline = "none"
     if len(baseline) > 1:
-        defaultbaseline = str(baseline[1]['executable'].id) + "+"
+        # 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":
         try:

codespeed/views_data.py

@@ -10,6 +10,20 @@
     Environment, Benchmark, Result)
 
 
+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.
+    """
+    name, _, suffix = ben.rpartition('.')
+    if name and suffix in dict(Benchmark.S_TYPES):
+        return name, suffix
+    return ben, 'legacy'
+
+
 def get_default_environment(enviros, data, multi=False):
     """Returns the default environment. Preference level is:
         * Present in URL parameters (permalinks)
@@ -169,7 +183,8 @@ def get_benchmark_results(data):
     project = Project.objects.get(name=data['proj'])
     executable = Executable.objects.get(name=data['exe'], project=project)
     branch = Branch.objects.get(name=data['branch'], project=project)
-    benchmark = Benchmark.objects.get(name=data['ben'])
+    ben_name, ben_source = parse_benchmark_ident(data['ben'])
+    benchmark = Benchmark.objects.get(name=ben_name, source=ben_source)
 
     number_of_revs = int(data.get('revs', 10))
 
@@ -252,7 +267,9 @@ def get_num_revs_and_benchmarks(data):
         benchmarks = []
         number_of_revs = int(data.get('revs', 10))
     else:
-        benchmarks = [get_object_or_404(Benchmark, name=data['ben'])]
+        ben_name, ben_source = parse_benchmark_ident(data['ben'])
+        benchmarks = [get_object_or_404(
+            Benchmark, name=ben_name, source=ben_source)]
         number_of_revs = int(data.get('revs', 10))
     return number_of_revs, benchmarks