mirror of
https://github.com/XRPLF/rippled.git
synced 2026-09-27 15:28:03 +00:00
fix(telemetry): refuse an empty capture, and name a bad baseline entry
An empty metric surface counted as a complete capture. build_query_plan
returns an empty plan without complaining for any config that yields no
gated keys, so pointing --metrics at the wrong file exits 0 and hands the
paste-me path a metrics:{} artifact to offer as the next baseline. Nothing
about such a run is evidence the pipeline works, so declared == 0 is now a
failure rather than vacuously complete.
The bounds checker also raised AttributeError on a baseline entry that is
not an object, instead of naming the key. A validator whose job is to catch
a malformed contract should report it, not crash on it.
Test cleanup is bound to its own temp tree, so a loop no longer leaves five
of six directories behind.
This commit is contained in:
@@ -43,10 +43,12 @@ baseline's own. Six rules are checked:
|
||||
declare, carries a reason, and has neither a threshold override nor a
|
||||
baseline value left behind.
|
||||
|
||||
Rule A subtracts ``excluded_keys`` before comparing, so a quantile removed from
|
||||
the gated set does not read as a missing baseline. Rule F is what keeps that
|
||||
subtraction honest: an exclusion is the one edit here that makes the gate cover
|
||||
LESS, so a stale or misspelt entry must fail rather than silently widen itself.
|
||||
Rule A subtracts ``excluded_keys`` from BOTH sides of its comparison, so a
|
||||
quantile removed from the gated set neither reads as a missing baseline nor as
|
||||
an undeclared one -- an exclusion left in the baseline is one failure, rule F's,
|
||||
which names the file to edit. Rule F is what keeps that subtraction honest: an
|
||||
exclusion is the one edit here that makes the gate cover LESS, so a stale or
|
||||
misspelt entry must fail rather than silently widen itself.
|
||||
|
||||
A PLACEHOLDER baseline -- ``"placeholder": true`` or an empty ``metrics``
|
||||
object -- exits 0, because that is the documented bootstrap state and CI has to
|
||||
@@ -56,8 +58,9 @@ without having checked anything is the same green-build-that-is-not failure this
|
||||
script exists to prevent, so renaming or deleting one of its inputs must not
|
||||
silence it.
|
||||
|
||||
A baseline ENTRY that is not a positive finite number is rejected before any
|
||||
rule runs, by ``_unusable_baseline``. Every rule does arithmetic on that value,
|
||||
A baseline ENTRY that is not an object, or whose value is not a positive finite
|
||||
number, is rejected before any rule runs -- see ``check_key`` and
|
||||
``_unusable_baseline``. Every rule does arithmetic on that value,
|
||||
and a degenerate one made the script crash with a traceback (rule D divides by
|
||||
it) or emit advice about the wrong file (a negative value inverts rule D's
|
||||
comparison). Reporting malformed input is what this script is for, so it must
|
||||
@@ -287,7 +290,20 @@ def check_key(key, entry, thresholds, ladders):
|
||||
malformed input, and reporting malformed input is this script's whole job,
|
||||
so it is rejected up front rather than arithmetic being attempted on it --
|
||||
see ``_unusable_baseline``.
|
||||
|
||||
The entry's SHAPE is checked first, for the same reason. A hand edit that
|
||||
writes the bare number instead of the ``{"value": .., "unit": ..}`` object
|
||||
leaves no ``.get`` to call, and the script died with an AttributeError
|
||||
traceback naming a line in itself rather than the key at fault.
|
||||
"""
|
||||
if not isinstance(entry, dict):
|
||||
return [
|
||||
f"{key}: baseline entry {entry!r} is not an object carrying value and "
|
||||
f"unit, so no bound can be derived from it. Recapture the baseline "
|
||||
f"from a CI run rather than editing it by hand -- see "
|
||||
f"baselines/README.md"
|
||||
]
|
||||
|
||||
value, unit = entry.get("value"), entry.get("unit", "")
|
||||
edges = ladders.get(unit)
|
||||
if value is None or edges is None:
|
||||
@@ -381,8 +397,16 @@ def main():
|
||||
failures.extend(check_exclusions(metrics_cfg, thresholds, gated, declared))
|
||||
# Rule A compares against the GATED surface, so a deliberately excluded
|
||||
# quantile is not reported as a baseline that was never captured.
|
||||
declared -= set(metrics_cfg.get("excluded_keys", {}))
|
||||
for key in sorted(set(gated) - declared):
|
||||
#
|
||||
# The same keys come off rule A's over-coverage side too. An excluded key
|
||||
# left in the baseline is rule F's finding, reported with the file to edit;
|
||||
# rule A would add a second failure for the same single mistake, saying the
|
||||
# key is not declared -- which is not even true, it is declared and then
|
||||
# excluded. Only exclusions the surface really declares are subtracted, so a
|
||||
# misspelt exclusion naming a stale baseline key still reaches rule A.
|
||||
excluded_declared = set(metrics_cfg.get("excluded_keys", {})) & declared
|
||||
declared -= excluded_declared
|
||||
for key in sorted(set(gated) - declared - excluded_declared):
|
||||
failures.append(
|
||||
f"{key}: in the baseline but not declared by {METRICS}, so it is "
|
||||
f"reported every run and can never gate -- remove it (rule A)"
|
||||
|
||||
@@ -46,7 +46,12 @@ class CheckerCase(unittest.TestCase):
|
||||
|
||||
def setUp(self):
|
||||
self.tree = Path(tempfile.mkdtemp())
|
||||
self.addCleanup(self._cleanup)
|
||||
# Bound to THIS tree, not read off self.tree when the cleanup finally
|
||||
# runs. A test that calls setUp again for a fresh tree (see the
|
||||
# degenerate-baseline subTests) rebinds self.tree, and a late read would
|
||||
# make every registered cleanup remove the LAST tree, leaving each
|
||||
# earlier one behind in /tmp.
|
||||
self.addCleanup(self._cleanup, self.tree)
|
||||
for rel in INPUTS:
|
||||
dest = self.tree / rel
|
||||
dest.parent.mkdir(parents=True, exist_ok=True)
|
||||
@@ -55,11 +60,12 @@ class CheckerCase(unittest.TestCase):
|
||||
script.parent.mkdir(parents=True, exist_ok=True)
|
||||
shutil.copy(CHECKER, script)
|
||||
|
||||
def _cleanup(self):
|
||||
for path in self.tree.rglob("*"):
|
||||
def _cleanup(self, tree):
|
||||
"""Remove one scratch tree, restoring permissions rmtree needs first."""
|
||||
for path in tree.rglob("*"):
|
||||
if path.is_file():
|
||||
path.chmod(stat.S_IRUSR | stat.S_IWUSR)
|
||||
shutil.rmtree(self.tree, ignore_errors=True)
|
||||
shutil.rmtree(tree, ignore_errors=True)
|
||||
|
||||
def run_checker(self):
|
||||
"""Run the checker in the scratch tree, returning (code, stdout+stderr)."""
|
||||
|
||||
Reference in New Issue
Block a user