From 24a414806e983ee59fe11085ba6730c739b2a327 Mon Sep 17 00:00:00 2001 From: mattip Date: Fri, 17 Jul 2026 14:09:51 +0300 Subject: [PATCH] more source fixes --- codespeed/admin.py | 12 +++++++++--- .../management/commands/import_sqlite_dump.py | 4 ++-- codespeed/models.py | 8 -------- codespeed/results.py | 17 ++++++++++++----- codespeed/tests/test_views.py | 12 +++++++++--- codespeed/views.py | 16 +++++----------- codespeed/views_data.py | 8 +------- 7 files changed, 38 insertions(+), 39 deletions(-) diff --git a/codespeed/admin.py b/codespeed/admin.py index 08a32ca6..abf9d794 100644 --- a/codespeed/admin.py +++ b/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: diff --git a/codespeed/management/commands/import_sqlite_dump.py b/codespeed/management/commands/import_sqlite_dump.py index 623f1997..9aba6198 100644 --- a/codespeed/management/commands/import_sqlite_dump.py +++ b/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()) diff --git a/codespeed/models.py b/codespeed/models.py index f49fd1b1..d9b4cb50 100644 --- a/codespeed/models.py +++ b/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=`` permalinks keep working. - """ return "%s.%s" % (self.name, self.source) def __str__(self): diff --git a/codespeed/results.py b/codespeed/results.py index 02c101f3..97e3e2ea 100644 --- a/codespeed/results.py +++ b/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) diff --git a/codespeed/tests/test_views.py b/codespeed/tests/test_views.py index 985cb879..8f07a61f 100644 --- a/codespeed/tests/test_views.py +++ b/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): diff --git a/codespeed/views.py b/codespeed/views.py index 9c300508..723f2e59 100644 --- a/codespeed/views.py +++ b/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() - # (":"), which gettimelinedata splits on ":" + # 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": @@ -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 ":" option keys from - # getbaselineexecutables() + # 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": diff --git a/codespeed/views_data.py b/codespeed/views_data.py index a1c5717a..74b9ed42 100644 --- a/codespeed/views_data.py +++ b/codespeed/views_data.py @@ -11,13 +11,7 @@ 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. - """ + """Split a '.' (or bare '') into (name, source='legacy').""" name, _, suffix = ben.rpartition('.') if name and suffix in dict(Benchmark.S_TYPES): return name, suffix