From fdac4eb9f4593c2acc1ffa78123e635c202ec0f4 Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 17 Sep 2026 12:10:02 +0100 Subject: [PATCH 1/3] Compare the timeout tiers exactly, not approximately Every tier comparison is a sum, and a sum is exact only if every term is. One float among them converts the whole of it back, silently. So `seconds` returns a `fractions.Fraction` and so does everything the comparisons add to it. The range is the reason. humantime reaches 2**64 seconds and a double holds 53 bits of significand, so above 2**53 it cannot represent two budgets a second apart. `18446744073709551614s` and `18446744073709551615s` are both inputs in the estate differential and both convert to the same double, so an ordering between them compares equal and passes whichever way round it is written. At the other end a tenth of a second has no exact double, so a budget assembled from tenths and one written as a decimal would differ by a rounding error rather than by anything anyone configured. `display_seconds` returns a float beside it, whitaker's shape: a float is what a reader wants in a message and not what a comparison should rest on, and different names make each caller choose. The four call sites in #727 were not the whole of it. The three compositions the ordering contract evaluates each mix a duration with a workflow reading and a constant, so the watchdog budget, the job ceiling converted from `timeout-minutes`, and the five allowances in `timeout_budgets` are exact too. Making only the duration exact would have left every sum a float and the new contract vacuous. One float remains, deliberately. `Fraction` raises on `nan` and `inf`, which would turn a workflow interpolating an expression to `inf` into unreadable text rather than the named refusal that case deserves. The watchdog text is therefore parsed as a float, checked for finiteness and sign, then converted from the text rather than from the float, so a tenth stays a tenth. Nothing is compared against the float. `timeout_exactness_test` drives the three compositions at two to the sixtieth, where neighbouring doubles are 256 seconds apart and a one-second difference is lost outright rather than only on one side of a tie. Each case asserts the collapse beside the ordering, so a case that stopped exercising the loss fails rather than passing quietly. Every one of the eight terms was reverted to a float in turn and each failed a case naming it: Total.as_seconds 5 cases the watchdog reading 2 the job ceiling conversion 2 OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS 3 CEILING_MARGIN_SECONDS 3 COLD_BUILD_ALLOWANCE_SECONDS 3 TERMINATION_SAFETY_MARGIN_SECONDS 4 NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS 2 The two allowances that `required_ceiling` adds together fail the same ordering case, which is why the constants are also asserted one by one under their own names; the report then says which of the two moved. Nextest's default grace period fails only the `nextests-own-default` leg, so the two legs of that pair discriminate rather than duplicate. The Hypothesis properties and the two local unit tables move to the exact type with them, and their comparisons drop `pytest.approx`: a tolerance can now only hide a disagreement, since a millisecond is a thousandth and no float holds one. `required_ceiling` moves to `timeout_budgets`, beside the constants it sums. Its signature change took `timeout_ordering_test` past the 400-line limit, and the function belongs with its terms rather than with the contract that reads it. The reader still reads none of the estate differential's seventy-two disagreements, re-measured after the change. --- docs/developers-guide.md | 50 ++- .../coverage_lane_multi_step_test.py | 9 +- .../coverage_lane_reading_test.py | 20 +- tests/workflow_contracts/coverage_lanes.py | 19 +- tests/workflow_contracts/lane_environment.py | 20 +- tests/workflow_contracts/nextest_budgets.py | 34 +- .../nextest_duration_test.py | 18 +- tests/workflow_contracts/nextest_durations.py | 48 ++- tests/workflow_contracts/nextest_totals.py | 30 +- .../timeout_budget_properties_test.py | 25 +- tests/workflow_contracts/timeout_budgets.py | 58 ++- .../timeout_exactness_test.py | 352 ++++++++++++++++++ .../timeout_ordering_test.py | 29 +- .../workflow_contracts/whole_run_ordering.py | 9 +- .../whole_run_ordering_test.py | 23 +- 15 files changed, 641 insertions(+), 103 deletions(-) create mode 100644 tests/workflow_contracts/timeout_exactness_test.py diff --git a/docs/developers-guide.md b/docs/developers-guide.md index b78d27f4f..d0c779582 100644 --- a/docs/developers-guide.md +++ b/docs/developers-guide.md @@ -6972,12 +6972,60 @@ into seconds before the second arrives; summed the other way round the nanosecond accumulator overflows and the whole duration is refused, which would be this contract refusing a configuration nextest runs quite happily. +### The tiers are compared exactly, not approximately + +Every tier comparison is a sum, and a sum is exact only if every term is. One +`float` among them converts the whole of it back, and the conversion is silent. +So `seconds` returns a `fractions.Fraction`, and so does everything the +comparisons add to it: the watchdog budget read from a workflow, the job +ceiling converted from `timeout-minutes`, and the five allowances and margins +declared in `timeout_budgets.py`. + +The reason is the range. humantime reaches 2**64 seconds and a double holds 53 +bits of significand, so above 2**53 it cannot represent two budgets a second +apart. `18446744073709551614s` and `18446744073709551615s` are both inputs in +the estate differential and both convert to the same double, so an ordering +assertion between them compares equal and passes whichever way round it is +written. The nanosecond end makes the same point: a tenth of a second has no +exact double, so a budget assembled from tenths and one written as a decimal +would differ by a rounding error rather than by anything anyone configured. + +A float is still what a reader wants to see in a message, so `display_seconds` +returns one. The two are behind different names on purpose: a caller chooses +which it wants rather than getting the lossy one by default. + +None of this can be demonstrated on the budgets this repository configures. +They are minutes and seconds, nowhere near either end, and they never will be +otherwise, so a contract resting on the real files would pass with every term a +float. `timeout_exactness_test.py` therefore drives the three compositions the +ordering contract evaluates at two to the sixtieth, where neighbouring doubles +are 256 seconds apart and a one-second difference is lost outright rather than +only on one side of a tie. Each case asserts the collapse alongside the +ordering, so a case that stopped exercising the loss fails rather than passing +quietly, and one further case asserts that the values actually in force arrive +exact, so a float reintroduced on the live path is caught without waiting for a +budget nobody will set. + +Each of the eight terms was reverted to a float in turn and every one failed a +case naming it. Two of them, the outside-work allowance and the ceiling margin, +are added by the same function and fail the same ordering case, which is why +the constants are also asserted one by one under their own names: the report +then says which of the two moved. + +The watchdog reading is the one place a float still appears, and deliberately. +`Fraction` has no notion of `nan` or `inf` and raises on both, which would turn +a workflow interpolating an expression to `inf` into unreadable text rather +than the named refusal that case deserves. So the text is parsed as a float, +checked for finiteness and sign, and then converted from the text rather than +from the float, which keeps a tenth exactly a tenth. Nothing is compared +against the float on the way through. + The port's scope is narrow and deliberately so. `nextest_durations` owns one thing: turning the text of a nextest duration into seconds exactly as `humantime` would, and refusing what `humantime` refuses. It is a workflow-contract helper, not a repository-wide duration parser. Its call-sites are `nextest_budgets.py`, which reads `.config/nextest.toml` budgets, -`timeout_ordering_test.py`, and the two test modules that drive the reading +`timeout_ordering_test.py`, and the three test modules that drive the reading directly. Nothing outside `tests/workflow_contracts` imports it, and nothing inside should grow a second duration reader beside it. humantime's unit table sits beside it in `nextest_units.py`, and humantime's accumulator in diff --git a/tests/workflow_contracts/coverage_lane_multi_step_test.py b/tests/workflow_contracts/coverage_lane_multi_step_test.py index 91aadf233..e45220a21 100644 --- a/tests/workflow_contracts/coverage_lane_multi_step_test.py +++ b/tests/workflow_contracts/coverage_lane_multi_step_test.py @@ -14,6 +14,7 @@ Run via ``make test-workflow-contracts``. """ +import fractions import typing as typ import pytest @@ -125,15 +126,15 @@ def test_two_coverage_steps_in_one_job_are_judged_together() -> None: workflow="ci.yml", job="build-test", step="cover one", - watchdog=1800.0, - job_timeout=60 * 60.0, + watchdog=fractions.Fraction(1800), + job_timeout=fractions.Fraction(60 * 60), ), CoverageLane( workflow="ci.yml", job="build-test", step="cover two", - watchdog=2700.0, - job_timeout=60 * 60.0, + watchdog=fractions.Fraction(2700), + job_timeout=fractions.Fraction(60 * 60), ), ) diff --git a/tests/workflow_contracts/coverage_lane_reading_test.py b/tests/workflow_contracts/coverage_lane_reading_test.py index fe7880b61..2dfbc08fb 100644 --- a/tests/workflow_contracts/coverage_lane_reading_test.py +++ b/tests/workflow_contracts/coverage_lane_reading_test.py @@ -9,6 +9,7 @@ Run via ``make test-workflow-contracts``. """ +import fractions import typing as typ import pytest @@ -167,14 +168,21 @@ def test_the_required_ceiling_sums_the_watchdogs_and_adds_the_margin() -> None: the margin changes nothing observable. Both terms are therefore driven with controlled numbers. """ - assert required_ceiling([1800.0, 2700.0]) == pytest.approx( - 4500.0 + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS + assert required_ceiling([ + fractions.Fraction(1800), + fractions.Fraction(2700), + ]) == fractions.Fraction(4500) + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + ( + CEILING_MARGIN_SECONDS ), "two steps need the sum of their budgets, not the larger of them" - assert required_ceiling([1800.0]) == pytest.approx( - 1800.0 + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS + assert ( + required_ceiling([fractions.Fraction(1800)]) + == fractions.Fraction(1800) + + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + + CEILING_MARGIN_SECONDS ), "one step needs its own budget, the allowance and the margin" - assert required_ceiling([]) == pytest.approx( - OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS + assert ( + required_ceiling([]) + == OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS ), "the margin is a term of its own, not a fraction of the others" diff --git a/tests/workflow_contracts/coverage_lanes.py b/tests/workflow_contracts/coverage_lanes.py index d96697033..9538fd189 100644 --- a/tests/workflow_contracts/coverage_lanes.py +++ b/tests/workflow_contracts/coverage_lanes.py @@ -6,6 +6,7 @@ no module outgrows the 400-line limit the Python lint gate enforces. """ +import fractions import typing as typ from lane_environment import WatchdogValueError, nextest_profile_of, watchdog_of @@ -27,10 +28,11 @@ class CoverageLane(typ.NamedTuple): The job the step belongs to. step : str The step's declared name. - watchdog : float or None - The watchdog budget in seconds, or None when the job sets none + watchdog : fractions.Fraction or None + The watchdog budget in seconds exactly, or None when the job + sets none and so inherits the action's 1,800 s default. - job_timeout : float or None + job_timeout : fractions.Fraction or None The job's ``timeout-minutes`` in seconds, or None when it declares none and so inherits GitHub's six-hour default. condition : tuple[object, object] @@ -47,8 +49,8 @@ class CoverageLane(typ.NamedTuple): workflow: str job: str step: str - watchdog: float | None - job_timeout: float | None + watchdog: fractions.Fraction | None + job_timeout: fractions.Fraction | None condition: tuple[object, object] = (None, None) nextest_profile: str | None = None @@ -169,7 +171,12 @@ def _lanes_in_job( message = f"{workflow}:{job_name}: {error}" raise WatchdogValueError(message) from error raw_timeout = job.get("timeout-minutes") - timeout = None if raw_timeout is None else float(raw_timeout) * 60.0 + # Read from the text and multiplied exactly. This ceiling is + # compared against a sum of watchdog budgets and two allowances, so + # a float here would discard the exactness the other terms carry; + # the minute-to-second conversion is itself a term of that + # comparison rather than a display detail. + timeout = None if raw_timeout is None else fractions.Fraction(str(raw_timeout)) * 60 return [ CoverageLane( workflow=workflow, diff --git a/tests/workflow_contracts/lane_environment.py b/tests/workflow_contracts/lane_environment.py index 9fb279a67..a6efcd940 100644 --- a/tests/workflow_contracts/lane_environment.py +++ b/tests/workflow_contracts/lane_environment.py @@ -12,6 +12,7 @@ enforces. """ +import fractions import math import typing as typ @@ -22,7 +23,7 @@ def watchdog_of( document: dict[str, typ.Any], job: dict[str, typ.Any], step: dict[str, typ.Any], -) -> float | None: +) -> fractions.Fraction | None: """Return the watchdog budget in force for one step. All three levels are read, innermost first, as GitHub resolves them. @@ -143,7 +144,7 @@ class WatchdogValueError(ValueError): """ -def _budget_from(raw: object) -> float | None: +def _budget_from(raw: object) -> fractions.Fraction | None: """Return the resolved watchdog budget, or None when none is set. This reads the one declaration :func:`_declared_in_scope` chose, so @@ -166,8 +167,11 @@ def _budget_from(raw: object) -> float | None: Returns ------- - float or None - The budget in seconds, or None when the source sets none. + fractions.Fraction or None + The budget in seconds exactly, or None when the source sets + none. Exact because it is compared against a sum of budgets + read from three files, and one float among those terms loses + the whole comparison silently. Raises ------ @@ -180,6 +184,12 @@ def _budget_from(raw: object) -> float | None: text = str(raw).strip() if not text: return None + # Parsed as a float first and converted afterwards. `Fraction` has + # no notion of `nan` or `inf`: it raises on both, which would make + # them unreadable text rather than the named refusal below, and a + # workflow interpolating an expression to `inf` is exactly the case + # that refusal exists to name. The float here is a parser, not a + # value: nothing is compared against it before it becomes exact. try: seconds = float(text) except ValueError as error: @@ -198,4 +208,4 @@ def _budget_from(raw: object) -> float | None: f"ceiling arithmetic and failing there" ) raise WatchdogValueError(message) - return seconds + return fractions.Fraction(text) diff --git a/tests/workflow_contracts/nextest_budgets.py b/tests/workflow_contracts/nextest_budgets.py index 65ee0c2cf..e5347d17c 100644 --- a/tests/workflow_contracts/nextest_budgets.py +++ b/tests/workflow_contracts/nextest_budgets.py @@ -18,6 +18,7 @@ """ import tomllib +import typing as typ from itertools import starmap from nextest_durations import ( @@ -31,6 +32,9 @@ TERMINATION_SAFETY_MARGIN_SECONDS, ) +if typ.TYPE_CHECKING: + import fractions + #: One value-and-unit pair of a humantime duration. nextest parses its #: durations with `humantime` through `humantime_serde`, which reads a @@ -176,7 +180,7 @@ def _multiplier_of(path: str, multiplier: object) -> int: raise NextestConfigurationError(message) -def _budget_of(path: str, value: object) -> float: +def _budget_of(path: str, value: object) -> fractions.Fraction: """Return the per-test budget one ``slow-timeout`` declares. Parameters @@ -188,8 +192,8 @@ def _budget_of(path: str, value: object) -> float: Returns ------- - float - The budget in seconds. + fractions.Fraction + The budget in seconds, exactly. Raises ------ @@ -229,7 +233,7 @@ def _budget_of(path: str, value: object) -> float: return seconds(period) * _multiplier_of(path, multiplier) -def largest_test_allowance(config_text: str) -> float: +def largest_test_allowance(config_text: str) -> fractions.Fraction: """Return the longest a single test may run, in seconds. nextest warns once per ``period`` and terminates after @@ -245,8 +249,8 @@ def largest_test_allowance(config_text: str) -> float: Returns ------- - float - The longest per-test budget. + fractions.Fraction + The longest per-test budget, exactly. A ``slow-timeout`` that names no ``terminate-after`` raises :class:`UnboundedTestError` from :func:`_budget_of` rather than @@ -294,7 +298,7 @@ def bounds_a_single_test(config_text: str, profile: str = "default") -> bool: return isinstance(table, dict) and table.get("terminate-after") is not None -def grace_period(config_text: str) -> float: +def grace_period(config_text: str) -> fractions.Fraction: """Return the longest grace period the configuration names, in seconds. Read from the configuration rather than fixed, so a profile that @@ -308,7 +312,7 @@ def grace_period(config_text: str) -> float: Returns ------- - float + fractions.Fraction The largest configured grace period, or nextest's default. """ periods = [ @@ -320,7 +324,7 @@ def grace_period(config_text: str) -> float: return max(periods, default=NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS) -def termination_allowance(config_text: str) -> float: +def termination_allowance(config_text: str) -> fractions.Fraction: """Return the time nextest may take to stop the run, in seconds. Two terms, not one. Hitting the whole-run budget starts nextest's @@ -340,13 +344,17 @@ def termination_allowance(config_text: str) -> float: Returns ------- - float - The grace period plus the safety margin. + fractions.Fraction + The grace period plus the safety margin, exactly. Both terms + are exact so that the sum is: a float in either would convert + the whole of it back silently. """ return grace_period(config_text) + TERMINATION_SAFETY_MARGIN_SECONDS -def global_timeout(config_text: str, profile: str = CAPPED_PROFILE) -> float | None: +def global_timeout( + config_text: str, profile: str = CAPPED_PROFILE +) -> fractions.Fraction | None: """Return one profile's whole-run budget, or None when it sets none. Read from the named profile's own table alone. nextest's profiles @@ -367,7 +375,7 @@ def global_timeout(config_text: str, profile: str = CAPPED_PROFILE) -> float | N Returns ------- - float or None + fractions.Fraction or None The whole-run budget in seconds, or None when that profile declares none. diff --git a/tests/workflow_contracts/nextest_duration_test.py b/tests/workflow_contracts/nextest_duration_test.py index 2787d0d86..d478cb86c 100644 --- a/tests/workflow_contracts/nextest_duration_test.py +++ b/tests/workflow_contracts/nextest_duration_test.py @@ -12,6 +12,7 @@ parser rather than by reading about it. """ +import fractions import re import typing as typ @@ -28,7 +29,12 @@ #: The four units the repository's own configuration uses, with their #: lengths, for the scaling property below. -UNITS: typ.Final[dict[str, float]] = {"ms": 0.001, "s": 1.0, "m": 60.0, "h": 3600.0} +UNITS: typ.Final[dict[str, fractions.Fraction]] = { + "ms": fractions.Fraction(1, 1_000), + "s": fractions.Fraction(1), + "m": fractions.Fraction(60), + "h": fractions.Fraction(3_600), +} whole_numbers = st.integers(min_value=1, max_value=10_000) units = st.sampled_from(sorted(UNITS)) @@ -89,8 +95,14 @@ def test_each_unit_converts_exactly(duration: str, expected: float) -> None: @given(value=whole_numbers, unit=units) def test_every_unit_scales_its_value(value: int, unit: str) -> None: - """A duration is its number times the length of its unit.""" - assert seconds(f"{value}{unit}") == pytest.approx(value * UNITS[unit]), ( + """A duration is its number times the length of its unit. + + Compared exactly rather than approximately. Both sides are exact + now, so a tolerance would only be able to hide a disagreement: a + millisecond's length is a thousandth, which no float holds, and + ``pytest.approx`` would accept a reader that had rounded it. + """ + assert seconds(f"{value}{unit}") == value * UNITS[unit], ( f"{value}{unit} must scale by the length of its unit" ) diff --git a/tests/workflow_contracts/nextest_durations.py b/tests/workflow_contracts/nextest_durations.py index 4873a565e..d85987621 100644 --- a/tests/workflow_contracts/nextest_durations.py +++ b/tests/workflow_contracts/nextest_durations.py @@ -35,6 +35,7 @@ ``docs/developers-guide.md`` sets out all three with their inputs. """ +import fractions import re import string import typing as typ @@ -233,7 +234,7 @@ def _add_fraction(duration: str, total: _Total, matched: str, unit: _Unit) -> No >>> total = _Total() >>> _add_fraction("1.5m", total, "5", _UNITS["m"]) >>> total.as_seconds() - 30.0 + Fraction(30, 1) A thousandth of an hour does not, because the division there is over whole seconds and 3.6 is not one: @@ -266,8 +267,8 @@ def _add_fraction(duration: str, total: _Total, matched: str, unit: _Unit) -> No _add_landed(duration, total, scaled // denominator, scaling) -def seconds(duration: str) -> float: - """Convert a nextest duration to seconds. +def seconds(duration: str) -> fractions.Fraction: + """Convert a nextest duration to seconds, exactly. Parameters ---------- @@ -276,8 +277,12 @@ def seconds(duration: str) -> float: Returns ------- - float - The duration in seconds. + fractions.Fraction + The duration in seconds, exactly. Exact because these values + are compared with one another: see :meth:`nextest_totals.Total. + as_seconds` for the two magnitudes at which a float stops + telling two budgets apart. Use :func:`display_seconds` when the + number is going into a message rather than into a comparison. Raises ------ @@ -287,14 +292,14 @@ def seconds(duration: str) -> float: Examples -------- >>> seconds("45m") - 2700.0 + Fraction(2700, 1) >>> seconds("2h 30m") - 9000.0 + Fraction(9000, 1) >>> seconds("0.5s 0.5s") - 1.0 + Fraction(1, 1) """ if duration == _BARE_ZERO: - return 0.0 + return fractions.Fraction(0) text = duration.strip(_SPACE_CHARS) if not text: message = f"unrecognized nextest duration {duration!r}: it is empty" @@ -306,3 +311,28 @@ def seconds(duration: str) -> float: _read_pair(duration, match, total) position = match.end() return total.as_seconds() + + +def display_seconds(duration: str) -> float: + """Convert a duration to seconds as a float, for a message. + + Lossy on purpose, and named apart from :func:`seconds` on purpose. + A float is what a reader wants to see in an assertion message; it + is not what a comparison should be made on, because above 2**53 + seconds it cannot tell two budgets a second apart apart. Keeping + the two behind different names means a caller chooses which it + wants rather than getting the lossy one by default. + + Returns + ------- + float + The duration in seconds, rounded to what a float can hold. + + Examples + -------- + >>> display_seconds("45m") + 2700.0 + >>> display_seconds("1.5h") + 5400.0 + """ + return float(seconds(duration)) diff --git a/tests/workflow_contracts/nextest_totals.py b/tests/workflow_contracts/nextest_totals.py index 2ec957bde..bdd9a4e28 100644 --- a/tests/workflow_contracts/nextest_totals.py +++ b/tests/workflow_contracts/nextest_totals.py @@ -14,6 +14,7 @@ still imports them from there. """ +import fractions import typing as typ from nextest_units import SECOND as _SECOND @@ -95,6 +96,8 @@ class Total: >>> total.add("1s 1s", 1, 0) >>> total.add("1s 1s", 1, 0) >>> total.as_seconds() + Fraction(2, 1) + >>> float(total.as_seconds()) 2.0 """ @@ -140,12 +143,29 @@ def add(self, duration: str, seconds: int, nanoseconds: int) -> None: self.seconds = running self.nanoseconds = nanos - def as_seconds(self) -> float: - """Return the total in seconds. + def as_seconds(self) -> fractions.Fraction: + """Return the total in seconds, exactly. + + A ``Fraction`` rather than a ``float`` because these values are + compared with each other rather than merely printed. humantime's + range reaches 2**64 seconds and a float carries 53 bits of + significand, so above 2**53 it cannot hold two budgets that + differ by a second: ``18446744073709551614s`` and + ``18446744073709551615s`` are both inputs in the differential + this reader is measured against, and both convert to the same + float. An ordering assertion between them would compare equal + and pass whichever way round it was written. + + The nanosecond part makes the same point at the other end. + ``0.1s`` has no exact float, so a budget assembled from tenths + and one written as a decimal would differ by a rounding error + rather than by anything anyone configured. Returns ------- - float - Whole seconds and the nanosecond part together. + fractions.Fraction + Whole seconds and the nanosecond part together, exactly. """ - return self.seconds + self.nanoseconds / _SECOND + return fractions.Fraction(self.seconds) + fractions.Fraction( + self.nanoseconds, _SECOND + ) diff --git a/tests/workflow_contracts/timeout_budget_properties_test.py b/tests/workflow_contracts/timeout_budget_properties_test.py index 003d6b431..28dda745d 100644 --- a/tests/workflow_contracts/timeout_budget_properties_test.py +++ b/tests/workflow_contracts/timeout_budget_properties_test.py @@ -12,6 +12,7 @@ never applies, and they would pass. """ +import fractions import typing as typ import pytest @@ -33,7 +34,12 @@ ) #: The units nextest accepts, with their length in seconds. -UNITS: typ.Final[dict[str, float]] = {"ms": 0.001, "s": 1.0, "m": 60.0, "h": 3600.0} +UNITS: typ.Final[dict[str, fractions.Fraction]] = { + "ms": fractions.Fraction(1, 1_000), + "s": fractions.Fraction(1), + "m": fractions.Fraction(60), + "h": fractions.Fraction(3_600), +} COVERAGE_STEP: typ.Final[str] = ( "leynos/shared-actions/.github/actions/generate-coverage@abc123" @@ -116,7 +122,10 @@ def test_the_largest_budget_is_the_largest_product( ) ) expected = max(value * UNITS[unit] * times for value, unit, times in budgets) - assert largest_test_allowance(config) == pytest.approx(expected), ( + # Exact, not approximate: every term here is exact, and a tolerance + # would accept a reading that had lost a millisecond's thousandth + # to a float on the way through. + assert largest_test_allowance(config) == expected, ( "the largest budget is the largest period times its own multiplier" ) @@ -239,11 +248,11 @@ def test_the_termination_allowance_tracks_the_largest_grace_period( ) ) largest = max(value * UNITS[unit] for value, unit in periods) - assert grace_period(config) == pytest.approx(largest), ( + assert grace_period(config) == largest, ( "the largest configured grace period governs" ) - assert termination_allowance(config) == pytest.approx( - largest + TERMINATION_SAFETY_MARGIN_SECONDS + assert ( + termination_allowance(config) == largest + TERMINATION_SAFETY_MARGIN_SECONDS ), "the allowance is the grace period plus the margin, not the larger" @@ -262,9 +271,9 @@ def test_an_unconfigured_grace_period_falls_back_to_nextest_s_default( ), profile=profile, ) - assert grace_period(config) == pytest.approx( - NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS - ), "an absent grace period must fall back to nextest's default" + assert grace_period(config) == NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS, ( + "an absent grace period must fall back to nextest's default" + ) @pytest.mark.parametrize( diff --git a/tests/workflow_contracts/timeout_budgets.py b/tests/workflow_contracts/timeout_budgets.py index 325e9abdc..e5c9d8673 100644 --- a/tests/workflow_contracts/timeout_budgets.py +++ b/tests/workflow_contracts/timeout_budgets.py @@ -11,8 +11,12 @@ ``docs/developers-guide.md``. """ +import fractions import typing as typ +if typ.TYPE_CHECKING: + import collections.abc as cabc + from workflow_loading import REPO_ROOT #: The environment variable the shared coverage action reads for its @@ -37,6 +41,14 @@ #: in `docs/developers-guide.md`. CAPPED_PROFILE: typ.Final[str] = "ci" +# Every constant below is a term of a tier comparison, so each is an +# exact `Fraction` rather than a `float`. A sum is exact only if every +# term is, and one float among them converts the whole sum back +# silently: at the magnitudes humantime admits, that turns an ordering +# between two budgets a second apart into a comparison of equals. The +# values themselves are whole numbers of seconds and always will be; +# the type is about what they are added to, not about what they are. + #: Everything in a coverage job that is not the `cargo` invocation the #: watchdog bounds: checkout, toolchain setup, cache restore, linting, and #: whatever follows the coverage step. The job timer covers it; the @@ -49,7 +61,9 @@ #: of `coverage-main.yml` it was 119 s on run 33411190301. Fifteen minutes #: covers the worse of those with 252 s to spare, and none of those runs #: was genuinely cold. -OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS: typ.Final[float] = 15 * 60.0 +OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS: typ.Final[fractions.Fraction] = fractions.Fraction( + 15 * 60 +) #: Build time inside the `cargo` invocation, before nextest starts its own #: clock. The watchdog covers it as well as the whole-run budget, so the @@ -61,23 +75,57 @@ #: and every one of those runs recompiled the dependency graph from #: scratch. All of them read a warm sccache, so ten minutes is the #: allowance for the case none of them measured, an empty one. -COLD_BUILD_ALLOWANCE_SECONDS: typ.Final[float] = 10 * 60.0 +COLD_BUILD_ALLOWANCE_SECONDS: typ.Final[fractions.Fraction] = fractions.Fraction( + 10 * 60 +) #: How far a ceiling must sit above the sum it contains, rather than #: merely reaching it. A ceiling equal to that sum cancels the job at #: the moment the watchdog would have reported the overrun, and the #: report is the only thing that makes an overrun actionable. -CEILING_MARGIN_SECONDS: typ.Final[float] = 15 * 60.0 +CEILING_MARGIN_SECONDS: typ.Final[fractions.Fraction] = fractions.Fraction(15 * 60) #: What nextest allows a test between `SIGTERM` and `SIGKILL` when the #: configuration names no `grace-period`, as this one does not. -NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS: typ.Final[float] = 10.0 +NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS: typ.Final[fractions.Fraction] = ( + fractions.Fraction(10) +) #: Added to that grace period to cover the teardown and report writing #: that follow it. A separate term rather than a floor over the two, so #: raising a grace period raises the requirement instead of vanishing #: into it. -TERMINATION_SAFETY_MARGIN_SECONDS: typ.Final[float] = 60.0 +TERMINATION_SAFETY_MARGIN_SECONDS: typ.Final[fractions.Fraction] = fractions.Fraction( + 60 +) NEXTEST_CONFIG = REPO_ROOT / ".config" / "nextest.toml" WORKFLOWS_DIRECTORY = REPO_ROOT / ".github" / "workflows" + + +def required_ceiling( + budgets: cabc.Sequence[fractions.Fraction], +) -> fractions.Fraction: + """Return the smallest acceptable ceiling for one job, in seconds. + + Three terms. Each coverage step may legitimately spend its whole + watchdog, so the sum is the floor. The measured work outside those + windows is added because the job timer covers it and the watchdogs + do not. The margin is added because a ceiling equal to that sum + cancels the job at the moment the watchdog would have reported the + overrun, and the report is the only thing that makes an overrun + actionable. + + Parameters + ---------- + budgets : cabc.Sequence[fractions.Fraction] + One watchdog budget per coverage step in the job. + + Returns + ------- + fractions.Fraction + The smallest acceptable ceiling, in seconds, exactly. The sum + is exact only if every term is, which is why the two allowances + it adds are exact as well. + """ + return sum(budgets) + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS diff --git a/tests/workflow_contracts/timeout_exactness_test.py b/tests/workflow_contracts/timeout_exactness_test.py new file mode 100644 index 000000000..d8241c5bc --- /dev/null +++ b/tests/workflow_contracts/timeout_exactness_test.py @@ -0,0 +1,352 @@ +"""The tier comparisons stay exact all the way from the files they read. + +``nextest_duration_test`` proves that a nextest duration is read exactly. +That is only half of it. Every tier comparison is a sum, and each sum +mixes a duration read from ``.config/nextest.toml`` with a budget read +from a workflow and with a constant declared in ``timeout_budgets``. A +sum is exact only if every term is: one ``float`` among them converts the +whole of it back, and the conversion is silent. + +So this module drives the three compositions the ordering contract +actually evaluates, each with two inputs one second apart and large +enough that a float cannot hold both. The repository's own budgets are +nowhere near that magnitude and never will be, which is precisely why +the loss cannot be exposed by the real files: a contract resting on them +would pass with every term a float. + +Each case asserts the float collapse alongside the exact comparison. The +collapse is the thing being avoided rather than an incidental detail, and +a case that stopped exercising it would keep passing while proving +nothing. +""" + +import fractions +import typing as typ + +import pytest +from coverage_lanes import coverage_lanes_of +from nextest_budgets import global_timeout, termination_allowance +from nextest_durations import seconds +from timeout_budgets import ( + CEILING_MARGIN_SECONDS, + COLD_BUILD_ALLOWANCE_SECONDS, + COVERAGE_ACTION, + NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS, + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS, + TERMINATION_SAFETY_MARGIN_SECONDS, + WATCHDOG_VARIABLE, + required_ceiling, +) + +#: A magnitude whose neighbouring floats are 256 seconds apart, so a +#: one-second difference is lost outright rather than only sometimes. +#: Two to the fifty-third is the first magnitude that loses anything, but +#: there the spacing is two and whether a given pair collapses depends on +#: which side of a tie it falls; at two to the sixtieth every difference +#: below 128 seconds vanishes, which is what these cases need. +HUGE_SECONDS: typ.Final[int] = 2**60 + + +def _workflow(watchdog: int, timeout_minutes: int) -> dict[str, dict[str, object]]: + """Return one document declaring a single coverage job. + + Parameters + ---------- + watchdog : int + The watchdog budget in seconds, set at job level as both of this + repository's workflows set it. + timeout_minutes : int + The job's ``timeout-minutes``. + + Returns + ------- + dict[str, dict[str, object]] + A document map as ``coverage_lanes_of`` takes. + """ + return { + "ci.yml": { + "jobs": { + "coverage": { + "timeout-minutes": timeout_minutes, + "env": {WATCHDOG_VARIABLE: watchdog}, + "steps": [{"uses": f"{COVERAGE_ACTION}@" + "0" * 40}], + } + } + } + } + + +def _watchdog_read(watchdog: int) -> fractions.Fraction: + """Return the watchdog budget the lane reader takes from a document. + + Parameters + ---------- + watchdog : int + The value to declare. + + Returns + ------- + fractions.Fraction + What the reader made of it. + """ + (lane,) = coverage_lanes_of(_workflow(watchdog, timeout_minutes=65)) + assert lane.watchdog is not None, "the job declares a watchdog at job level" + return lane.watchdog + + +def _job_ceiling_read(timeout_minutes: int) -> fractions.Fraction: + """Return the job ceiling the lane reader takes from a document. + + Parameters + ---------- + timeout_minutes : int + The ``timeout-minutes`` to declare. + + Returns + ------- + fractions.Fraction + The ceiling in seconds. + """ + (lane,) = coverage_lanes_of(_workflow(1800, timeout_minutes=timeout_minutes)) + assert lane.job_timeout is not None, "the job declares timeout-minutes" + return lane.job_timeout + + +def _assert_orders_strictly( + larger: fractions.Fraction, smaller: fractions.Fraction, what: str +) -> None: + """Assert an exact ordering, and that a float would have lost it. + + The second assertion is what keeps the first honest. These cases + exist to catch a term reverting to a float, so a case whose two + inputs stopped colliding as floats would pass without exercising + anything, and would go on passing after the defect returned. + + Parameters + ---------- + larger : fractions.Fraction + The greater of the two readings. + smaller : fractions.Fraction + The lesser. + what : str + What is being ordered, for the message. + """ + assert larger > smaller, f"{what} must order strictly" + # RUF069 is right in general and wrong here: comparing two floats + # for equality is the assertion, not an oversight. These two must + # collide as floats or the case above is not exercising the loss it + # was written to catch. + assert float(larger) == float(smaller), ( # ruff: ignore[float-equality-comparison] + f"the float collapse this guards against must still be real for " + f"{what}; if these differ, the case no longer exercises what it was " + f"written for" + ) + + +def test_the_duration_reader_keeps_a_second_a_float_would_lose() -> None: + """Two budgets one second apart must not read as one value. + + The first term of every composition below is a duration out of + ``.config/nextest.toml``. Both of these are inputs in the estate + differential, so this is not a magnitude invented for the contract. + """ + _assert_orders_strictly( + seconds(f"{HUGE_SECONDS + 1}s"), + seconds(f"{HUGE_SECONDS}s"), + "durations one second apart", + ) + + +def test_the_lane_reader_keeps_a_second_a_float_would_lose() -> None: + """Two watchdog budgets one second apart must not read as one value. + + The workflow side was read with ``float(text)``, so the exactness + won on the nextest side would be discarded the moment a watchdog + entered the comparison. Both the job ceiling and the watchdog floor + compare against a workflow value, so both inherit it. + """ + _assert_orders_strictly( + _watchdog_read(HUGE_SECONDS + 1), + _watchdog_read(HUGE_SECONDS), + "watchdogs one second apart", + ) + + +def test_the_job_ceiling_reader_keeps_a_minute_a_float_would_lose() -> None: + """Two ceilings one minute apart must not read as one value. + + The ceiling is converted as well as read: ``timeout-minutes`` is + multiplied by sixty. Doing either in floating point loses the + comparison that holds a job above the sum of its watchdogs, so a + ceiling a minute short of that sum would be accepted. + """ + _assert_orders_strictly( + _job_ceiling_read(HUGE_SECONDS + 1), + _job_ceiling_read(HUGE_SECONDS), + "ceilings one minute apart", + ) + + +def test_the_job_ceiling_arithmetic_stays_exact() -> None: + """The first composition: the watchdogs, the outside work, the margin. + + ``required_ceiling`` adds a job's watchdog budgets, the measured work + outside them and the margin above that sum. Either allowance being a + float makes the total a float whatever the budgets are, and the + comparison that reads it would then accept a job ceiling a second + short of containing its own watchdogs. + """ + _assert_orders_strictly( + required_ceiling([fractions.Fraction(HUGE_SECONDS + 1)]), + required_ceiling([fractions.Fraction(HUGE_SECONDS)]), + "required ceilings one second apart", + ) + + +def _config(whole_run: int, *, grace: bool = True) -> str: + """Return a configuration declaring one profile's tiers. + + Parameters + ---------- + whole_run : int + The ``global-timeout`` in seconds. + grace : bool + Whether the profile declares a ``grace-period``. Without one + nextest's own default applies, and that default is a term of the + allowance like any other, so both spellings are driven. + + Returns + ------- + str + The configuration text. + """ + declared = ', grace-period = "5s"' if grace else "" + return ( + "[profile.default]\n" + f'global-timeout = "{whole_run}s"\n' + f'slow-timeout = {{ period = "60s", terminate-after = 1{declared} }}\n' + ) + + +def test_the_termination_allowance_stays_exact() -> None: + """The second composition: a grace period plus the safety margin. + + Two terms, and the margin is the one that was a float. The grace + period is the term that moves here, so the case fails if either the + duration reaching it or the margin added to it stops being exact. + + Nextest's own default is the other spelling of the first term, and + it cannot be driven by an ordering because nothing about it varies. + It is a term of the watchdog floor below, where the whole-run budget + supplies the movement, and it is named in + ``test_every_constant_the_compositions_add_is_exact``. + """ + larger = _config(1, grace=False).replace( + "terminate-after = 1", + f'terminate-after = 1, grace-period = "{HUGE_SECONDS + 1}s"', + ) + smaller = _config(1, grace=False).replace( + "terminate-after = 1", f'terminate-after = 1, grace-period = "{HUGE_SECONDS}s"' + ) + _assert_orders_strictly( + termination_allowance(larger), + termination_allowance(smaller), + "termination allowances one second apart", + ) + + +def _watchdog_floor(whole_run: int, *, grace: bool = True) -> fractions.Fraction: + """Return the watchdog floor the ordering contract derives. + + This is the expression ``whole_run_ordering.watchdog_required_for`` + evaluates, rebuilt here from its parts so that a term reverting to a + float fails against the composition rather than against a helper. + + Parameters + ---------- + whole_run : int + The whole-run budget in seconds. + grace : bool + Whether the profile declares a ``grace-period``. + + Returns + ------- + fractions.Fraction + The smallest watchdog covering the whole run, nextest's + termination procedure and a cold build. + """ + config_text = _config(whole_run, grace=grace) + budget = global_timeout(config_text, profile="default") + assert budget is not None, "the profile declares a global-timeout" + return budget + termination_allowance(config_text) + COLD_BUILD_ALLOWANCE_SECONDS + + +@pytest.mark.parametrize( + "grace", + [ + pytest.param(True, id="a-declared-grace-period"), + pytest.param(False, id="nextests-own-default"), + ], +) +def test_the_watchdog_floor_stays_exact(*, grace: bool) -> None: + """The third composition: the whole run, the allowance, the cold build. + + Three terms, two of them constants. Either constant being a float + makes the floor a float even though the whole-run budget reaching it + is exact, and a watchdog a second below the run it must cover would + then compare equal to one that covers it. + """ + _assert_orders_strictly( + _watchdog_floor(HUGE_SECONDS + 1, grace=grace), + _watchdog_floor(HUGE_SECONDS, grace=grace), + f"watchdog floors one second apart (grace-period declared: {grace})", + ) + + +def test_the_configured_values_arrive_exact() -> None: + """The real files read exactly too, not merely the constructed ones. + + The cases above use magnitudes this repository will never configure, + which is what makes them able to see the loss at all. This one is the + other half: the values actually in force arrive as exact numbers, so + a float reintroduced anywhere on the live path is caught without + waiting for a budget nobody will ever set. + """ + lanes = coverage_lanes_of() + assert lanes, "the repository declares at least one coverage lane" + assert isinstance(seconds("600s"), fractions.Fraction), ( + "a nextest duration must arrive exact" + ) + assert all( + lane.watchdog is None or isinstance(lane.watchdog, fractions.Fraction) + for lane in lanes + ), "every workflow watchdog must arrive exact" + assert all( + lane.job_timeout is None or isinstance(lane.job_timeout, fractions.Fraction) + for lane in lanes + ), "every job ceiling must arrive exact through its conversion to seconds" + assert isinstance( + required_ceiling([fractions.Fraction(1800)]), fractions.Fraction + ), "the required ceiling must stay exact through its sum" + + +def test_every_constant_the_compositions_add_is_exact() -> None: + """Each constant is a term, so each is exact in its own right. + + Named one by one rather than asserted over a collection, so a + constant reverting to a float fails with its own name in the report + rather than as a count. + """ + for name, value in ( + ("OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS", OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS), + ("CEILING_MARGIN_SECONDS", CEILING_MARGIN_SECONDS), + ("COLD_BUILD_ALLOWANCE_SECONDS", COLD_BUILD_ALLOWANCE_SECONDS), + ("TERMINATION_SAFETY_MARGIN_SECONDS", TERMINATION_SAFETY_MARGIN_SECONDS), + ( + "NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS", + NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS, + ), + ): + assert isinstance(value, fractions.Fraction), ( + f"{name} is a term of a tier comparison and must be exact" + ) diff --git a/tests/workflow_contracts/timeout_ordering_test.py b/tests/workflow_contracts/timeout_ordering_test.py index d8722e539..2293877fe 100644 --- a/tests/workflow_contracts/timeout_ordering_test.py +++ b/tests/workflow_contracts/timeout_ordering_test.py @@ -54,13 +54,10 @@ OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS, TERMINATION_SAFETY_MARGIN_SECONDS, WATCHDOG_VARIABLE, + required_ceiling, ) from whole_run_ordering import whole_run_ordering_faults -if typ.TYPE_CHECKING: - import collections.abc as cabc - - #: The condition each coverage lane legitimately carries, keyed by #: workflow and job, as the step's ``if`` and its job's. #: @@ -171,30 +168,6 @@ def _budgets_per_job( return grouped -def required_ceiling(budgets: cabc.Sequence[float]) -> float: - """Return the smallest acceptable ceiling for one job, in seconds. - - Three terms. Each coverage step may legitimately spend its whole - watchdog, so the sum is the floor. The measured work outside those - windows is added because the job timer covers it and the watchdogs - do not. The margin is added because a ceiling equal to that sum - cancels the job at the moment the watchdog would have reported the - overrun, and the report is the only thing that makes an overrun - actionable. - - Parameters - ---------- - budgets : cabc.Sequence[float] - One watchdog budget per coverage step in the job. - - Returns - ------- - float - The smallest acceptable ceiling, in seconds. - """ - return sum(budgets) + OUTSIDE_WATCHDOG_ALLOWANCE_SECONDS + CEILING_MARGIN_SECONDS - - def test_the_job_ceiling_covers_every_watchdog_and_the_work_around_them( coverage_lanes: tuple[CoverageLane, ...], ) -> None: diff --git a/tests/workflow_contracts/whole_run_ordering.py b/tests/workflow_contracts/whole_run_ordering.py index 4a3ee1b97..e2a6f63cc 100644 --- a/tests/workflow_contracts/whole_run_ordering.py +++ b/tests/workflow_contracts/whole_run_ordering.py @@ -21,11 +21,12 @@ if typ.TYPE_CHECKING: import collections.abc as cabc + import fractions from coverage_lanes import CoverageLane -def watchdog_required_for(config_text: str) -> float | None: +def watchdog_required_for(config_text: str) -> fractions.Fraction | None: """Return the watchdog a configured whole-run budget demands, or None.""" # Three terms. The whole-run budget is what nextest may spend once # tests begin; the termination allowance is what it may spend @@ -55,7 +56,7 @@ def whole_run_ordering_faults( return faults -def _per_test_faults(config_text: str, whole_run: float) -> list[str]: +def _per_test_faults(config_text: str, whole_run: fractions.Fraction) -> list[str]: """Return the fault, if any, in the whole run against one test.""" largest = largest_test_allowance(config_text) if whole_run > largest: @@ -69,7 +70,9 @@ def _per_test_faults(config_text: str, whole_run: float) -> list[str]: def _lane_faults( - lanes: cabc.Iterable[CoverageLane], whole_run: float, required: float + lanes: cabc.Iterable[CoverageLane], + whole_run: fractions.Fraction, + required: fractions.Fraction, ) -> list[str]: """Return one fault per lane whose watchdog cannot cover the run.""" faults = [] diff --git a/tests/workflow_contracts/whole_run_ordering_test.py b/tests/workflow_contracts/whole_run_ordering_test.py index 9ce429ce4..a9c4c704b 100644 --- a/tests/workflow_contracts/whole_run_ordering_test.py +++ b/tests/workflow_contracts/whole_run_ordering_test.py @@ -13,6 +13,7 @@ Run via ``make test-workflow-contracts``. """ +import fractions import typing as typ import pytest @@ -52,14 +53,14 @@ def _config(global_timeout: str | None = None) -> str: ) -def _lane(watchdog: float | None) -> CoverageLane: +def _lane(watchdog: fractions.Fraction | None) -> CoverageLane: """Return one coverage lane carrying a watchdog and nothing else.""" return CoverageLane( workflow="ci.yml", job="build-test", step="Test and Measure Coverage", watchdog=watchdog, - job_timeout=100 * 60.0, + job_timeout=fractions.Fraction(100 * 60), ) @@ -71,7 +72,9 @@ def test_a_configuration_with_no_whole_run_budget_has_no_fault() -> None: reports that: ``timeout_ordering_test`` asserts the key's presence separately, and this repository's own file now sets one. """ - assert not whole_run_ordering_faults(_config(), [_lane(4200.0)]), ( + assert not whole_run_ordering_faults( + _config(), [_lane(fractions.Fraction(4200))] + ), ( "a configuration setting no global-timeout has no tier three, so the " "rule must report nothing rather than fail an incomplete file" ) @@ -102,7 +105,9 @@ def test_a_whole_run_below_the_per_test_allowance_is_a_fault() -> None: makes the per-test tier unreachable while every value in the file still reads as deliberate. """ - faults = whole_run_ordering_faults(_config("300s"), [_lane(100_000.0)]) + faults = whole_run_ordering_faults( + _config("300s"), [_lane(fractions.Fraction(100_000))] + ) assert len(faults) == 1, faults assert "largest per-test allowance" in faults[0], ( @@ -116,7 +121,9 @@ def test_a_whole_run_equal_to_the_per_test_allowance_is_a_fault() -> None: A rule written with `>=` passes this case and fails nothing else, so the strictness of the comparison is stated rather than implied. """ - faults = whole_run_ordering_faults(_config("600s"), [_lane(100_000.0)]) + faults = whole_run_ordering_faults( + _config("600s"), [_lane(fractions.Fraction(100_000))] + ) assert len(faults) == 1, faults assert "largest per-test allowance" in faults[0], ( @@ -135,7 +142,7 @@ def test_a_watchdog_below_the_requirement_is_a_fault() -> None: required = watchdog_required_for(config) assert required is not None, "a configured global-timeout yields a requirement" - faults = whole_run_ordering_faults(config, [_lane(required - 1.0)]) + faults = whole_run_ordering_faults(config, [_lane(required - 1)]) assert len(faults) == 1, faults assert "below the" in faults[0], ( @@ -180,7 +187,9 @@ def test_every_lane_at_fault_is_reported() -> None: """ config = _config("40m") - faults = whole_run_ordering_faults(config, [_lane(1.0), _lane(2.0)]) + faults = whole_run_ordering_faults( + config, [_lane(fractions.Fraction(1)), _lane(fractions.Fraction(2))] + ) assert len(faults) == 2, faults From 4f5866d93af2327972f60b36a8168f045ce080eb Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Thu, 17 Sep 2026 12:50:53 +0100 Subject: [PATCH 2/3] Document the public float, and shorten the private summaries Two of the review's findings, taken as written. `display_seconds` is a public interface, so its parameter gets a NumPy-style section. It names no `Raises`: it raises nothing itself, which the docstring linter enforces, so the delegation to `seconds` is stated in the return description instead. The six private helpers in the exactness contract take single-line summaries, as the path instructions require. The reasoning that was in them is kept as comments rather than dropped: why the watchdog sits at job level, why the collapse assertion keeps the ordering assertion honest, why both spellings of the grace period are driven, and that the floor is the ordering contract's own expression rebuilt from its parts. `nextest_budgets` collapses its three-name import onto one line to stay inside the 400-line limit, which the exact signatures had taken it past. --- tests/workflow_contracts/nextest_budgets.py | 13 +-- tests/workflow_contracts/nextest_durations.py | 9 +- .../timeout_exactness_test.py | 108 +++--------------- 3 files changed, 30 insertions(+), 100 deletions(-) diff --git a/tests/workflow_contracts/nextest_budgets.py b/tests/workflow_contracts/nextest_budgets.py index e5347d17c..7273c9c07 100644 --- a/tests/workflow_contracts/nextest_budgets.py +++ b/tests/workflow_contracts/nextest_budgets.py @@ -21,11 +21,7 @@ import typing as typ from itertools import starmap -from nextest_durations import ( - NextestConfigurationError, - UnboundedTestError, - seconds, -) +from nextest_durations import NextestConfigurationError, UnboundedTestError, seconds from timeout_budgets import ( CAPPED_PROFILE, NEXTEST_DEFAULT_GRACE_PERIOD_SECONDS, @@ -335,7 +331,8 @@ def termination_allowance(config_text: str) -> fractions.Fraction: the second is a fixed margin for the teardown and report writing that follow it. A single floor over the two would absorb every grace period below the margin, so raising one would look free until the - run it cancelled. + run it cancelled. Both terms are exact so that the sum is: a float + in either would convert the whole of it back, silently. Parameters ---------- @@ -345,9 +342,7 @@ def termination_allowance(config_text: str) -> fractions.Fraction: Returns ------- fractions.Fraction - The grace period plus the safety margin, exactly. Both terms - are exact so that the sum is: a float in either would convert - the whole of it back silently. + The grace period plus the safety margin, exactly. """ return grace_period(config_text) + TERMINATION_SAFETY_MARGIN_SECONDS diff --git a/tests/workflow_contracts/nextest_durations.py b/tests/workflow_contracts/nextest_durations.py index d85987621..3c7f0821d 100644 --- a/tests/workflow_contracts/nextest_durations.py +++ b/tests/workflow_contracts/nextest_durations.py @@ -323,10 +323,17 @@ def display_seconds(duration: str) -> float: the two behind different names means a caller chooses which it wants rather than getting the lossy one by default. + Parameters + ---------- + duration : str + A duration as nextest spells it, such as ``"60s"``. + Returns ------- float - The duration in seconds, rounded to what a float can hold. + The duration in seconds, rounded to what a float can hold. The + text is read by :func:`seconds`, so a duration nextest would + refuse is refused here in the same way. Examples -------- diff --git a/tests/workflow_contracts/timeout_exactness_test.py b/tests/workflow_contracts/timeout_exactness_test.py index d8241c5bc..753e3b1ba 100644 --- a/tests/workflow_contracts/timeout_exactness_test.py +++ b/tests/workflow_contracts/timeout_exactness_test.py @@ -48,21 +48,9 @@ def _workflow(watchdog: int, timeout_minutes: int) -> dict[str, dict[str, object]]: - """Return one document declaring a single coverage job. - - Parameters - ---------- - watchdog : int - The watchdog budget in seconds, set at job level as both of this - repository's workflows set it. - timeout_minutes : int - The job's ``timeout-minutes``. - - Returns - ------- - dict[str, dict[str, object]] - A document map as ``coverage_lanes_of`` takes. - """ + """Return one document declaring a single coverage job.""" + # The watchdog sits at job level because both of this repository's + # workflows declare it there. return { "ci.yml": { "jobs": { @@ -77,36 +65,14 @@ def _workflow(watchdog: int, timeout_minutes: int) -> dict[str, dict[str, object def _watchdog_read(watchdog: int) -> fractions.Fraction: - """Return the watchdog budget the lane reader takes from a document. - - Parameters - ---------- - watchdog : int - The value to declare. - - Returns - ------- - fractions.Fraction - What the reader made of it. - """ + """Return the watchdog budget the lane reader takes from a document.""" (lane,) = coverage_lanes_of(_workflow(watchdog, timeout_minutes=65)) assert lane.watchdog is not None, "the job declares a watchdog at job level" return lane.watchdog def _job_ceiling_read(timeout_minutes: int) -> fractions.Fraction: - """Return the job ceiling the lane reader takes from a document. - - Parameters - ---------- - timeout_minutes : int - The ``timeout-minutes`` to declare. - - Returns - ------- - fractions.Fraction - The ceiling in seconds. - """ + """Return the job ceiling in seconds, converted from its minutes.""" (lane,) = coverage_lanes_of(_workflow(1800, timeout_minutes=timeout_minutes)) assert lane.job_timeout is not None, "the job declares timeout-minutes" return lane.job_timeout @@ -115,22 +81,11 @@ def _job_ceiling_read(timeout_minutes: int) -> fractions.Fraction: def _assert_orders_strictly( larger: fractions.Fraction, smaller: fractions.Fraction, what: str ) -> None: - """Assert an exact ordering, and that a float would have lost it. - - The second assertion is what keeps the first honest. These cases - exist to catch a term reverting to a float, so a case whose two - inputs stopped colliding as floats would pass without exercising - anything, and would go on passing after the defect returned. - - Parameters - ---------- - larger : fractions.Fraction - The greater of the two readings. - smaller : fractions.Fraction - The lesser. - what : str - What is being ordered, for the message. - """ + """Assert an exact ordering, and that a float would have lost it.""" + # The second assertion keeps the first honest. These cases exist to + # catch a term reverting to a float, so one whose two inputs stopped + # colliding as floats would pass without exercising anything, and + # would go on passing after the defect returned. assert larger > smaller, f"{what} must order strictly" # RUF069 is right in general and wrong here: comparing two floats # for equality is the assertion, not an oversight. These two must @@ -204,22 +159,10 @@ def test_the_job_ceiling_arithmetic_stays_exact() -> None: def _config(whole_run: int, *, grace: bool = True) -> str: - """Return a configuration declaring one profile's tiers. - - Parameters - ---------- - whole_run : int - The ``global-timeout`` in seconds. - grace : bool - Whether the profile declares a ``grace-period``. Without one - nextest's own default applies, and that default is a term of the - allowance like any other, so both spellings are driven. - - Returns - ------- - str - The configuration text. - """ + """Return a configuration declaring one profile's tiers.""" + # Both spellings of the grace period are driven: without a declared + # one nextest's default applies, and that default is a term of the + # allowance like any other. declared = ', grace-period = "5s"' if grace else "" return ( "[profile.default]\n" @@ -256,25 +199,10 @@ def test_the_termination_allowance_stays_exact() -> None: def _watchdog_floor(whole_run: int, *, grace: bool = True) -> fractions.Fraction: - """Return the watchdog floor the ordering contract derives. - - This is the expression ``whole_run_ordering.watchdog_required_for`` - evaluates, rebuilt here from its parts so that a term reverting to a - float fails against the composition rather than against a helper. - - Parameters - ---------- - whole_run : int - The whole-run budget in seconds. - grace : bool - Whether the profile declares a ``grace-period``. - - Returns - ------- - fractions.Fraction - The smallest watchdog covering the whole run, nextest's - termination procedure and a cold build. - """ + """Return the watchdog floor the ordering contract derives.""" + # The expression `whole_run_ordering.watchdog_required_for` + # evaluates, rebuilt from its parts so that a term reverting to a + # float fails against the composition rather than against a helper. config_text = _config(whole_run, grace=grace) budget = global_timeout(config_text, profile="default") assert budget is not None, "the profile declares a global-timeout" From 4c81243a163259ea6fcd5cc7afd1c78a0fc41fbe Mon Sep 17 00:00:00 2001 From: Payton McIntosh Date: Fri, 18 Sep 2026 14:58:49 +0100 Subject: [PATCH 3/3] Drive the watchdog floor through the function the contract calls `timeout_exactness_test` rebuilt the watchdog floor from its three terms rather than calling `watchdog_required_for`. Rebuilding is worth keeping: it names which term lost its exactness rather than reporting the sum. But it left the function the ordering contract actually calls unexercised at a magnitude where a float can be seen at all, so a cast applied inside it, to its result or to a term as it was added, passed every case in the module. Proved by mutation. Wrapping the function's return in `Fraction(float(...))` fails the new case alone; all 576 others, including the rebuilt composition for both spellings of the grace period, stayed green. The new case exposed a second thing. `_config` declared `[profile.default]` while `global_timeout` defaults to the profile CI selects, so a case driving the function through that configuration finds no budget and the function returns None. The rebuilt helper never met this because it passes `profile="default"` explicitly. The profile is now a parameter, defaulting to `default` so the existing cases are unchanged, and the new case declares the capped profile. Refs #727. --- .../timeout_exactness_test.py | 75 +++++++++++++++++-- 1 file changed, 68 insertions(+), 7 deletions(-) diff --git a/tests/workflow_contracts/timeout_exactness_test.py b/tests/workflow_contracts/timeout_exactness_test.py index 753e3b1ba..3645b7156 100644 --- a/tests/workflow_contracts/timeout_exactness_test.py +++ b/tests/workflow_contracts/timeout_exactness_test.py @@ -28,6 +28,7 @@ from nextest_budgets import global_timeout, termination_allowance from nextest_durations import seconds from timeout_budgets import ( + CAPPED_PROFILE, CEILING_MARGIN_SECONDS, COLD_BUILD_ALLOWANCE_SECONDS, COVERAGE_ACTION, @@ -37,6 +38,7 @@ WATCHDOG_VARIABLE, required_ceiling, ) +from whole_run_ordering import watchdog_required_for #: A magnitude whose neighbouring floats are 256 seconds apart, so a #: one-second difference is lost outright rather than only sometimes. @@ -158,14 +160,26 @@ def test_the_job_ceiling_arithmetic_stays_exact() -> None: ) -def _config(whole_run: int, *, grace: bool = True) -> str: - """Return a configuration declaring one profile's tiers.""" +def _config(whole_run: int, *, grace: bool = True, profile: str = "default") -> str: + """Return a configuration declaring one profile's tiers. + + The profile is a parameter because the readers disagree about which + one they mean. ``global_timeout`` defaults to the profile CI selects, + while a caller naming ``default`` is reading the table nextest + inherits from. A case driving the one through a configuration + declaring the other finds no budget at all. + + Returns + ------- + str + The configuration text, declaring the named profile alone. + """ # Both spellings of the grace period are driven: without a declared # one nextest's default applies, and that default is a term of the # allowance like any other. declared = ', grace-period = "5s"' if grace else "" return ( - "[profile.default]\n" + f"[profile.{profile}]\n" f'global-timeout = "{whole_run}s"\n' f'slow-timeout = {{ period = "60s", terminate-after = 1{declared} }}\n' ) @@ -199,16 +213,63 @@ def test_the_termination_allowance_stays_exact() -> None: def _watchdog_floor(whole_run: int, *, grace: bool = True) -> fractions.Fraction: - """Return the watchdog floor the ordering contract derives.""" - # The expression `whole_run_ordering.watchdog_required_for` - # evaluates, rebuilt from its parts so that a term reverting to a - # float fails against the composition rather than against a helper. + """Return the watchdog floor, rebuilt from the terms it sums. + + Deliberately not a call to ``watchdog_required_for``. Rebuilding the + sum means a term reverting to a float fails against the term rather + than against the function, so the report names which input lost its + exactness. ``test_the_watchdog_floor_function_stays_exact`` drives + the function itself, which is what the ordering contract calls, and + is what catches a cast applied inside it rather than to one of its + inputs. + + Returns + ------- + fractions.Fraction + The whole-run budget, the termination allowance and the cold + build allowance, summed exactly. + """ config_text = _config(whole_run, grace=grace) budget = global_timeout(config_text, profile="default") assert budget is not None, "the profile declares a global-timeout" return budget + termination_allowance(config_text) + COLD_BUILD_ALLOWANCE_SECONDS +def _watchdog_floor_of(whole_run: int) -> fractions.Fraction: + """Return what ``watchdog_required_for`` derives for one configuration. + + The configuration declares the profile CI selects, because that is + the one the function reads. Declaring ``default`` instead yields no + budget and the function returns None, which is the shape of a case + that asserts nothing rather than one that fails. + + Returns + ------- + fractions.Fraction + The watchdog floor the function derives for that configuration. + """ + floor = watchdog_required_for(_config(whole_run, profile=CAPPED_PROFILE)) + assert floor is not None, "the profile declares a global-timeout" + return floor + + +def test_the_watchdog_floor_function_stays_exact() -> None: + """``watchdog_required_for`` itself keeps a second a float would lose. + + The composition cases below rebuild this sum from its terms, which + localises a lossy input but leaves the function the ordering + contract actually calls unexercised at a magnitude that can see the + loss. A ``float`` applied inside ``watchdog_required_for``, to its + result or to any term as it is added, would therefore pass every + other case here. This one refuses it. + """ + _assert_orders_strictly( + _watchdog_floor_of(HUGE_SECONDS + 1), + _watchdog_floor_of(HUGE_SECONDS), + "watchdog floors one second apart, through the public function", + ) + + @pytest.mark.parametrize( "grace", [