Additional review feedback

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):

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()
+
+        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()
 
-        # 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)
+        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,
+        )

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):

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 = (

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)}")

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(

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)
 

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)
 

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.