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..7273c9c07 100644 --- a/tests/workflow_contracts/nextest_budgets.py +++ b/tests/workflow_contracts/nextest_budgets.py @@ -18,19 +18,19 @@ """ import tomllib +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, 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 +176,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 +188,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 +229,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 +245,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 +294,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 +308,7 @@ def grace_period(config_text: str) -> float: Returns ------- - float + fractions.Fraction The largest configured grace period, or nextest's default. """ periods = [ @@ -320,7 +320,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 @@ -331,7 +331,8 @@ def termination_allowance(config_text: str) -> float: 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 ---------- @@ -340,13 +341,15 @@ 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. """ 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 +370,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..3c7f0821d 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,35 @@ 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. + + 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 + text is read by :func:`seconds`, so a duration nextest would + refuse is refused here in the same way. + + 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..3645b7156 --- /dev/null +++ b/tests/workflow_contracts/timeout_exactness_test.py @@ -0,0 +1,341 @@ +"""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 ( + CAPPED_PROFILE, + 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, +) +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. +#: 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.""" + # The watchdog sits at job level because both of this repository's + # workflows declare it there. + 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.""" + (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 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 + + +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 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 + # 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, 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 ( + f"[profile.{profile}]\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, 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", + [ + 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