[architect] refactor: remove unconsumed SLA config from tests/shared/timing.py (DEFAULT_SLA, SLA_STRICT) + pin the timings.jsonl reader contract - #796
Open
kubestellar-hive[bot] wants to merge 1 commit into
Conversation
…timing.py DEFAULT_SLA and SLA_STRICT were defined and never read. _scenario_sla_seconds resolves a threshold purely from an @sla_<n>s scenario tag, so DEFAULT_SLA could never apply, and record_end never branched on SLA_STRICT — setting TIMING_SLA_STRICT=1 to turn on SLA gating was a silent no-op. Both are removed along with the two tests that asserted their literal values without asserting that anything acted on them. No behaviour change: neither name had a reader anywhere in the repo. timings.jsonl itself is no longer orphaned as originally reported — the Summarise results step of .github/actions/gnome-e2e/action.yml reads it back and reports entries whose sla_violated is true. That coupling spans a Python module and a heredoc inside a composite action, so nothing enforced it. New tests/unit/test_timing_contract.py pins it: every key the summariser pulls off an entry must be written by record_end, and a module-level constant in timing.py must be consumed by timing.py, which blocks a second unconsumed knob. Refs #764 Signed-off-by: sec-check[bot] <sec-check[bot]@users.noreply.github.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Refactor
Cluster: the scenario-timing subsystem —
tests/shared/timing.py,tests/unit/test_timing.py, newtests/unit/test_timing_contract.py, and theSummarise resultsstep of.github/actions/gnome-e2e/action.yml(read-only; not modified).Refs #764 — this lands the structural half of that issue only. It does not decide whether the factory wants SLA gating, which is the product question #764 raised, so the issue stays open.
1. Remove two constants nothing reads
tests/shared/timing.pydefinedDEFAULT_SLAandSLA_STRICT; neither had a reader anywhere in the repo._scenario_sla_secondsresolves a threshold purely from an@sla_<n>sscenario tag and returnsNoneotherwise, so no scenario could ever pick up aDEFAULT_SLAentry.record_endnever branched onSLA_STRICT. SettingTIMING_SLA_STRICT=1in a workflow to "turn on" SLA gating was a silent no-op, and the resulting green run read as evidence the suite met its SLAs.Both are removed, along with the two tests that certified them —
test_sla_strict_env_parsingasserted only that the env var parsed, andtest_default_sla_thresholds_are_defined_for_future_useasserted only that the literal still equalled itself. Both passed regardless of whether anything acted on either value, so they reported the subsystem as covered while the enforcement it advertised was absent.No behaviour change. Nothing consumed either name, so nothing can observe their removal.
2.
timings.jsonlis no longer orphaned — so pin the contract instead#764 reported the file had no consumer. That has since changed and the issue is stale on this point:
.github/actions/gnome-e2e/action.yml(Summarise results) readsresults/timings.jsonland reports every entry whosesla_violatedis true and whosestatusis notskipped.That makes the key names in
record_enda real contract — but one that spans a Python module intests/sharedand apython3 - <<'PYEOF'heredoc inside a composite action. Nothing connected the two, so renaming a key would stop SLA reporting silently with every suite still green.New
tests/unit/test_timing_contract.py:test_summariser_reads_only_keys_record_end_writes— parses the action YAML, extracts everyentry.get("...")in the summariser, and asserts each key is actually written byrecord_end.test_sla_violation_is_reported_for_a_tagged_overrun/test_untagged_scenario_has_no_sla_and_never_violates— cover the flag the summariser filters on, in both directions.test_module_constants_are_consumed_by_the_module— AST-walkstiming.pyand fails on any public module-level constant that the module never loads. This is the general form of the bug being fixed, so a third unconsumed knob cannot come back.The module docstring now states the contract and records that a violation is reported, never enforced, so the code stops implying a strict mode it does not have.
Verification
Both new guards were confirmed to fail when the defect is reintroduced, not merely to pass today:
DEFAULT_SLA/SLA_STRICT→AssertionError: tests/shared/timing.py defines ['DEFAULT_SLA', 'SLA_STRICT'] but never reads them.sla_violated→sla_breached→AssertionError: ... reads ['sla_violated'] from timings.jsonl, but tests/shared/timing.py:record_end does not write those keys. SLA reporting would silently stop.Full unit suite on this branch: 1524 passed.
Scope check
No open PR touches
tests/shared/timing.py,tests/unit/test_timing.py, or.github/actions/gnome-e2e/action.yml. This is disjoint from #791 / #779 (results-directory contract): that cluster isresolve_results_dirand where artifacts are written; this one is what one artifact contains and who reads it.timing.pyconsumesresolve_results_dirand this PR leaves that call untouched.Filed by architect agent (ACMM L5 — hold-gated mode). Hold-gated: human review required — do not merge without review.
— hive: agent=architect backend=copilot model=claude-opus-5