From 8c8c65d28d9c13cb2b3277ad7a36aa1b27784cb8 Mon Sep 17 00:00:00 2001 From: mattip Date: Fri, 17 Jul 2026 09:50:52 +0300 Subject: [PATCH 1/2] disambiguate benchmarks by source --- codespeed/models.py | 16 ++++++++++- codespeed/results.py | 14 +++++++--- codespeed/static/js/codespeed.js | 3 ++- codespeed/static/js/timeline.js | 10 ++++++- codespeed/templates/codespeed/timeline.html | 11 +++++--- codespeed/tests/test_views.py | 28 ++++++++++++++++--- codespeed/views.py | 30 ++++++++++++++++----- codespeed/views_data.py | 21 +++++++++++++-- 8 files changed, 110 insertions(+), 23 deletions(-) diff --git a/codespeed/models.py b/codespeed/models.py index 1bc24803..f49fd1b1 100644 --- a/codespeed/models.py +++ b/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=`` permalinks keep working. + """ + return "%s.%s" % (self.name, self.source) + def __str__(self): return self.name diff --git a/codespeed/results.py b/codespeed/results.py index f806949f..02c101f3 100644 --- a/codespeed/results.py +++ b/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() diff --git a/codespeed/static/js/codespeed.js b/codespeed/static/js/codespeed.js index 2fd23f9d..68df1e7a 100644 --- a/codespeed/static/js/codespeed.js +++ b/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'); diff --git a/codespeed/static/js/timeline.js b/codespeed/static/js/timeline.js index 6f921856..9843ec5f 100644 --- a/codespeed/static/js/timeline.js +++ b/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 '' 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); diff --git a/codespeed/templates/codespeed/timeline.html b/codespeed/templates/codespeed/timeline.html index 71fa46a0..4a4e6484 100644 --- a/codespeed/templates/codespeed/timeline.html +++ b/codespeed/templates/codespeed/timeline.html @@ -65,13 +65,16 @@ {% endif %} -
    {% for bench in benchmarks|dictsort:"name" %} + {% regroup benchmarks by get_source_display as benchmark_groups %} + {% for group in benchmark_groups %} +
      {{ group.grouper }} + {% for bench in group.list|dictsort:"name" %}
    • - +
      • {% endfor %} -
    +
{% endfor %} @@ -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 }}", diff --git a/codespeed/tests/test_views.py b/codespeed/tests/test_views.py index 4d1acca9..985cb879 100644 --- a/codespeed/tests/test_views.py +++ b/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) diff --git a/codespeed/views.py b/codespeed/views.py index 5b324834..9c300508 100644 --- a/codespeed/views.py +++ b/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() + # (":"), 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 ":" 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: diff --git a/codespeed/views_data.py b/codespeed/views_data.py index 29deed9b..a1c5717a 100644 --- a/codespeed/views_data.py +++ b/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 ``.`` (e.g. 'nbody.pyperformance') and, for + backwards compatibility, a bare ```` 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 From e812104745ea8e9b87ef53dc817bacc3637e63c2 Mon Sep 17 00:00:00 2001 From: mattip Date: Fri, 17 Jul 2026 09:57:05 +0300 Subject: [PATCH 2/2] add migration --- ...rk_name_alter_benchmark_unique_together.py | 22 +++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 codespeed/migrations/0006_alter_benchmark_name_alter_benchmark_unique_together.py diff --git a/codespeed/migrations/0006_alter_benchmark_name_alter_benchmark_unique_together.py b/codespeed/migrations/0006_alter_benchmark_name_alter_benchmark_unique_together.py new file mode 100644 index 00000000..2616bfd8 --- /dev/null +++ b/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')}, + ), + ]