From 9a888a62fd421947c936dbb042e35392b79c8243 Mon Sep 17 00:00:00 2001 From: Jeremy Stretch Date: Wed, 5 Aug 2026 15:46:37 -0400 Subject: [PATCH] Additional review feedback --- netbox/core/tests/test_api.py | 26 ++++++++++-- netbox/core/tests/test_models.py | 41 ++++++++++++++----- netbox/core/tests/test_tables.py | 36 +++++++++++----- netbox/core/tests/test_views.py | 33 ++++++++++++++- .../extras/management/commands/runscript.py | 3 +- .../extras/tests/test_management_commands.py | 7 ++-- netbox/netbox/tables/columns.py | 3 ++ netbox/utilities/string.py | 12 +++--- netbox/utilities/tests/test_string.py | 6 ++- 9 files changed, 130 insertions(+), 37 deletions(-) diff --git a/netbox/core/tests/test_api.py b/netbox/core/tests/test_api.py index c043e2948..75eb4acb5 100644 --- a/netbox/core/tests/test_api.py +++ b/netbox/core/tests/test_api.py @@ -212,7 +212,7 @@ class JobTestCase( ) def test_list_objects_by_execution_time(self): - """The Job list endpoint supports filtering and ordering by execution_time.""" + """The Job list endpoint supports filtering by execution_time.""" self.add_permissions('core.view_job') url = reverse('core-api:job-list') @@ -221,10 +221,30 @@ class JobTestCase( self.assertHttpStatus(response, status.HTTP_200_OK) self.assertEqual(response.data['count'], 1) - # Ordering by execution_time should be accepted (NULLs sort to one end) + def test_ordering_by_execution_time(self): + """ + Ordering by execution_time must place jobs with no execution time last in both directions, + and must rank a running job by its elapsed time (matching the jobs table in the UI). + """ + self.add_permissions('core.view_job') + url = reverse('core-api:job-list') + + # 'Job 2' is running; give it a start time so it has an elapsed time exceeding Job 3's 90s + Job.objects.filter(name='Job 2').update(started=timezone.now() - timezone.timedelta(hours=1)) + + response = self.client.get(f'{url}?ordering=-execution_time', **self.header) + self.assertHttpStatus(response, status.HTTP_200_OK) + self.assertEqual( + [job['name'] for job in response.data['results']], + ['Job 2', 'Job 3', 'Job 1'], + ) + response = self.client.get(f'{url}?ordering=execution_time', **self.header) self.assertHttpStatus(response, status.HTTP_200_OK) - self.assertEqual(response.data['count'], 3) + self.assertEqual( + [job['name'] for job in response.data['results']], + ['Job 3', 'Job 2', 'Job 1'], + ) class BackgroundTaskTestCase(RQQueueTestMixin, TestCase): diff --git a/netbox/core/tests/test_models.py b/netbox/core/tests/test_models.py index e19fbdb3f..bf1f169a1 100644 --- a/netbox/core/tests/test_models.py +++ b/netbox/core/tests/test_models.py @@ -418,17 +418,36 @@ class JobTestCase(TestCase): self.assertIsNone(job.started) self.assertIsNone(job.elapsed_time) - @patch('core.models.jobs.job_end') - def test_duration_derives_from_execution_time(self, mock_job_end): + def test_elapsed_time_expression_matches_property(self): """ - The duration property should be rendered from the recorded execution_time, and should be - null for a job which never started. + The elapsed_time_expression() queryset expression should agree with the elapsed_time + property for completed, running, and never-started jobs. """ - job = self._make_job(None, JobNotificationChoices.NOTIFICATION_NEVER) - job.execution_time = timedelta(seconds=90) - self.assertEqual(job.duration, '1 minutes, 30.00 seconds') + completed = self._make_job(None, JobNotificationChoices.NOTIFICATION_NEVER) + completed.started = timezone.now() - timedelta(seconds=90) + completed.completed = timezone.now() + completed.execution_time = timedelta(seconds=90) + completed.status = JobStatusChoices.STATUS_COMPLETED + completed.save() - # A job terminated without ever starting has no execution time, and thus no duration - unstarted = self._make_job(None, JobNotificationChoices.NOTIFICATION_NEVER) - unstarted.terminate(status=JobStatusChoices.STATUS_ERRORED) - self.assertIsNone(unstarted.duration) + running = self._make_job(None, JobNotificationChoices.NOTIFICATION_NEVER) + running.started = timezone.now() - timedelta(minutes=5) + running.save() + + pending = self._make_job(None, JobNotificationChoices.NOTIFICATION_NEVER) + pending.status = JobStatusChoices.STATUS_PENDING + pending.save() + + annotated = { + job.pk: job + for job in Job.objects.annotate(elapsed=Job.elapsed_time_expression()) + } + + self.assertEqual(annotated[completed.pk].elapsed, timedelta(seconds=90)) + self.assertIsNone(annotated[pending.pk].elapsed) + # The running job's elapsed time is computed at query time, so compare approximately + self.assertAlmostEqual( + annotated[running.pk].elapsed.total_seconds(), + running.elapsed_time.total_seconds(), + delta=5, + ) diff --git a/netbox/core/tests/test_tables.py b/netbox/core/tests/test_tables.py index 5727d1a76..bb75446a8 100644 --- a/netbox/core/tests/test_tables.py +++ b/netbox/core/tests/test_tables.py @@ -101,24 +101,40 @@ class JobExecutionTimeColumnTestCase(TestCase): index = rows[0].index('Execution Time') self.assertIsNone(rows[1][index]) - def test_ordering_sorts_nulls_last(self): + def test_ordering_matches_displayed_values(self): """ - Jobs with no recorded execution time must sort last in both directions, so that sorting by - execution time does not bury the longest-running jobs behind pending ones. + Sorting must order by the value the column displays — which for a running job is its elapsed + time, not a null — so that a long-running job is not buried. Jobs which never started sort + last in both directions. """ - recorded = ['negative', 'completed-subsecond', 'completed-90s', 'completed-long'] - unrecorded = {'running', 'pending'} + # 'running' has been going 5 minutes, so it sorts between the 90s and 2d3h jobs + ascending = ['negative', 'completed-subsecond', 'completed-90s', 'running', 'completed-long'] for descending, expected in ( - (False, recorded), - (True, list(reversed(recorded))), + (False, ascending), + (True, list(reversed(ascending))), ): with self.subTest(descending=descending): table = JobTable(Job.objects.all()) - queryset, _modified = table.columns['execution_time'].order(Job.objects.all(), descending) + queryset, modified = table.columns['execution_time'].order(Job.objects.all(), descending) + self.assertTrue(modified) names = list(queryset.values_list('name', flat=True)) - self.assertEqual(names[:len(recorded)], expected) - self.assertEqual(set(names[len(recorded):]), unrecorded) + self.assertEqual(names, expected + ['pending']) + + def test_ordering_breaks_ties_on_pk(self): + """ + Tied rows need a stable secondary sort, or paginating through them can skip or repeat rows. + """ + Job.objects.bulk_create( + Job(name=f'tied-{i}', job_id=uuid.uuid4(), status=JobStatusChoices.STATUS_PENDING) + for i in range(4) + ) + table = JobTable(Job.objects.all()) + queryset, _modified = table.columns['execution_time'].order(Job.objects.filter( + name__startswith='tied-' + ), True) + pks = list(queryset.values_list('pk', flat=True)) + self.assertEqual(pks, sorted(pks)) class ObjectChangeTableTestCase(TableTestCases.StandardTableTestCase): diff --git a/netbox/core/tests/test_views.py b/netbox/core/tests/test_views.py index ad89d002a..fde439571 100644 --- a/netbox/core/tests/test_views.py +++ b/netbox/core/tests/test_views.py @@ -1,7 +1,7 @@ import json import urllib.parse import uuid -from datetime import UTC, datetime +from datetime import UTC, datetime, timedelta from django.contrib.contenttypes.models import ContentType from django.urls import reverse @@ -151,6 +151,37 @@ class JobTestCase( ] ) + def test_execution_time_on_detail_view(self): + """ + The job detail view must present execution time consistently with the jobs list: the recorded + value for a completed job, the elapsed time (visually distinguished) for a running one, and a + placeholder for a job which never started. + """ + self.add_permissions('core.view_job') + now = timezone.now() + + completed = Job.objects.get(name='Job 3') + completed.started = now - timedelta(seconds=90) + completed.completed = now + completed.execution_time = timedelta(seconds=90) + completed.save() + response = self.client.get(completed.get_absolute_url()) + self.assertHttpStatus(response, 200) + self.assertIn('1m 30s', str(response.content)) + + running = Job.objects.get(name='Job 2') + running.started = now - timedelta(hours=2) + running.save() + response = self.client.get(running.get_absolute_url()) + self.assertHttpStatus(response, 200) + content = str(response.content) + self.assertIn('2h', content) + self.assertIn('Still running', content) + + pending = Job.objects.get(name='Job 1') + response = self.client.get(pending.get_absolute_url()) + self.assertHttpStatus(response, 200) + class JobLogViewTestCase(TestCase): user_permissions = ( diff --git a/netbox/extras/management/commands/runscript.py b/netbox/extras/management/commands/runscript.py index ef3a46fac..60948058f 100644 --- a/netbox/extras/management/commands/runscript.py +++ b/netbox/extras/management/commands/runscript.py @@ -9,6 +9,7 @@ from extras.jobs import ScriptJob from extras.scripts import get_module_and_script from users.models import User from utilities.request import NetBoxFakeRequest +from utilities.string import humanize_duration class Command(BaseCommand): @@ -106,4 +107,4 @@ class Command(BaseCommand): commit=commit, ) - logger.info(f"Script completed in {job.duration}") + logger.info(f"Script completed in {humanize_duration(job.elapsed_time)}") diff --git a/netbox/extras/tests/test_management_commands.py b/netbox/extras/tests/test_management_commands.py index afc2f8d73..968d48760 100644 --- a/netbox/extras/tests/test_management_commands.py +++ b/netbox/extras/tests/test_management_commands.py @@ -1,3 +1,4 @@ +from datetime import timedelta from io import BytesIO, StringIO from types import SimpleNamespace from unittest.mock import MagicMock, patch @@ -273,7 +274,7 @@ class RunScriptTestCase(TestCase): return form script_obj = SimpleNamespace(python_class=TestScript) - job = SimpleNamespace(duration='0 seconds') + job = SimpleNamespace(elapsed_time=timedelta(0)) with ( patch( @@ -358,7 +359,7 @@ class RunScriptTestCase(TestCase): return form script_obj = SimpleNamespace(python_class=TestScript) - job = SimpleNamespace(duration='0 seconds') + job = SimpleNamespace(elapsed_time=timedelta(0)) with ( patch( @@ -398,7 +399,7 @@ class RunScriptTestCase(TestCase): return form script_obj = SimpleNamespace(python_class=TestScript) - job = SimpleNamespace(duration='0 seconds') + job = SimpleNamespace(elapsed_time=timedelta(0)) with ( patch( diff --git a/netbox/netbox/tables/columns.py b/netbox/netbox/tables/columns.py index bde929776..a4e302105 100644 --- a/netbox/netbox/tables/columns.py +++ b/netbox/netbox/tables/columns.py @@ -119,6 +119,9 @@ class DurationColumn(tables.Column): """ def render(self, value): if not isinstance(value, timedelta): + if not value: + # A zero count of minutes renders as empty rather than "0s" + return '' value = timedelta(minutes=value) return humanize_duration(value) diff --git a/netbox/utilities/string.py b/netbox/utilities/string.py index 75769993b..404c34f50 100644 --- a/netbox/utilities/string.py +++ b/netbox/utilities/string.py @@ -11,9 +11,9 @@ __all__ = ( def humanize_duration(value): """ - Express a timedelta in a human-friendly format. Example: 1h 5m 23s. Sub-second durations are - rendered with millisecond precision (e.g. 0.43s). Returns an empty string for None; zero and - negative durations render as "0s". + Express a timedelta in a human-friendly format. Example: 1h 5m 23s. Durations of a second or + more are rounded to whole seconds; shorter durations are rounded to the millisecond (e.g. + 0.43s). Returns an empty string for None; zero and negative durations render as "0s". """ if value is None: return '' @@ -21,15 +21,15 @@ def humanize_duration(value): # Negative durations (which can result from clock skew) are clamped to zero total_seconds = max(value.total_seconds(), 0) - # Render sub-second durations with millisecond precision, as rounding them to whole seconds - # would report every short-lived duration as zero. Trailing zeros are stripped. + # Render sub-second durations to the millisecond, as rounding them to whole seconds would + # report every short-lived duration as zero. Trailing zeros are stripped. if 0 < total_seconds < 1: milliseconds = f'{total_seconds:.3f}'.rstrip('0').rstrip('.') if milliseconds != '0': return f'{milliseconds}s' # Round to whole seconds and decompose - days, remainder = divmod(int(total_seconds), 86400) + days, remainder = divmod(round(total_seconds), 86400) hours, remainder = divmod(remainder, 3600) minutes, seconds = divmod(remainder, 60) diff --git a/netbox/utilities/tests/test_string.py b/netbox/utilities/tests/test_string.py index 2a377ec98..c789ef23f 100644 --- a/netbox/utilities/tests/test_string.py +++ b/netbox/utilities/tests/test_string.py @@ -38,8 +38,10 @@ class HumanizeDurationTest(TestCase): # Anything below a millisecond has no decimal representation, so it reads as 0s. self.assertEqual(humanize_duration(timedelta(microseconds=400)), '0s') - def test_fractional_seconds_truncated_above_one_second(self): - self.assertEqual(humanize_duration(timedelta(seconds=1, milliseconds=999)), '1s') + def test_fractional_seconds_rounded_above_one_second(self): + self.assertEqual(humanize_duration(timedelta(seconds=1, milliseconds=999)), '2s') + self.assertEqual(humanize_duration(timedelta(seconds=1, milliseconds=100)), '1s') + self.assertEqual(humanize_duration(timedelta(seconds=59, milliseconds=600)), '1m') def test_negative_duration_clamped_to_zero(self): # A negative duration (e.g. resulting from clock skew) never renders as negative.