From e1eb5a95ee2e64999a920fdaefc3197ba417b710 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Fri, 4 Sep 2026 01:23:11 +0200 Subject: [PATCH 1/5] feat(engine): count the packets that left in a different order Jitter and the latency spike change the order packets go out in, and nothing measured whether that actually happened. It is not derivable from the settings: whether a delayed packet is overtaken depends on the gap between packets, so at two packets a second a 50 ms spike reorders nothing. Without the number, a run where reordering never occurred reads exactly like a run the application survived - the hole loss_bursts was added to close for runs of loss. - engine: read the arrival sequence the heap entry already carried and compare it against a per-direction high-water mark, after send() rather than before, so a packet the driver refused cannot make the next one look overtaken by one that never reached the stack - counter surfaces on the Statistics tile and session panel, in the stats CSV as packets_reordered, and in the repro report; the NDJSON sample schema is left alone (frozen contract) - carve damage.py out of engine.py: adding the counter pushed that module past the file-size crowd band (817 logic lines against 816.2), and the ratchet is answered by moving code out, not by raising the number. The five names moved read a stats dict and touch no thread, handle or packet - tests/test_reordering.py: an overtaken packet counts, kept order does not, the directions are judged separately, a refused send does not move the mark, and a restarted session does not inherit the old one Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 9 ++ README.md | 1 + beantester/damage.py | 78 +++++++++++++++ beantester/engine.py | 125 +++++++++++------------- beantester/gui/csv_export.py | 1 + beantester/gui/pages/stats.py | 4 +- beantester/repro.py | 7 +- lang/en.json | 2 + lang/pl.json | 2 + lang/zh.json | 2 + tests/test_engine.py | 2 +- tests/test_reordering.py | 174 ++++++++++++++++++++++++++++++++++ 12 files changed, 336 insertions(+), 71 deletions(-) create mode 100644 beantester/damage.py create mode 100644 tests/test_reordering.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 80b9f64..e3a6189 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,15 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol ## [Unreleased] +### Added + +- **A "Reordered" figure on the Statistics page, and a `packets_reordered` column in the + stats CSV.** Jitter and the latency spike change the order packets go out in, but nothing + ever said whether that actually happened, and it depends on how busy the traffic is rather + than on the setting alone. Zero with jitter set now means the packets were too far apart to + overtake each other, instead of looking the same as an application that coped. An existing + stats CSV is rotated to a dated backup the first time the new column is written. + ## [0.6.0] - 2026-09-03 ### Added diff --git a/README.md b/README.md index c79966b..c5edbbc 100644 --- a/README.md +++ b/README.md @@ -947,6 +947,7 @@ what `packets_seen` counted in the first place - so every row records it in `cap | `dropped_overflow` | dropped because the tool's own queue was full (see the note on it below) | | `corrupted` | packets whose payload was flipped | | `duplicated` | extra copies queued | +| `packets_reordered` | packets that left the tool in a different order than they went in. 0 with jitter or a spike configured means the packets were too far apart for any of them to overtake another. Counted per direction, so two busy connections can overtake each other without either end seeing anything out of order | | `dropped_syn` | TCP SYNs dropped ("connections that never open") | | `dropped_mtu` | dropped for exceeding the max size (MTU black hole) | | `dropped_nat` | dropped because the NAT mapping had expired | diff --git a/beantester/damage.py b/beantester/damage.py new file mode 100644 index 0000000..6256899 --- /dev/null +++ b/beantester/damage.py @@ -0,0 +1,78 @@ +"""What a session did to the traffic, as pure arithmetic over a stats dict. + +Carved out of ``engine.py`` on 2026-09-04, and the reason is worth keeping: adding +the ``reordered`` counter pushed that module past the crowd band of the file-size +ratchet (``tests/test_code_shape.py``), whose answer is to move code out rather +than raise the number. These five names were the obvious passengers - none of them +touches a thread, a handle or a packet, they only READ a snapshot, and two of the +three callers were already importing them across a module boundary. + +Nothing here imports anything: it sits at the bottom of the layering, below +``core`` and ``engine``, so the GUI, the repro report and the tests can all ask +"how much damage did this session do" without dragging the engine in. +""" + +# Which counter a dropped packet lands in, by the reason BeanCore.decide() gave. +# Module level on purpose: written as a literal inside the capture loop it was +# rebuilt for every dropped packet, and a session set to 100% loss drops as often +# as it sees. It is also the SINGLE SOURCE for what counts as damage below. +DROP_BY_REASON = {"syn": "drop_syn", "mtu": "drop_mtu", "nat": "drop_nat", + "rst": "drop_rst", "lan": "drop_lan", + "internet_only": "drop_internet_only", "block": "drop_block", + "flap": "drop_flap", "rate": "drop_rate"} + +# Damage the simulated link inflicted: every reason decide() can name, plus the +# unnamed default (the configured Loss). Derived from the map above so that a new +# impairment cannot quietly fall outside the figure - which is exactly how +# "Effective loss" came to read 0.0% through a session losing 90% to a speed +# limit. Guarded by +# test_engine.py::test_every_drop_counter_and_drop_reason_is_classified. +IMPAIRMENT_DROP_KEYS = (*dict.fromkeys(DROP_BY_REASON.values()), "drop_loss") + +# Losses the TOOL caused, not the link: its delay queue filled up, the session +# ended with packets still parked in it, or re-injecting one failed outright. +# Deliberately NOT part of the loss figure. +# The README defines the term - traffic dropped above a speed limit is counted +# "because that is how a congested link behaves" - and tips.stat_shutdown says of +# these outright "They were not lost in the network". Both have their own tiles, +# and overflow additionally raises a log warning and a banner. Counting them here +# would also let the figure exceed 100%: the delay queue holds out-of-scope +# packets too, so with a narrow target it can drop more than were ever in scope. +TOOL_DROP_KEYS = ("drop_overflow", "drop_shutdown", "drop_send") + + +def impairment_loss_pct(stats): + """Share of the traffic the tool was aiming at that the impairments killed. + + Numerator: every drop ``decide()`` made. Denominator: packets that passed the + targeting gate (``scoped_seen``), not everything captured - with a target set, + other applications' traffic is watched but never impaired, so counting it only + dilutes the answer. Measured before this became one function: 50% loss with a + third of the traffic in scope reported 16.7% while the target application + itself saw 50.1%. + + Both parts are per-packet and every drop counted here happened to a packet + that was in scope, so the result cannot exceed 100%. With no targeting set, + ``scoped_seen == seen`` and this is simply the loss the session inflicted. + + Takes a stats dict rather than an engine so the GUI can compute it from the + snapshot it already holds. A snapshot without ``scoped_seen`` (an older file, + a partial fake) falls back to ``seen``. + """ + scoped = stats.get("scoped_seen", stats.get("seen", 0)) + if not scoped: + return 0.0 + return 100.0 * sum(stats.get(k, 0) for k in IMPAIRMENT_DROP_KEYS) / scoped + + +def corruption_pct(stats): + """Share of the targeted traffic whose payload was actually altered. + + Same denominator as ``impairment_loss_pct``, for the same reason. ``corrupted`` + counts successful payload flips only - a packet with no payload (a bare ACK) + has nothing to corrupt and is not counted. + """ + scoped = stats.get("scoped_seen", stats.get("seen", 0)) + if not scoped: + return 0.0 + return 100.0 * stats.get("corrupted", 0) / scoped diff --git a/beantester/engine.py b/beantester/engine.py index f06c8d4..a95316c 100644 --- a/beantester/engine.py +++ b/beantester/engine.py @@ -99,6 +99,7 @@ from . import portmap from . import winenv from .core import BeanCore +from .damage import DROP_BY_REASON from .i18n import T from .scenario_runner import ScenarioRunner from .target_resolver import TargetResolver @@ -106,72 +107,6 @@ WATCHDOG_TICK_S = 0.2 # how often the deadline / worker health is checked -# Which counter a dropped packet lands in, by the reason BeanCore.decide() gave. -# Module level on purpose: written as a literal inside the capture loop it was -# rebuilt for every dropped packet, and a session set to 100% loss drops as often -# as it sees. It is also the SINGLE SOURCE for what counts as damage below. -DROP_BY_REASON = {"syn": "drop_syn", "mtu": "drop_mtu", "nat": "drop_nat", - "rst": "drop_rst", "lan": "drop_lan", - "internet_only": "drop_internet_only", "block": "drop_block", - "flap": "drop_flap", "rate": "drop_rate"} - -# Damage the simulated link inflicted: every reason decide() can name, plus the -# unnamed default (the configured Loss). Derived from the map above so that a new -# impairment cannot quietly fall outside the figure - which is exactly how -# "Effective loss" came to read 0.0% through a session losing 90% to a speed -# limit. Guarded by -# test_engine.py::test_every_drop_counter_and_drop_reason_is_classified. -IMPAIRMENT_DROP_KEYS = (*dict.fromkeys(DROP_BY_REASON.values()), "drop_loss") - -# Losses the TOOL caused, not the link: its delay queue filled up, the session -# ended with packets still parked in it, or re-injecting one failed outright. -# Deliberately NOT part of the loss figure. -# The README defines the term - traffic dropped above a speed limit is counted -# "because that is how a congested link behaves" - and tips.stat_shutdown says of -# these outright "They were not lost in the network". Both have their own tiles, -# and overflow additionally raises a log warning and a banner. Counting them here -# would also let the figure exceed 100%: the delay queue holds out-of-scope -# packets too, so with a narrow target it can drop more than were ever in scope. -TOOL_DROP_KEYS = ("drop_overflow", "drop_shutdown", "drop_send") - - -def impairment_loss_pct(stats): - """Share of the traffic the tool was aiming at that the impairments killed. - - Numerator: every drop ``decide()`` made. Denominator: packets that passed the - targeting gate (``scoped_seen``), not everything captured - with a target set, - other applications' traffic is watched but never impaired, so counting it only - dilutes the answer. Measured before this became one function: 50% loss with a - third of the traffic in scope reported 16.7% while the target application - itself saw 50.1%. - - Both parts are per-packet and every drop counted here happened to a packet - that was in scope, so the result cannot exceed 100%. With no targeting set, - ``scoped_seen == seen`` and this is simply the loss the session inflicted. - - Takes a stats dict rather than an engine so the GUI can compute it from the - snapshot it already holds. A snapshot without ``scoped_seen`` (an older file, - a partial fake) falls back to ``seen``. - """ - scoped = stats.get("scoped_seen", stats.get("seen", 0)) - if not scoped: - return 0.0 - return 100.0 * sum(stats.get(k, 0) for k in IMPAIRMENT_DROP_KEYS) / scoped - - -def corruption_pct(stats): - """Share of the targeted traffic whose payload was actually altered. - - Same denominator as ``impairment_loss_pct``, for the same reason. ``corrupted`` - counts successful payload flips only - a packet with no payload (a bare ACK) - has nothing to corrupt and is not counted. - """ - scoped = stats.get("scoped_seen", stats.get("seen", 0)) - if not scoped: - return 0.0 - return 100.0 * stats.get("corrupted", 0) / scoped - - # Every running engine, so the interpreter can never exit with an open divert # (a leaked handle keeps the WinDivert driver - and its .sys file - loaded). _LIVE_ENGINES: weakref.WeakSet = weakref.WeakSet() @@ -593,7 +528,16 @@ def reset_stats(self): # reset_buckets, which start() calls in the same breath. loss_bursts=0, drop_loss=0, drop_overflow=0, corrupted=0, - duplicated=0, drop_syn=0, drop_mtu=0, drop_nat=0, + duplicated=0, + # Packets the injector sent AFTER one that arrived + # later than they did - see _inject_loop for what this + # can and cannot say. It sits beside `duplicated` + # because both describe what the tool DID to a packet + # rather than a packet it dropped, and nothing else in + # here describes the ordering effect of latency, + # jitter and the latency spike. + reordered=0, + drop_syn=0, drop_mtu=0, drop_nat=0, drop_rst=0, drop_lan=0, drop_internet_only=0, drop_block=0, drop_flap=0, drop_rate=0, drop_shutdown=0, drop_send=0, @@ -620,6 +564,13 @@ def reset_stats(self): driver_wait_peak_ms=0.0) # counters back to zero means the warning should be able to fire again: # a fresh measurement window that overflows must say so afresh + # Highest arrival number already sent, per direction, for the `reordered` + # counter above. Reset here with the counters it feeds, or a restarted + # session would judge its first packets against the last session's high + # water mark and report every one of them as overtaken. Touched ONLY by + # the inject thread while a session runs, and this method runs before + # that thread exists (__init__ and start(), both before the workers). + self._last_sent = {True: -1, False: -1} self._overflow_warned = 0.0 self._send_warned = 0.0 self._driver_wait_warned = 0.0 @@ -2001,6 +1952,42 @@ def _enqueue(self, release, packet, copy=False, key=None, modified=False): self._warn_overflow() # outside the lock: it logs, and logging waits return queued + def _note_order(self, arrived, is_out): + """Did this packet leave after one that arrived AFTER it? Then it was overtaken. + + The injector knows both orders: the heap entry carries the number the packet + was given when it ARRIVED, and the order packets leave this loop is the order + the stack sees them in. An arrival number lower than the highest already sent + on this direction means something that arrived later went out first. + + Why the counter exists at all: "10% got +50 ms" is NOT "10% arrived out of + order". Whether anything overtakes anything depends on the gap between + packets, which is a property of the TRAFFIC - at two packets a second a 50 ms + spike reorders nothing. Without this number a run where reordering never + happened reads exactly like a run the application coped with, which is the + hole ``loss_bursts`` was added to close for runs of loss. + + 🔴 PER DIRECTION, NOT PER FLOW - said here rather than left to be discovered. + Two interleaved flows can overtake each other without either receiver seeing + anything out of order, and this counts that. Per flow would mean a dict lookup + per packet on this thread. The end-to-end answer is the rig + (``internal_tools/probe_reorder_truth.py``), which numbers its own datagrams + and counts gaps at the RECEIVER instead of reading this counter. + + Called only after ``send()`` returned, so the number describes what reached + the stack rather than what the loop intended: a packet the driver refused + leaves the mark alone and cannot make the packet behind it look overtaken by + one that never went out. + + It lives outside ``_inject_loop`` because the loop is already four levels deep + at the call site and the nesting ratchet (``tests/test_code_shape.py``) is + answered by moving code out, not by raising the number. + """ + if arrived < self._last_sent[is_out]: + self._bump("reordered") + else: + self._last_sent[is_out] = arrived + def _inject_loop(self): while self._running: with self._cv: @@ -2008,7 +1995,7 @@ def _inject_loop(self): self._cv.wait() if not self._running: break - release, _, packet, _, key, modified = self._heap[0] + release, arrived, packet, _, key, modified = self._heap[0] now = time.monotonic() if release > now: self._cv.wait(timeout=min(release - now, 0.5)) @@ -2057,6 +2044,8 @@ def _inject_loop(self): # heading the session panel used for delivered: measured # bytes_in = 5 122 600 B in a row that received 409 600 B. self._log_delivered(key, size, is_out) + # AFTER the send, deliberately - see _note_order. + self._note_order(arrived, bool(is_out)) except Exception as e: # The packet is already off the heap: not delivered, and until this # counter existed, not recorded either - it simply left the diff --git a/beantester/gui/csv_export.py b/beantester/gui/csv_export.py index e03e593..3c355a1 100644 --- a/beantester/gui/csv_export.py +++ b/beantester/gui/csv_export.py @@ -22,6 +22,7 @@ # by spreadsheets, so it gets column names that mean something. CSV_COLUMNS = {"seen": "packets_seen", "scoped_seen": "packets_in_scope", "drop_loss": "dropped_loss", "loss_bursts": "loss_runs", + "reordered": "packets_reordered", "drop_overflow": "dropped_overflow", "drop_syn": "dropped_syn", "drop_mtu": "dropped_mtu", "drop_nat": "dropped_nat", "drop_rst": "dropped_rst", "rst_reset": "connections_reset", diff --git a/beantester/gui/pages/stats.py b/beantester/gui/pages/stats.py index 9104fc8..17e5d38 100644 --- a/beantester/gui/pages/stats.py +++ b/beantester/gui/pages/stats.py @@ -13,7 +13,7 @@ import tkinter as tk from tkinter import ttk -from ...engine import impairment_loss_pct +from ...damage import impairment_loss_pct from ...i18n import T, event_kind_label from ...views import sort_events from ..chart import draw_throughput_chart @@ -67,6 +67,7 @@ ("loss_bursts", "stats.loss_runs", "", "tips.stat_loss_runs"), ("corrupted", "stats.corrupted", "", "tips.stat_corrupted"), ("duplicated", "stats.duplicated", "", "tips.stat_duplicated"), + ("reordered", "stats.reordered", "", "tips.stat_reordered"), ("drop_overflow", "stats.overflow", "", "tips.stat_overflow"), ("drop_shutdown", "stats.shutdown_dropped", "", "tips.stat_shutdown"), ("drop_send", "stats.send_failed", "", "tips.stat_send_failed"), @@ -499,6 +500,7 @@ def refresh_counters(self): # the FULL traffic on purpose - they are what the TOOL lost, including # traffic the user never targeted, and narrowing them would hide it. for key in ("seen", "queue", "drop_loss", "loss_bursts", "corrupted", "duplicated", + "reordered", "drop_overflow", "drop_shutdown", "drop_send", "drop_rate", "drop_syn", "drop_mtu", "drop_nat", "drop_rst", "drop_lan", "drop_internet_only", diff --git a/beantester/repro.py b/beantester/repro.py index a9ef8b6..e2130ad 100644 --- a/beantester/repro.py +++ b/beantester/repro.py @@ -3,7 +3,7 @@ import time from .appinfo import TOOL_ID, command_name -from .engine import corruption_pct, impairment_loss_pct +from .damage import corruption_pct, impairment_loss_pct from .i18n import translate from .settings import DEFAULT_SETTINGS, setting_expression from .utils import bytes_to_mb, number_string, to_number @@ -101,6 +101,11 @@ def build_repro_report(engine, settings): # means the session was too short to see one, which is the difference # between a run that proved nothing and a tool that is broken. loss_runs=stats.get("loss_bursts", 0), + # How many packets left this tool AFTER one that arrived later than they + # did. Zero with jitter or a latency spike configured means the traffic + # was too sparse for anything to overtake anything, not that the delay + # was never applied - the same distinction loss_runs draws above. + reordered=stats.get("reordered", 0), # connections_reset held drop_rst - the PACKETS a reset connection swallows # during its cooldown, which for a 30 s cooldown on a busy flow is thousands # against a handful of actual resets. The three RST numbers answer three diff --git a/lang/en.json b/lang/en.json index 94f7b59..9a680a9 100644 --- a/lang/en.json +++ b/lang/en.json @@ -415,6 +415,7 @@ "stats.packets": "Packets", "stats.queued": "Queued", "stats.rate_dropped": "Rate-limit drop", + "stats.reordered": "Reordered", "stats.rst_reset": "RST reset", "stats.rst_sent": "RST sent", "stats.scope_note": "Counters cover ALL captured traffic (per \"Traffic to modify\"), not only the traffic your targeting impairs.", @@ -566,6 +567,7 @@ "tips.stat_overflow": "Packets dropped because the queue overflowed (heavy overload). This counter always covers ALL captured traffic, even when the view is narrowed to the target: these are packets the TOOL lost, and hiding the ones outside your target would hide its own damage.", "tips.stat_queue": "How many packets are waiting in the queue (grows with latency or a speed limit).", "tips.stat_rate": "Packets dropped because the speed-limit buffer filled up (offered traffic stayed above the limit longer than the buffer holds). Counted separately from loss and from tool overflow.", + "tips.stat_reordered": "How many packets came out of the tool in a different order than they went in. Zero with Jitter or Spike chance set means the packets were too far apart for any of them to overtake another, not that the delay was missing.", "tips.stat_rst": "Packets of reset connections (RST).", "tips.stat_rst_sent": "How many RST packets were injected to reset connections.", "tips.stat_seen": "How many packets passed through the tool in total.", diff --git a/lang/pl.json b/lang/pl.json index 169d17c..5743c0f 100644 --- a/lang/pl.json +++ b/lang/pl.json @@ -415,6 +415,7 @@ "stats.packets": "Pakiety", "stats.queued": "W kolejce", "stats.rate_dropped": "Odrzuc. przez limit", + "stats.reordered": "Zmieniona kolejność", "stats.rst_reset": "RST zerwane", "stats.rst_sent": "RST wysłane", "stats.scope_note": "Liczniki obejmują CAŁY przechwycony ruch (wg „Ruch do modyfikacji”), a nie tylko ten, który psuje celowanie.", @@ -566,6 +567,7 @@ "tips.stat_overflow": "Pakiety porzucone, bo kolejka się przepełniła (silne przeciążenie). Ten licznik zawsze obejmuje CAŁY przechwycony ruch, nawet gdy widok jest zawężony do celu: to są pakiety zgubione przez NARZĘDZIE, a ukrycie tych spoza celu ukryłoby jego własne szkody.", "tips.stat_queue": "Ile pakietów czeka w kolejce (rośnie, gdy działa opóźnienie lub limit prędkości).", "tips.stat_rate": "Pakiety porzucone, bo bufor limitu prędkości się zapełnił (ruch powyżej limitu dłużej niż mieści bufor). Liczone osobno od strat i od przepełnienia narzędzia.", + "tips.stat_reordered": "Ile pakietów wyszło z narzędzia w innej kolejności, niż do niego weszło. Zero przy ustawionym Jitterze albo Szansie skoku znaczy, że pakiety szły zbyt rzadko, żeby któryś wyprzedził inny, a nie że opóźnienia nie było.", "tips.stat_rst": "Pakiety zerwanych połączeń (RST).", "tips.stat_rst_sent": "Ile pakietów RST wstrzyknięto, by zerwać połączenia.", "tips.stat_seen": "Ile pakietów łącznie przeszło przez narzędzie.", diff --git a/lang/zh.json b/lang/zh.json index 45dfb15..07121a9 100644 --- a/lang/zh.json +++ b/lang/zh.json @@ -415,6 +415,7 @@ "stats.packets": "数据包", "stats.queued": "队列中", "stats.rate_dropped": "因限速丢弃", + "stats.reordered": "乱序包数", "stats.rst_reset": "RST 重置", "stats.rst_sent": "已发送 RST", "stats.scope_note": "计数器覆盖所有已捕获流量（由“要修改的流量”决定），而不只是目标规则实际施加弱网的流量。", @@ -566,6 +567,7 @@ "tips.stat_overflow": "因队列溢出（严重过载）而被丢弃的数据包。即使视图已收窄到目标，此计数器也始终覆盖所有已捕获流量，因为这些数据包是本工具丢失的。隐藏目标之外的部分会掩盖工具自身造成的损害。", "tips.stat_queue": "当前正在队列中等待的数据包数，会随延迟或限速而增加。", "tips.stat_rate": "因限速缓冲区已满而被丢弃的数据包，即输入流量持续高于限速值且超过缓冲区承载时间。它与普通丢包及工具队列溢出分开统计。", + "tips.stat_reordered": "有多少数据包离开工具时的顺序与进入时不同。设置了抖动或尖峰概率却显示 0，说明数据包之间间隔太大，谁也没能超过谁，而不是延迟没有生效。", "tips.stat_rst": "属于已重置连接（RST）的数据包。", "tips.stat_rst_sent": "为重置连接而注入的 RST 数据包数。", "tips.stat_seen": "经过本工具的全部数据包数量。", diff --git a/tests/test_engine.py b/tests/test_engine.py index 742e482..2a36fec 100644 --- a/tests/test_engine.py +++ b/tests/test_engine.py @@ -12,7 +12,7 @@ from beantester import BeanEngine from beantester.core import BeanCore -from beantester.engine import (DROP_BY_REASON, IMPAIRMENT_DROP_KEYS, TOOL_DROP_KEYS, +from beantester.damage import (DROP_BY_REASON, IMPAIRMENT_DROP_KEYS, TOOL_DROP_KEYS, impairment_loss_pct) from beantester.settings import DEFAULT_SETTINGS, apply_settings from beantester.synthetic import SyntheticDivert diff --git a/tests/test_reordering.py b/tests/test_reordering.py new file mode 100644 index 0000000..6ef6db4 --- /dev/null +++ b/tests/test_reordering.py @@ -0,0 +1,174 @@ +"""The ``reordered`` counter: did the tool actually change the order of packets? + +Why this counter needs a guard of its own. Configuring jitter or a latency spike +does NOT mean anything was reordered - whether a delayed packet is overtaken +depends on how far apart the packets were, which is a property of the traffic +rather than of the settings. So a run where nothing ever overtook anything reads +exactly like a run where the application coped, and the only thing that can tell +them apart is a number. That is the same hole ``loss_bursts`` was added to close +for losses arriving in runs. + +These tests drive ``BeanEngine._enqueue`` directly rather than going through +``decide()``. That is deliberate: the question here is what the INJECTOR counts +given a known release order, and driving the decision pipeline would make the +test depend on a random draw to produce the very ordering it wants to assert on. +The decision side is covered by ``test_core.py``. + +The release offsets below are generous (hundreds of ms for the "late" packet) +for one reason worth stating: the heap pops by release time, so the ORDER under +test is deterministic no matter how loaded the machine is - the single way this +could go wrong is the injector sending the late packet before the early one has +been queued at all, which needs a stall longer than that offset between two +adjacent statements. +""" +import time + +from beantester import BeanEngine + +from fakes import FakeDivert, FakePacket, check + + +class RefusingDivert(FakeDivert): + """A diverter that refuses ONE packet, so a failed send can be observed.""" + + def __init__(self, packets, refuse): + super().__init__(packets) + self.refuse = refuse + + def send(self, p, recalculate_checksum=True): + if p is self.refuse: + raise OSError("the driver refused this one") + super().send(p, recalculate_checksum=recalculate_checksum) + + +def _run(releases, divert=None): + """Queue ``(offset, packet)`` pairs in order, wait for delivery, return stats. + + ``releases`` is queued in list order, so the first entry is the packet that + ARRIVED first - which is the whole variable these tests turn. + """ + fake = divert if divert is not None else FakeDivert([]) + engine = BeanEngine() + engine.start("test", divert=fake) + try: + now = time.monotonic() + for offset, packet in releases: + engine._enqueue(now + offset, packet) + deadline = time.time() + 10 + while time.time() < deadline: + s = engine.stats_snapshot() + if s["queue"] == 0 and len(fake.sent) + s["drop_send"] >= len(releases): + break + time.sleep(0.01) + time.sleep(0.05) + return engine.stats_snapshot(), [p for _, p in fake.sent] + finally: + engine.stop() + + +def test_a_packet_that_is_overtaken_is_counted_as_reordered(): + """The point of the counter: one packet leaves after one that arrived later.""" + first = FakePacket(port=1001) + second = FakePacket(port=1002) + stats, sent = _run([(0.40, first), (0.05, second)]) + + check("the packet that arrived second was sent first", + sent == [second, first], f"(sent {len(sent)} packets)") + check("one reorder is counted", stats["reordered"] == 1, + f"(reordered={stats['reordered']})") + + +def test_packets_that_keep_their_order_are_not_counted(): + """The other half, and the one that stops the counter being always-on. + + Without this, a counter that simply incremented per packet would pass the + test above and be worthless. + """ + first = FakePacket(port=1001) + second = FakePacket(port=1002) + stats, sent = _run([(0.05, first), (0.30, second)]) + + check("the order was kept", sent == [first, second], f"(sent {len(sent)})") + check("nothing is counted as reordered", stats["reordered"] == 0, + f"(reordered={stats['reordered']})") + + +def test_the_two_directions_are_judged_separately(): + """A design decision, pinned: the high-water mark is PER DIRECTION. + + An inbound packet overtaking an outbound one is not a reorder anybody can + observe - they are different conversations. With one shared mark this reads + as a reorder, so the mistake would be invisible except as a counter that + ticks up on ordinary two-way traffic. + """ + outbound = FakePacket(port=1001, is_outbound=True) + inbound = FakePacket(port=1002, is_outbound=False) + stats, sent = _run([(0.40, outbound), (0.05, inbound)]) + + check("the inbound packet went out first", sent == [inbound, outbound], + f"(sent {len(sent)})") + check("crossing directions is not a reorder", stats["reordered"] == 0, + f"(reordered={stats['reordered']})") + + +def test_a_packet_the_driver_refused_does_not_make_the_next_one_look_overtaken(): + """Counted after send(), not before - and this is what makes the difference. + + The refused packet arrived LAST and was released FIRST. If the counter marked + it as sent before the driver had taken it, the packet behind it would be + judged against a packet that never reached the stack and reported as + overtaken by it. + """ + arrived_first = FakePacket(port=1001) + arrived_second = FakePacket(port=1002) + fake = RefusingDivert([], refuse=arrived_second) + stats, sent = _run([(0.40, arrived_first), (0.05, arrived_second)], divert=fake) + + check("only the accepted packet was sent", sent == [arrived_first], + f"(sent {len(sent)})") + check("the refused packet was counted as a failed send", + stats["drop_send"] == 1, f"(drop_send={stats['drop_send']})") + check("nothing is reported as overtaken", stats["reordered"] == 0, + f"(reordered={stats['reordered']})") + + +def test_a_restarted_session_does_not_inherit_the_previous_high_water_mark(): + """Second sessions start clean, or every early packet reads as overtaken. + + ``reset_stats`` zeroes the counter itself; the mark the counter is judged + against has to go with it. Missing that, the numbering carries on across the + restart while the mark stays high, so the next session reports reordering it + never did. + """ + engine = BeanEngine() + + first_run = FakeDivert([]) + engine.start("test", divert=first_run) + now = time.monotonic() + engine._enqueue(now + 0.40, FakePacket(port=1001)) + engine._enqueue(now + 0.05, FakePacket(port=1002)) + deadline = time.time() + 10 + while time.time() < deadline and len(first_run.sent) < 2: + time.sleep(0.01) + check("the first session did reorder", engine.stats_snapshot()["reordered"] == 1, + f"(reordered={engine.stats_snapshot()['reordered']})") + engine.stop() + + second_run = FakeDivert([]) + engine.start("test", divert=second_run) + try: + now = time.monotonic() + engine._enqueue(now + 0.05, FakePacket(port=2001)) + engine._enqueue(now + 0.30, FakePacket(port=2002)) + deadline = time.time() + 10 + while time.time() < deadline and len(second_run.sent) < 2: + time.sleep(0.01) + time.sleep(0.05) + stats = engine.stats_snapshot() + finally: + engine.stop() + + check("the second session sent two packets in order", + len(second_run.sent) == 2, f"(sent {len(second_run.sent)})") + check("the second session reports no reordering", stats["reordered"] == 0, + f"(reordered={stats['reordered']})") From 97b89c2df07914b029b52e45f712908e0cec3350 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Fri, 4 Sep 2026 01:27:30 +0200 Subject: [PATCH 2/5] test(engine): make the reordering guards repeatable, and fix the one that could not fail Five entries in the mutation registry, so "these tests catch it" is a claim CI repeats instead of a sentence in a changelog. The fifth mutant SURVIVED the first run, and that was information about the test: the arrival counter was built in __init__ and never reset, so a second session's numbers were always higher than a stale high-water mark and the restart assertion could not fail in either direction. Fixed at the source rather than in the test - arrival numbering is now per session and resets alongside the mark it is judged against, since the mark only means anything against numbers from the same numbering. stop() clears the heap, so a reused number cannot meet a queued entry that still holds it as a tie-breaker. - repoint "engine: the Internet-only drop loses its own counter" at damage.py: the carve-out moved its pattern, so it reported SKIP, and a skip reads like a pass. Re-run from its new home: caught - reordering mutations, re-run after the fix: 5 of 5 caught Co-Authored-By: Claude Opus 5 --- beantester/engine.py | 20 ++++++---- tests/test_mutation_registry.py | 67 ++++++++++++++++++++++++++++++++- 2 files changed, 79 insertions(+), 8 deletions(-) diff --git a/beantester/engine.py b/beantester/engine.py index a95316c..89e6706 100644 --- a/beantester/engine.py +++ b/beantester/engine.py @@ -135,7 +135,6 @@ def __init__(self, log_fn=lambda *_: None): self._divert = None self._running = False self._heap = [] - self._counter = itertools.count() self._cv = threading.Condition() self.max_queue = 20000 self._slock = threading.Lock() @@ -564,12 +563,19 @@ def reset_stats(self): driver_wait_peak_ms=0.0) # counters back to zero means the warning should be able to fire again: # a fresh measurement window that overflows must say so afresh - # Highest arrival number already sent, per direction, for the `reordered` - # counter above. Reset here with the counters it feeds, or a restarted - # session would judge its first packets against the last session's high - # water mark and report every one of them as overtaken. Touched ONLY by - # the inject thread while a session runs, and this method runs before - # that thread exists (__init__ and start(), both before the workers). + # Arrival numbering, and the high-water mark it is judged against for the + # `reordered` counter above. ONE fact in two variables, so they are reset + # in one place: the mark only means anything against numbers from the same + # numbering, and a session that inherited one without the other would judge + # its first packets against a stranger. `stop()` clears the heap, so no + # entry from the previous session can collide with a reused number - which + # matters, because the number is also the heap's tie-breaker and two equal + # keys would push it into comparing packet objects. + # + # Both are touched ONLY by the inject thread while a session runs, and this + # method runs before that thread exists (from __init__ and from start(), + # both before the workers are spawned). + self._counter = itertools.count() self._last_sent = {True: -1, False: -1} self._overflow_warned = 0.0 self._send_warned = 0.0 diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 7668c25..6a82f2e 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -1884,8 +1884,11 @@ # Without its own row the drop falls through to the unnamed default and # is reported as packet LOSS - the exact confusion drop_flap was split # out to end. + # Moved with DROP_BY_REASON when damage.py was carved out of engine.py + # (2026-09-04). The registry reported it the same day: a pattern that no + # longer matches is a SKIP, and a skip reads like a pass. "label": "engine: the Internet-only drop loses its own counter", - "file": "beantester/engine.py", + "file": "beantester/damage.py", "old": ' "internet_only": "drop_internet_only", "block": "drop_block",', "new": ' "block": "drop_block",', "test": "test_every_drop_counter_and_drop_reason_is_classified", @@ -2034,6 +2037,68 @@ " __import__('url' + 'lib.request')", "test": "test_a_real_run_raises_no_network_audit_event", }, + { + # The counter simply not counting. Its own tile would read 0 for ever, + # which is indistinguishable from a session where nothing overtook + # anything - the exact confusion the counter exists to end. + "label": "reordering: an overtaken packet is not counted", + "file": "beantester/engine.py", + "old": ' self._bump("reordered")', + "new": " pass", + "test": "test_a_packet_that_is_overtaken_is_counted_as_reordered", + }, + { + # The other direction, and the one a bare "does it count?" test misses: + # a counter that increments per delivered packet passes the test above + # and means nothing. + "label": "reordering: every packet counts as reordered", + "file": "beantester/engine.py", + "old": " if arrived < self._last_sent[is_out]:", + "new": " if True:", + "test": "test_packets_that_keep_their_order_are_not_counted", + }, + { + # One shared high-water mark instead of one per direction. Ordinary + # two-way traffic then ticks the counter for inbound and outbound + # packets overtaking each other, which no receiver can observe. + "label": "reordering: one high-water mark shared by both directions", + "file": "beantester/engine.py", + "old": " if arrived < self._last_sent[is_out]:\n" + ' self._bump("reordered")\n' + " else:\n" + " self._last_sent[is_out] = arrived", + "new": " if arrived < self._last_sent[True]:\n" + ' self._bump("reordered")\n' + " else:\n" + " self._last_sent[True] = arrived", + "test": "test_the_two_directions_are_judged_separately", + }, + { + # Marking a packet as sent on the FAILURE path. The packet behind it is + # then judged against one that never reached the stack and reported as + # overtaken by it. Surgical on purpose: it touches only the except + # branch, so the other four tests here stay green and this one is shown + # to be load-bearing on its own. + "label": "reordering: a refused send still moves the mark", + "file": "beantester/engine.py", + "old": ' self._bump("drop_send")\n' + ' self._charge_flow(key, "dropped")\n', + "new": ' self._bump("drop_send")\n' + ' self._charge_flow(key, "dropped")\n' + " self._note_order(arrived,\n" + ' bool(getattr(packet, "is_outbound", True)))\n', + "test": "test_a_packet_the_driver_refused_does_not_make_the_next_one_look_overtaken", + }, + { + # The mark surviving a restart while the counter is zeroed. The next + # session then reports reordering it never did, on its very first + # packets, and the numbers look plausible. + "label": "reordering: a restart keeps the previous high-water mark", + "file": "beantester/engine.py", + "old": " self._last_sent = {True: -1, False: -1}", + "new": ' self._last_sent = getattr(self, "_last_sent", {True: -1, False: -1})', + "test": "test_a_restarted_session_does_not_inherit_the_previous_high_water_mark", + }, ] # The runner's own check: a patch that cannot compile must be reported as BROKEN, not From 3bad8dfbcbf894b4e7d0742edb639d6a70e9f207 Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Fri, 4 Sep 2026 01:30:44 +0200 Subject: [PATCH 3/5] docs(gui): name packet order where somebody testing it will look The knob for out-of-order delivery already existed - a latency spike is "delay X% of packets by Y ms", which reorders traffic without smearing every other packet's delay the way jitter does. Nothing said so. It sat in a card called "Latency (ping)" under a tooltip about momentary lag, and the Control page search matches names rather than tooltip bodies, so a person looking for reordering found nothing. - rename the card to "Latency (ping) and packet order" in all three languages. The section title is folded into the haystack of every field inside it, so one string makes the card and all four of its fields findable. MEASURED: "order" and "kolejnosc" went from 0 hits to 5 - rewrite tips.spike so its second sentence carries the consequence that matters instead of restating the first - README: a paragraph on testing out-of-order delivery, and what a zero in the new Reordered figure does and does not mean - guard the discoverability itself, since a later reword would take it away silently Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 ++++++++ README.md | 14 ++++++++++++++ lang/en.json | 4 ++-- lang/pl.json | 4 ++-- lang/zh.json | 4 ++-- tests/test_form_search.py | 32 ++++++++++++++++++++++++++++++++ 6 files changed, 60 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e3a6189..8ded115 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,14 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol overtake each other, instead of looking the same as an application that coped. An existing stats CSV is rotated to a dated backup the first time the new column is written. +### Changed + +- **The "Latency (ping)" card is now "Latency (ping) and packet order", and the spike + tooltip says what the spike is good for.** Delaying a slice of packets is how you test a + protocol that has to survive out-of-order delivery, and the field search matches names + rather than tooltips - so searching the Control page for "order" (or "kolejnosc") found + nothing at all, in either language. It now finds the card and all four fields in it. + ## [0.6.0] - 2026-09-03 ### Added diff --git a/README.md b/README.md index c5edbbc..124d83e 100644 --- a/README.md +++ b/README.md @@ -299,6 +299,19 @@ one. With the given probability it appends extra delay (ms) to a **single packet momentary "lag" actually arrives. The chance is per packet and applies **in each direction**, so a round trip hits it about twice as often as the number suggests. +**Testing out-of-order delivery** - this is what the spike pair is for, and it is the reason the +section is called "Latency (ping) and packet order". A spiked packet arrives after packets that +were sent later than it, so *Spike chance* and *Spike size* reorder traffic **without** smearing +every other packet's delay the way jitter does - which matters when the thing under test is a UDP +protocol that has to survive reordering on its own: a game, QUIC, telemetry, voice. Set a spike +size larger than the gap between your packets, or nothing will overtake anything. + +Whether that actually happened is a separate question from whether it was configured, so the tool +counts it: **Reordered** on the Statistics page (and `packets_reordered` in the stats CSV). 0 with a +spike set means your traffic was too sparse for any packet to overtake another - not that the +setting was ignored. The count is per direction, so two busy connections can overtake each other +without either end seeing anything out of order. + **Impairments** - *Loss*: percentage of packets vanishing without a trace (5% is already a clearly failing network). *Corruption*: percentage of packets with a flipped data bit - it affects **only payload-bearing packets**. Packets with no data (e.g. pure ACK, SYN) have nothing to flip, @@ -1263,6 +1276,7 @@ the root, so all existing commands (README, reproduction reports, PyInstaller) w bean_network_tester.py launcher + compatibility facade (re-exports the public API) beantester/ the implementation package core.py pure per-packet decision core (BeanCore) + damage.py how much a session damaged: drop reasons, loss/corruption shares engine.py capture/inject threads, statistics (BeanEngine) matchers.py filter expressions (list/range/!/>/ Date: Fri, 4 Sep 2026 01:46:09 +0200 Subject: [PATCH 4/5] feat(gui): read throughput in KB/s, Mbit/s or MB/s A speed limit is typed in KB/s and a link is sold in Mbit/s, and the tool made you do that conversion in your head - in the one place where getting it wrong is invisible, because a wrong limit still runs. The switch is DISPLAY only, and that is forced rather than chosen: KB/s is what a saved config file, the throughput schedule, the shipped scenarios, --down/--up and the NDJSON down_kbps/up_kbps fields all carry, and several of those are frozen contracts. So it converts on the way to the screen and never on the way to a file. - new CHOICE kind in the Pref registry, rendered as a readonly combobox. The language box beside it is hand-rendered and is a named exception, so copying that would have copied the exception rather than the rule - gui/rates.py gains the unit table and the conversion; the rate fields are a VIEW over the field registry (unit == "KB/s"), not a list of names - wired into both Statistics tiles and their captions, the copied text, the session peak and average, the chart axis and caption, and a live grey readout beside Download and Upload. The chart converts labels only: the series and the ceiling stay in KB/s - the readout is pushed from set_pref rather than polled - the dropdown is in a different window from the label it moves - 1024 KB/s is 8.39 Mbit/s and not 8: K is 1024 here, a megabit is a decimal million, and the two do not cancel. The factor is derived in the open and the comfortable wrong answer is asserted against, because that version looks more right than this one Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 7 ++ README.md | 13 +++ beantester/gui/app.py | 13 +++ beantester/gui/chart.py | 27 ++++- beantester/gui/form.py | 44 ++++++++ beantester/gui/pages/stats.py | 49 +++++++- beantester/gui/panels/settings.py | 33 +++++- beantester/gui/prefs.py | 35 +++++- beantester/gui/rates.py | 61 ++++++++++ lang/en.json | 3 + lang/pl.json | 3 + lang/zh.json | 3 + tests/test_mutation_registry.py | 40 +++++++ tests/test_rate_units.py | 179 ++++++++++++++++++++++++++++++ 14 files changed, 497 insertions(+), 13 deletions(-) create mode 100644 tests/test_rate_units.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 8ded115..4166370 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,13 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol protocol that has to survive out-of-order delivery, and the field search matches names rather than tooltips - so searching the Control page for "order" (or "kolejnosc") found nothing at all, in either language. It now finds the card and all four fields in it. +- **A speed unit you can pick: `KB/s`, `Mbit/s` or `MB/s` (Settings window).** The Statistics + page, the chart, the session peak and average, and a grey readout beside Download and Upload all + follow it. It changes what you READ, never what you type - the limits stay in KB/s, because that + is the number a saved config file, the throughput schedule, the shipped scenarios, `--down`, + `--up` and the NDJSON output all carry. `K` here is 1024 and a megabit is a decimal million, so + 1024 KB/s reads as 8.39 Mbit/s rather than 8, which is the honest conversion and not a rounding + error. ## [0.6.0] - 2026-09-03 diff --git a/README.md b/README.md index 124d83e..765bf73 100644 --- a/README.md +++ b/README.md @@ -267,6 +267,19 @@ KB/s. 0 = no limit. Ping is small packets, so a speed limit barely changes it - use a file download. A positive value always limits something: an extremely small limit (below 1 B/s) is floored to 1 B/s, it does not silently turn into "no limit". +**Speed unit (Settings window)** - whether the Statistics page, the chart and the readout next to +these two fields show throughput in `KB/s`, `Mbit/s` or `MB/s`. It changes what you READ, never +what you type: the limits themselves stay in KB/s, because that is the number a saved +configuration file, the throughput schedule, the shipped scenarios, `--down`/`--up` and the NDJSON +output all carry. Pick a unit and a grey `(= 8.39 Mbit/s)` appears beside the field, so you can +type the number the tool wants while reading the number your link is sold in. + +> **`K` here is 1024, and a megabit is a decimal million** - the two conventions do not cancel out, +> so 1024 KB/s is **8.39 Mbit/s**, not 8. That is the honest conversion, not a rounding error: a +> byte is 8 bits, `1024 x 1024 x 8 = 8 388 608` bits per second, and megabit means 10^6 bits +> everywhere a link is sold. Divide by 8.39, not by 8, when you want a limit to match an +> advertised speed. + **Buffer** - the capacity of the link buffer for a speed limit, in milliseconds (0 = unlimited buffer). It sets how much queueing delay may build up on a rate-limited link before it starts dropping the excess (bufferbloat). Without this buffer the token bucket could "run away" tens of diff --git a/beantester/gui/app.py b/beantester/gui/app.py index ae84951..f55a437 100644 --- a/beantester/gui/app.py +++ b/beantester/gui/app.py @@ -720,6 +720,19 @@ def set_pref(self, key, value): unclean exit, unlike session state that is written on close).""" self.ui.set(prefs.ui_key(key), value) self.ui.persist() + if key == "rate_unit": + # PUSHED, not polled. The readout sits on the Control page while the + # dropdown that moves it is in another window, so waiting for the next + # rebuild would make the preference look ignored. Only the Control + # form is told: the settings surface holds no rate field (its sections + # are the table limit and the scope card), so the Settings window's own + # form has nothing to rewrite - and `rate_hints` is keyed off the + # registry, so a rate field moved there later would start arriving here + # rather than needing this line changed. + form = getattr(self, "form", None) + if form is not None: + with crashlog.quiet("gui.app"): + form.sync_rate_hints() def chart_samples(self): """Chart history length in samples, derived from the seconds preference and diff --git a/beantester/gui/chart.py b/beantester/gui/chart.py index 9307165..a589eb3 100644 --- a/beantester/gui/chart.py +++ b/beantester/gui/chart.py @@ -1,6 +1,14 @@ -"""Throughput chart: grid, Y axis in KB/s, down/up series and live readouts.""" +"""Throughput chart: grid, Y axis, down/up series and live readouts. + +The series are KB/s, which is what the engine counts and what the caller +holds. The Y axis is LABELLED in whichever unit the reader picked (see +``gui/rates.py``), so the picture is the same and only the numbers beside it +change - the docstring used to say "Y axis in KB/s" and that stopped being +true the moment the preference existed. +""" from ..i18n import T from ..utils import nice_ceiling +from .rates import BASE_LABEL, DEFAULT_UNIT, UNIT_LABEL, in_unit from .scaling import chart_geometry, scaled from .theme import DOWN_C, FONT, GRID_C, MUT, UP_C @@ -18,8 +26,16 @@ def _axis_label(value, peak): return f"{value:.2f}" -def draw_throughput_chart(canvas, down_hist, up_hist, sample_interval_s=0.7): - """Redraw the throughput chart on the given canvas.""" +def draw_throughput_chart(canvas, down_hist, up_hist, sample_interval_s=0.7, + unit=DEFAULT_UNIT): + """Redraw the throughput chart on the given canvas. + + ``unit`` changes the AXIS LABELS and the caption only. The history and the + plot scale stay in KB/s, which is what the caller holds and what the + engine counts - converting the data as well would mean the "nice" ceiling + were computed on one scale and drawn on another, and the shape of the line + is the same either way. + """ c = canvas try: width = c.winfo_width() @@ -46,10 +62,11 @@ def draw_throughput_chart(canvas, down_hist, up_hist, sample_interval_s=0.7): y = y0 + ph - ph * frac c.create_line(x0, y, x0 + pw, y, fill=GRID_C) c.create_text(g["ml"] - scaled(8), y, anchor="e", fill=MUT, font=(FONT, 8), - text=_axis_label(peak * frac, peak)) + text=_axis_label(in_unit(peak * frac, unit), + in_unit(peak, unit))) # the unit caption sits ABOVE the plot, not on top of the topmost value c.create_text(scaled(6), y0 - scaled(12), anchor="w", fill=MUT, font=(FONT, 8), - text="KB/s") + text=UNIT_LABEL.get(unit, BASE_LABEL)) # X axis labels (time) - inside the bottom margin, not clipped by the edge n = len(down_hist) diff --git a/beantester/gui/form.py b/beantester/gui/form.py index f2a38ff..efbe1ab 100644 --- a/beantester/gui/form.py +++ b/beantester/gui/form.py @@ -30,6 +30,7 @@ from . import dialogs from .accordion import CollapsibleSection from .labels import wrapping_label +from .rates import DEFAULT_UNIT, RATE_FIELD_KEYS, rate_with_unit from .scaling import scaled from .theme import popdown_height, unhighlight_combobox from .tooltip import add_tooltip @@ -107,6 +108,7 @@ def __init__(self, parent, app, extras=None, scroller=None, sections=None, self.errors = {} # section id -> error label (packed only when set) self.notes = {} # section id -> override note label self.helps = {} # settings key -> its "?" cheat-sheet button + self.rate_hints = {} # rate field key -> its converted-value label self._invalid = set() # section ids whose fields currently fail validation self.columns = 1 self.column_frames = [] @@ -114,6 +116,7 @@ def __init__(self, parent, app, extras=None, scroller=None, sections=None, self.host = ttk.Frame(parent) self.host.pack(fill="both", expand=True) self._build() + self.sync_rate_hints() self.host.bind("", self._on_host_configure) # -- construction -------------------------------------------------------- # @@ -121,6 +124,7 @@ def _build(self): app = self.app self.sections, self.entries, self.labels = {}, {}, {} self.errors, self.notes, self.helps = {}, {}, {} + self.rate_hints = {} # Real column FRAMES, not grid columns: in a grid the row height is # shared across columns, so one tall section on the left blew a hole @@ -293,9 +297,49 @@ def _place_one(self, row, field, sec): if field.hint: ttk.Label(cell, text=T(field.hint), style="Hint.TLabel").pack( side="left", padx=(scaled(8), 0)) + if field.key in RATE_FIELD_KEYS: + # The same number in the unit the reader picked. The ENTRY stays in + # KB/s because that is what a config file, the schedule string and the + # command line all carry (see gui/rates.py), so this is the one place + # that says what the typed number means in Mbit/s or MB/s. Empty while + # the preference IS KB/s: repeating the value beside itself is noise. + hint = ttk.Label(cell, text="", style="Hint.TLabel") + hint.pack(side="left", padx=(scaled(8), 0)) + self.rate_hints[field.key] = hint entry.bind("", lambda e, s=sec.id: self._on_edit(s), add="+") entry.bind("", lambda e, s=sec.id: self._on_edit(s), add="+") + if field.key in RATE_FIELD_KEYS: + entry.bind("", lambda e: self.sync_rate_hints(), add="+") + entry.bind("", lambda e: self.sync_rate_hints(), add="+") + + def sync_rate_hints(self): + """Rewrite "(= 8.39 Mbit/s)" beside every rate field. Cheap, and idempotent. + + Called on every keystroke in one of those fields and by ``App.set_pref`` + when the unit changes, rather than on a timer: the two things that can + move it are a typed character and a dropdown, and both are events. With + the base unit selected the labels go EMPTY - a value repeated beside + itself is noise, not information. + """ + unit = self.app.pref("rate_unit") + for key, label in self.rate_hints.items(): + if not label.winfo_exists(): + continue + var = self.app.vars.get(key) + if unit == DEFAULT_UNIT or var is None: + label.config(text="") + continue + # A half-typed value is not an error here: this readout is a comment on + # what is in the box, so while the box says "12x" it simply says nothing + # rather than colouring the field or logging. + try: + kbps = float(str(var.get()).strip() or 0) + except (TypeError, ValueError): + label.config(text="") + continue + label.config(text=T("fields.rate_converted").format( + value=rate_with_unit(kbps, unit))) # -- events -------------------------------------------------------------- # def _show_match_help(self): diff --git a/beantester/gui/pages/stats.py b/beantester/gui/pages/stats.py index 17e5d38..2fbaa9e 100644 --- a/beantester/gui/pages/stats.py +++ b/beantester/gui/pages/stats.py @@ -18,7 +18,7 @@ from ...views import sort_events from ..chart import draw_throughput_chart from ..labels import wrapping_label -from ..rates import average_kbps +from ..rates import average_kbps, format_rate, rate_with_unit, RATE_FIELD_KEYS, UNIT_LABEL from ..scaling import scaled from ..scrollable import ScrollableFrame from .. import scope @@ -135,6 +135,10 @@ def __init__(self, app, parent): self.nb.bind("<>", lambda e: self._on_subpage()) self.stat_labels = {} + self.cap_labels = {} + # Which unit the rate captions currently SAY, so the tick can rewrite them + # when the preference changes and skip the work when it has not. + self._unit_shown = None self.sess_labels = {} self._cells = [] self._grid_cols = 0 @@ -173,6 +177,7 @@ def _build_live(self, parent): style="StatCap.TLabel") caption.pack(padx=scaled(10), pady=(0, scaled(8)), anchor="w") self.stat_labels[key] = value + self.cap_labels[key] = caption for w in (cell, value, caption): add_tooltip(w, tip) self._attach_copy(w, value, "live") @@ -320,10 +325,16 @@ def session_text(self): def live_text(self): """The counter grid, same rule - `CELLS` is the source, units included.""" rows = [] + # The copied text says the same unit the screen does: this is what ends up + # pasted into a bug report, so a caption disagreeing with the number beside + # it travels further than the window it came from. + chosen = UNIT_LABEL.get(self.app.pref("rate_unit"), "") for key, cap, unit, _tip in CELLS: label = self.stat_labels.get(key) if label is None: continue + if key in RATE_FIELD_KEYS: + unit = chosen caption = T(cap) + (" (%s)" % unit if unit else "") rows.append("%s: %s" % (caption, label.cget("text"))) return "\n".join(rows) @@ -433,7 +444,8 @@ def draw_chart(self): self._chart_job = None try: draw_throughput_chart(self.canvas, self.app.down_hist, self.app.up_hist, - sample_interval_s=self.app.TICK_MS / 1000.0) + sample_interval_s=self.app.TICK_MS / 1000.0, + unit=self.app.pref("rate_unit")) except Exception as _exc: crashlog.note(_exc, "gui.pages.stats") @@ -487,13 +499,36 @@ def _sync_scope_note(self): note.config(text=T(SCOPE_NOTES[state])) retip(note, SCOPE_TIPS[state]) + def _sync_rate_captions(self, unit): + """Rewrite the "(KB/s)" in the throughput captions when the unit changes. + + On the tick rather than at build time, for the same reason the scope notes + are: the preference flips in another window while this page is already + built, and a caption naming a unit the number is no longer in is the exact + class of lie convention 5 is about. Memoised on the unit, so an unchanged + preference costs one comparison. + """ + if unit == self._unit_shown: + return + self._unit_shown = unit + label = UNIT_LABEL.get(unit, "") + with crashlog.quiet("gui.pages.stats"): + for key, cap, _unit, _tip in CELLS: + if key not in RATE_FIELD_KEYS: + continue + caption = self.cap_labels.get(key) + if caption is not None: + caption.config(text=T(cap) + (f" ({label})" if label else "")) + def refresh_counters(self): self._sync_scope_note() self._chart_frame.config(text=self._throughput_title()) snap = self.app.last_snapshot or {} rates = self.app.last_rates - self.stat_labels["down"].config(text=f"{rates[0]:.0f}") - self.stat_labels["up"].config(text=f"{rates[1]:.0f}") + unit = self.app.pref("rate_unit") + self.stat_labels["down"].config(text=format_rate(rates[0], unit)) + self.stat_labels["up"].config(text=format_rate(rates[1], unit)) + self._sync_rate_captions(unit) # `seen` is the only counter here with a scoped twin. The impairment # counters are already scoped by construction (nothing outside the target # can be impaired), and drop_overflow / drop_shutdown / drop_send stay on @@ -512,6 +547,7 @@ def refresh_session(self): app = self.app snap = app.last_snapshot or {} info = app.engine.session_info() + unit = app.pref("rate_unit") host, ipv4, ipv6 = host_identity() self.sess_labels["host"].config(text=host) self.sess_labels["private_ipv4"].config(text=ipv4) @@ -540,7 +576,8 @@ def refresh_session(self): self.sess_labels["driver_wait"].config( text=f"{waited:.2f} ms" if waited else "-") self.sess_labels["peak_rate"].config( - text=f"{app.peak_down:.0f} / {app.peak_up:.0f} KB/s") + text="%s / %s" % (format_rate(app.peak_down, unit), + rate_with_unit(app.peak_up, unit))) down_mb = bytes_to_mb(app.scoped_stat(snap, "bytes_in")) up_mb = bytes_to_mb(app.scoped_stat(snap, "bytes_out")) total_mb = round(down_mb + up_mb, 2) @@ -551,7 +588,7 @@ def refresh_session(self): total_bytes = (app.scoped_stat(snap, "bytes_in") + app.scoped_stat(snap, "bytes_out")) avg = average_kbps(total_bytes, elapsed) - self.sess_labels["avg_rate"].config(text=f"{avg:.0f} KB/s") + self.sess_labels["avg_rate"].config(text=rate_with_unit(avg, unit)) def refresh_events(self): events = self.app.engine.events_snapshot()[-300:] diff --git a/beantester/gui/panels/settings.py b/beantester/gui/panels/settings.py index 282c91a..12b001a 100644 --- a/beantester/gui/panels/settings.py +++ b/beantester/gui/panels/settings.py @@ -27,7 +27,8 @@ from ..accordion import CollapsibleSection from ..form import ControlForm from ..labels import wrapping_label -from ..prefs import ACTION, BOOL, NUMBER, PREF_GROUPS, PREFS_BY_KEY, prefs_in_section +from ..prefs import (ACTION, BOOL, CHOICE, NUMBER, PREF_GROUPS, PREFS_BY_KEY, + prefs_in_section) from ..scaling import scaled from ..scrollable import ScrollableFrame from ..theme import popdown_height, unhighlight_combobox @@ -248,6 +249,36 @@ def _build_pref_row(self, card, pref): add_tooltip(btn, pref.tip) return + if pref.kind == CHOICE: + # label | readonly combobox | hint. Readonly for the same reason the + # traffic filter is: the value is one of a fixed set, and a typable box + # invites a spelling this build has no branch for. + label = ttk.Label(row, text=T(pref.label), style="Card.TLabel") + label.pack(side="left", padx=(0, scaled(6))) + labels = [text for _, text in pref.choices] + by_label = {text: value for value, text in pref.choices} + current = app.pref(pref.key) + shown = next((text for value, text in pref.choices if value == current), + labels[0]) + var = tk.StringVar(value=shown) + box = ttk.Combobox(row, textvariable=var, values=labels, + state="readonly", width=pref.width) + box.pack(side="left") + # The dropdown carries the VALUE, never the label: the labels are what + # a person reads and the values are what ui.json and every reader of + # the preference see, and collapsing the two is how a stored setting + # starts depending on the interface language. + box.bind("<>", + lambda e, k=pref.key, v=var, m=by_label: + self._store(k, m.get(v.get())), add="+") + add_tooltip(box, pref.tip) + add_tooltip(label, pref.tip) + if pref.hint: + ttk.Label(row, text=T(pref.hint), style="Hint.TLabel").pack( + side="left", padx=(scaled(8), 0)) + self._pref_vars[pref.key] = var + return + # NUMBER: label | entry | unit | hint, with live validation like the form label = ttk.Label(row, text=T(pref.label), style="Card.TLabel") label.pack(side="left", padx=(0, scaled(6))) diff --git a/beantester/gui/prefs.py b/beantester/gui/prefs.py index 803d928..83b715e 100644 --- a/beantester/gui/prefs.py +++ b/beantester/gui/prefs.py @@ -19,9 +19,12 @@ """ from typing import Any, NamedTuple, Optional, Tuple +from . import rates + NUMBER = "number" # validated float/int, inclusive bounds BOOL = "bool" # checkbox ACTION = "action" # a button that runs App.() +CHOICE = "choice" # readonly combobox over Pref.choices class Pref(NamedTuple): @@ -35,6 +38,15 @@ class Pref(NamedTuple): hint: str = "" # i18n key of the greyed hint width: int = 8 action: str = "" # App method name for kind == ACTION + # ``(stored value, label shown)`` pairs for kind == CHOICE, in the order the + # dropdown lists them. The label is a LITERAL, not an i18n key, and that is + # the same exception ``fields.Field.unit`` already carries: it is here for + # choices whose labels are SYMBOLS rather than words - "KB/s", "Mbit/s" - which + # read the same in every language and would otherwise cost three identical + # translations each, in three files, for ever. A choice whose labels are WORDS + # does not belong here: give it i18n keys instead, or it will ship English to + # every reader. Guarded by tests/test_prefs.py. + choices: Tuple[Tuple[str, str], ...] = () # Id of a ``fields.Section`` that renders this pref through its ``extra`` # builder, INSTEAD of a preference group. For the rare pref that belongs # beside a registry field rather than with the other preferences: "Show only @@ -57,6 +69,20 @@ class Pref(NamedTuple): Pref("log_lines", NUMBER, "prefs.log_lines", "tips.log_lines", default=500, bounds=(50.0, 100000.0), unit_key="prefs.unit_lines", hint="prefs.log_lines_hint", width=10), + # DISPLAY only, and that is forced rather than chosen: KB/s is what the CLI + # flags, the config file, the schedule string, the shipped scenarios and the + # NDJSON `down_kbps`/`up_kbps` fields all carry, and several of those are + # frozen contracts. So this converts on the way to the screen and never on the + # way to a file - you still TYPE a limit in KB/s, and the readout beside the + # field says what that is in the unit you picked. + # + # A Pref rather than a registry field, by convention 42: it must survive a + # restart and must NOT get a CLI flag or ride inside a traffic config, because + # a config file describes the traffic and not the window looking at it. + Pref("rate_unit", CHOICE, "prefs.rate_unit", "tips.rate_unit", + default=rates.DEFAULT_UNIT, + choices=tuple((key, label) for key, label, _ in rates.RATE_UNITS), + width=10), # Default False = what the tool has always done: every captured packet is # counted and listed, and targeting only decides what gets IMPAIRED. Turning # it on narrows the VIEW, never the capture and never the impairment - the @@ -89,7 +115,8 @@ class Pref(NamedTuple): # Prefs that name a ``section`` are rendered there instead and must NOT appear # here - see ``SECTION_PREFS`` below. PREF_GROUPS = ( - ("prefs.group_view", ("chart_seconds", "log_lines", "show_control_search")), + ("prefs.group_view", ("rate_unit", "chart_seconds", "log_lines", + "show_control_search")), ("prefs.group_behaviour", ("confirm_close", "restore_profile", "reset_layout")), ) @@ -116,4 +143,10 @@ def coerce(pref, raw): lo, hi = pref.bounds or (float("-inf"), float("inf")) value = min(max(value, lo), hi) return int(value) if float(value).is_integer() else value + if pref.kind == CHOICE: + # A value this build does not offer falls back to the default rather than + # being kept: ui.json survives downgrades and hand-editing, and a stored + # value nothing can render would leave the dropdown showing a blank while + # every reader of the preference got a string it has no branch for. + return raw if raw in {value for value, _ in pref.choices} else pref.default return raw diff --git a/beantester/gui/rates.py b/beantester/gui/rates.py index 0e9d49f..740ff6d 100644 --- a/beantester/gui/rates.py +++ b/beantester/gui/rates.py @@ -26,6 +26,67 @@ from collections import deque from typing import Optional +from ..fields import FIELD_DEFS + +# -- units ------------------------------------------------------------------ # +# Everything inside this program counts throughput in KB/s, where K is 1024: the +# CLI flags, the config file, the schedule string, the shipped scenarios and the +# NDJSON `down_kbps`/`up_kbps` fields all carry that number, and several of them +# are frozen contracts. So the unit preference is a DISPLAY choice and nothing +# else - it converts on the way to the screen and never on the way to a file. +# +# 🔴 The Mbit/s factor is the one number here that surprises people, so it is +# derived in the open rather than typed: 1 KB/s is 1024 bytes, a byte is 8 bits, +# and a megabit is 1e6 bits (decimal, as every network interface and every ISP +# means it). That makes 1024 KB/s come out as 8.389 Mbit/s and NOT 8.0. Rounding +# it to 8 would be a 4.9% lie in the direction people already expect, which is +# exactly the kind of number that never gets questioned again. +BASE_LABEL = "KB/s" +RATE_UNITS = ( + ("kb", BASE_LABEL, 1.0), + ("mbit", "Mbit/s", 1024.0 * 8.0 / 1_000_000.0), + ("mb", "MB/s", 1.0 / 1024.0), +) +UNIT_FACTOR = {key: factor for key, _, factor in RATE_UNITS} +UNIT_LABEL = {key: label for key, label, _ in RATE_UNITS} +DEFAULT_UNIT = RATE_UNITS[0][0] + +# Which settings fields carry a throughput in the base unit. A VIEW over the +# field registry rather than a list of names, so a third rate field is picked up +# by declaring its unit and nothing here has to remember it. +RATE_FIELD_KEYS = tuple(f.key for f in FIELD_DEFS if f.unit == BASE_LABEL) + + +def in_unit(kbps, unit): + """A KB/s figure expressed in ``unit``. Unknown units read as the base one.""" + try: + value = float(kbps) + except (TypeError, ValueError): + return 0.0 + return value * UNIT_FACTOR.get(unit, 1.0) + + +def format_rate(kbps, unit): + """``kbps`` rendered in ``unit``, with the precision that unit needs. + + KB/s keeps the whole numbers it has always printed. The other two are smaller + numbers - 1024 KB/s is 1.0 MB/s - so printing them the same way would round a + real speed limit to "0" and make the readout look broken. Three significant + figures, which is what ``utils.human_bytes`` settled on for the same reason. + """ + value = in_unit(kbps, unit) + if unit == DEFAULT_UNIT: + return f"{value:.0f}" + if value >= 100: + return f"{value:.0f}" + return f"{value:.1f}" if value >= 10 else f"{value:.2f}" + + +def rate_with_unit(kbps, unit): + """``format_rate`` plus the unit, for the places that print both together.""" + return f"{format_rate(kbps, unit)} {UNIT_LABEL.get(unit, BASE_LABEL)}" + + WINDOW_S = 1.0 # average over this much time WARMUP_S = 0.8 # below this the window is too young to trust AVG_MIN_S = 0.5 # session average needs at least this much elapsed time diff --git a/lang/en.json b/lang/en.json index 051d751..201f64f 100644 --- a/lang/en.json +++ b/lang/en.json @@ -205,6 +205,7 @@ "fields.period": "Period:", "fields.port": "Port:", "fields.port_label": "port:", + "fields.rate_converted": "(= {value})", "fields.row_limit": "Row limit:", "fields.row_limit_hint": "0 = no limit", "fields.rst": "TCP reset (RST):", @@ -357,6 +358,7 @@ "prefs.group_view": "Display", "prefs.log_lines": "Log lines kept", "prefs.log_lines_hint": "older lines drop off", + "prefs.rate_unit": "Speed unit:", "prefs.reset_layout": "Reset window layout", "prefs.restore_profile": "Restore the last profile on startup", "prefs.scope_view": "Show only the targeted traffic", @@ -532,6 +534,7 @@ "tips.nat": "NAT mapping expiry: if a connection is silent longer than the given seconds, the next inbound packet is dropped (the mapping 'disappears'). Tests keep-alive. 0 = off.", "tips.peak_rate": "The highest throughput reached, averaged over a 1 s window (a speed limit shapes the average rate, so a shorter window would report bursts above the limit).", "tips.profiles": "Ready-made settings (presets, e.g. '3G network') and your own. Selecting one fills the fields above immediately.", + "tips.rate_unit": "Which unit the Statistics page, the chart and the readout next to the speed limits show throughput in. You still type the limits themselves in KB/s, because that is the number a saved configuration file and the command line carry.", "tips.reset_layout": "Forget the remembered window size and position, collapsed sections and table sorting, and recentre the window. Your settings and session are kept.", "tips.reset_now": "Reset all active TCP connections now (for ~3 s). UDP has no reset - use loss or a link outage for it. Works after START.", "tips.restore_profile": "When on, the tool reopens with the last profile you picked already filled in. It does not start a capture - you still press START.", diff --git a/lang/pl.json b/lang/pl.json index 1c9e554..97b81bb 100644 --- a/lang/pl.json +++ b/lang/pl.json @@ -205,6 +205,7 @@ "fields.period": "Okres:", "fields.port": "Port:", "fields.port_label": "port:", + "fields.rate_converted": "(= {value})", "fields.row_limit": "Limit wierszy:", "fields.row_limit_hint": "0 = bez limitu", "fields.rst": "Zrywanie TCP (RST):", @@ -357,6 +358,7 @@ "prefs.group_view": "Wygląd", "prefs.log_lines": "Trzymane linie logu", "prefs.log_lines_hint": "starsze linie znikają", + "prefs.rate_unit": "Jednostka prędkości:", "prefs.reset_layout": "Zresetuj układ okna", "prefs.restore_profile": "Przywróć ostatni profil przy starcie", "prefs.scope_view": "Pokazuj tylko ruch celu", @@ -532,6 +534,7 @@ "tips.nat": "Wygasanie mapowania NAT: jeśli połączenie milczy dłużej niż podana liczba sekund, kolejny pakiet przychodzący jest odrzucany (mapowanie 'znika'). Test keep-alive. 0 = wyłączone.", "tips.peak_rate": "Najwyższa osiągnięta przepustowość, uśredniona w oknie 1 s (limit prędkości kształtuje średnią, więc krótsze okno pokazywałoby skoki powyżej limitu).", "tips.profiles": "Gotowe zestawy ustawień (presety, np. 'Sieci 3G') oraz Twoje własne. Wybór od razu wypełnia pola powyżej.", + "tips.rate_unit": "Jednostka, w której Statystyki, wykres i odczyt obok limitów prędkości pokazują przepustowość. Same limity nadal wpisujesz w KB/s, bo to jest ta liczba, którą niesie zapisany plik konfiguracji i wiersz poleceń.", "tips.reset_layout": "Zapomnij zapamiętany rozmiar i pozycję okna, zwinięte sekcje i sortowanie tabel, i wyśrodkuj okno. Ustawienia i sesja zostają.", "tips.reset_now": "Zerwij teraz wszystkie aktywne połączenia TCP (na ~3 s). UDP nie ma zrywania - użyj strat albo przerwy w łączu. Działa po naciśnięciu START.", "tips.restore_profile": "Gdy włączone, narzędzie otwiera się z ostatnio wybranym profilem już wpisanym. Nie startuje przechwytywania - nadal klikasz START.", diff --git a/lang/zh.json b/lang/zh.json index 8969724..f1bd6f5 100644 --- a/lang/zh.json +++ b/lang/zh.json @@ -205,6 +205,7 @@ "fields.period": "周期:", "fields.port": "端口:", "fields.port_label": "端口:", + "fields.rate_converted": "(= {value})", "fields.row_limit": "行数上限:", "fields.row_limit_hint": "0 = 不限制", "fields.rst": "TCP 重置（RST）:", @@ -357,6 +358,7 @@ "prefs.group_view": "显示", "prefs.log_lines": "保留的日志行数", "prefs.log_lines_hint": "更旧的日志行会被移除", + "prefs.rate_unit": "速度单位：", "prefs.reset_layout": "重置窗口布局", "prefs.restore_profile": "启动时恢复上次配置方案", "prefs.scope_view": "仅显示目标流量", @@ -532,6 +534,7 @@ "tips.nat": "模拟 NAT 映射过期：连接静默时间超过指定秒数后，下一个入站数据包会被丢弃，相当于映射“消失”。用于测试保活机制。0 = 关闭。", "tips.peak_rate": "达到过的最高吞吐量，按 1 秒窗口取平均。限速会平滑平均速率，因此若使用更短窗口，可能看到高于限速值的瞬时突发。", "tips.profiles": "可直接使用的内置预设（例如“3G 网络”）以及你保存的配置方案。选择后会立即填入上方字段。", + "tips.rate_unit": "统计页、图表以及速度限制旁边的读数用哪种单位显示吞吐量。限速本身仍然以 KB/s 输入，因为保存的配置文件和命令行携带的就是这个数值。", "tips.reset_layout": "清除已记住的窗口大小和位置、折叠状态以及表格排序，并把窗口重新居中。你的设置和当前会话会保留。", "tips.reset_now": "立即重置全部活动 TCP 连接，持续约 3 秒。UDP 没有“重置”机制，请用丢包或链路中断模拟。启动会话后可用。", "tips.restore_profile": "启用后，工具启动时会自动填入上次选择的配置方案，但不会自动开始捕获，仍需点击“开始”。", diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index 6a82f2e..dcf9f16 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -2099,6 +2099,46 @@ "new": ' self._last_sent = getattr(self, "_last_sent", {True: -1, False: -1})', "test": "test_a_restarted_session_does_not_inherit_the_previous_high_water_mark", }, + { + # The comfortable answer: a megabit as 1024*1024 bits makes 1024 KB/s come + # out as exactly 8.00, which is the number a reader expects and is 4.9% + # wrong. Nothing but an assertion on the digits can catch it, because the + # wrong version looks MORE right than the correct one. + "label": "units: a megabit becomes binary, so 1024 KB/s reads 8.00", + "file": "beantester/gui/rates.py", + "old": ' ("mbit", "Mbit/s", 1024.0 * 8.0 / 1_000_000.0),', + "new": ' ("mbit", "Mbit/s", 1024.0 * 8.0 / 1_048_576.0),', + "test": "test_a_kilobyte_here_is_1024_bytes_and_a_megabit_is_a_million_bits", + }, + { + # Writing the LABEL into ui.json instead of the value. It survives the + # restart, matches no known unit, falls back to KB/s - and reads as "the + # preference does not stick" rather than as a bug in the write. + "label": "units: the dropdown stores its label instead of its value", + "file": "beantester/gui/panels/settings.py", + "old": " self._store(k, m.get(v.get())), add=\"+\")", + "new": " self._store(k, v.get()), add=\"+\")", + "test": "test_the_dropdown_stores_the_value_and_never_the_label", + }, + { + # The view over the registry replaced by a list of names - the drift this + # project keeps paying for, and invisible until a third rate field exists. + "label": "units: the rate fields become a hand-written list", + "file": "beantester/gui/rates.py", + "old": "RATE_FIELD_KEYS = tuple(f.key for f in FIELD_DEFS if f.unit == BASE_LABEL)", + "new": 'RATE_FIELD_KEYS = ("down",)', + "test": "test_the_rate_fields_are_a_view_over_the_registry_not_a_list_of_names", + }, + { + # "1024 KB/s" printed beside a box that says 1024. Harmless-looking, and + # the reason the readout exists at all is that it says something the box + # does not. + "label": "units: the converted readout repeats the value in the base unit", + "file": "beantester/gui/form.py", + "old": " if unit == DEFAULT_UNIT or var is None:", + "new": " if var is None:", + "test": "test_the_converted_readout_appears_only_when_there_is_something_to_convert", + }, ] # The runner's own check: a patch that cannot compile must be reported as BROKEN, not diff --git a/tests/test_rate_units.py b/tests/test_rate_units.py new file mode 100644 index 0000000..f6ecfe8 --- /dev/null +++ b/tests/test_rate_units.py @@ -0,0 +1,179 @@ +"""The speed-unit preference: a DISPLAY choice, and it may never become more. + +Why this file exists separately from ``test_prefs.py``: the preference is the +small half. The load-bearing half is that KB/s stays the only unit anything +DURABLE ever sees - the CLI flags, the config file, the schedule string, the +shipped scenarios and the NDJSON ``down_kbps``/``up_kbps`` fields all carry that +number, and several of them are frozen contracts. A unit switch that leaked into +any of them would silently rewrite what a saved file means, and the file would +still load. + +So the tests below come in two halves: the arithmetic (pure, and the Mbit/s +factor is the one number here people get wrong), and the boundary (what the +switch is not allowed to touch). +""" +from beantester import fields as F +from beantester.gui import rates +from beantester.gui.prefs import CHOICE, PREFS, PREFS_BY_KEY, coerce +from fakes import check +from gui_harness import run_gui + + +def test_a_kilobyte_here_is_1024_bytes_and_a_megabit_is_a_million_bits(): + """The conversion, with the number people expect written down as wrong. + + 1024 KB/s is 8.389 Mbit/s and NOT 8.0. K is 1024 in this program (the engine + multiplies by 1024 to get bytes per second), a byte is 8 bits, and a megabit + is a decimal million - which is what every network interface and every ISP + means by it. Rounding the answer to a comfortable 8 would be a 4.9% error in + exactly the direction a reader already expects, which is the kind of number + that never gets questioned again. + """ + check("1024 KB/s is 1.00 MB/s", rates.rate_with_unit(1024, "mb") == "1.00 MB/s", + f"({rates.rate_with_unit(1024, 'mb')})") + check("1024 KB/s is 8.39 Mbit/s, not 8.00", + rates.rate_with_unit(1024, "mbit") == "8.39 Mbit/s", + f"({rates.rate_with_unit(1024, 'mbit')})") + exact = 1024 * 1024 * 8 / 1_000_000 + check("and the factor is derived, not typed", + abs(rates.in_unit(1024, "mbit") - exact) < 1e-9, + f"({rates.in_unit(1024, 'mbit')} vs {exact})") + check("the base unit is a no-op", rates.in_unit(937, "kb") == 937) + check("an unknown unit reads as the base one rather than raising", + rates.in_unit(937, "furlongs per fortnight") == 937) + + +def test_the_small_units_keep_the_digits_the_base_unit_does_not_need(): + """256 KB/s must not print as "0 MB/s". + + KB/s has always printed whole numbers and still does. The other two are + smaller numbers for the same speed, so printing them the same way would round + a real, configured limit to zero and make the readout look broken rather than + small. + """ + check("256 KB/s is 0.25 MB/s", rates.format_rate(256, "mb") == "0.25", + f"({rates.format_rate(256, 'mb')})") + check("...and the base unit is still an integer", + rates.format_rate(256, "kb") == "256", f"({rates.format_rate(256, 'kb')})") + check("a big number drops the decimals rather than reading like a measurement", + rates.format_rate(1024 * 200, "mbit") == "1678", + f"({rates.format_rate(1024 * 200, 'mbit')})") + check("garbage in is 0, not an exception (this runs on a half-typed field)", + rates.format_rate("12x", "mbit") == "0.00") + + +def test_the_rate_fields_are_a_view_over_the_registry_not_a_list_of_names(): + """A third rate field must arrive here by declaring its unit, not by editing. + + The moment this becomes a hand-written tuple it starts drifting, which is the + failure mode this project keeps paying for. Derived means a new field with + ``unit="KB/s"`` gets its converted readout with no second edit. + """ + expected = tuple(f.key for f in F.FIELD_DEFS if f.unit == rates.BASE_LABEL) + check("the rate fields come out of the field registry", + rates.RATE_FIELD_KEYS == expected, + f"({rates.RATE_FIELD_KEYS} vs {expected})") + check("and today that is download and upload", + set(rates.RATE_FIELD_KEYS) == {"down", "up"}, + f"({rates.RATE_FIELD_KEYS})") + + +def test_every_choice_preference_defaults_to_one_of_its_own_choices(): + """A default outside the list renders as a blank dropdown and stores nothing.""" + choices = [p for p in PREFS if p.kind == CHOICE] + check("there is at least one CHOICE pref (else this test is vacuous)", choices) + for pref in choices: + values = [value for value, _ in pref.choices] + check(f"{pref.key}: the default is one of its choices", + pref.default in values, f"({pref.default!r} not in {values})") + check(f"{pref.key}: an unknown stored value falls back to the default", + coerce(pref, "something-a-later-build-removed") == pref.default) + check(f"{pref.key}: a known one survives", + coerce(pref, values[-1]) == values[-1]) + + +def test_the_unit_preference_never_reaches_the_engine_or_a_file(): + """The boundary, and the reason the switch is a Pref rather than a field. + + A registry field would get a CLI flag and ride inside a saved traffic config + (convention 42), so a config written with Mbit/s selected would carry a + display choice into a file that describes TRAFFIC - and the schedule string + beside it would still be in KB/s. This asserts the separation mechanically + instead of trusting that nobody moves it later. + """ + check("the unit is not a settings field", + "rate_unit" not in {f.key for f in F.FIELD_DEFS}) + from beantester.settings import DEFAULT_SETTINGS + check("...and not a settings key", "rate_unit" not in DEFAULT_SETTINGS) + from beantester.cli import build_arg_parser + flags = {a for action in build_arg_parser()._actions + for a in action.option_strings} + check("...and has no CLI flag", "--rate-unit" not in flags, f"({sorted(flags)[:3]}...)") + check("the rate FIELDS still declare the base unit, whatever is on screen", + all(F.FIELDS[k].unit == rates.BASE_LABEL for k in rates.RATE_FIELD_KEYS)) + + +def test_the_dropdown_stores_the_value_and_never_the_label(): + """The bug this kind invites: writing "Mbit/s" into ui.json instead of "mbit". + + The combobox shows labels and the store keeps values, so the two are one + mapping apart - and a label written to disk would survive a restart, fail to + match any known unit and silently fall back to KB/s, which reads as "the + preference does not stick" rather than as a bug in the write. + """ + run_gui(""" + panel = app.open_window("settings") + var = panel._pref_vars["rate_unit"] + + # Find the combobox this pref rendered and fire ITS binding, so the + # label -> value mapping under test is the production one rather than a + # copy of it written here. + from fake_tk import walk + boxes = [w for w in walk(panel.win) + if getattr(w, "kw", {}).get("textvariable") is var] + assert boxes, "the choice pref rendered no widget bound to its variable" + box = boxes[0] + assert list(box.kw.get("values")) == ["KB/s", "Mbit/s", "MB/s"], box.kw.get("values") + assert box.kw.get("state") == "readonly", box.kw.get("state") + + var.set("Mbit/s") + for handler in box.bindings.get("<>", []): + handler(None) + + assert app.pref("rate_unit") == "mbit", app.pref("rate_unit") + assert app.ui.get("pref.rate_unit") == "mbit", app.ui.get("pref.rate_unit") + """) + + +def test_the_converted_readout_appears_only_when_there_is_something_to_convert(): + """Beside the speed limits: empty in KB/s, filled otherwise, quiet on garbage. + + Repeating "1024 KB/s" next to a box that says 1024 is noise, so the base unit + shows nothing. A half-typed value shows nothing either: this readout is a + comment on the box, not a validator, and colouring the field or logging from + here would make it one. + """ + run_gui(""" + app.select_page("control") + form = app.form + assert set(form.rate_hints) == {"down", "up"}, sorted(form.rate_hints) + + app.vars["down"].set("1024") + app.set_pref("rate_unit", "kb") + assert form.rate_hints["down"].kw.get("text") == "", \\ + form.rate_hints["down"].kw.get("text") + + app.set_pref("rate_unit", "mbit") + shown = form.rate_hints["down"].kw.get("text") + assert "8.39" in shown and "Mbit/s" in shown, shown + + app.set_pref("rate_unit", "mb") + shown = form.rate_hints["down"].kw.get("text") + assert "1.00" in shown and "MB/s" in shown, shown + + # a value in the middle of being typed says nothing rather than erroring + app.vars["down"].set("12x") + form.sync_rate_hints() + assert form.rate_hints["down"].kw.get("text") == "", \\ + form.rate_hints["down"].kw.get("text") + """) From 97f21ff88bdb30c6d922e7476e2bd7746382f10e Mon Sep 17 00:00:00 2001 From: DonislawDev Date: Fri, 4 Sep 2026 02:05:57 +0200 Subject: [PATCH 5/5] fix(gui): let the page react to the unit, and keep app.py under its ceiling The reaction to the speed-unit preference was in App.set_pref, which put five logic lines into the one module that sits ON the file-size ratchet with zero headroom by construction - it went 1166 to 1171 and the full suite said so. The answer to a ceiling here is to move code out, not to raise the number, and this project already has the right home for it: pages.pref_changed broadcasts to whichever page cares, and its own docstring gives this exact reason for existing. - move the refresh to ControlPage.on_pref_changed, beside the one that shows and hides the search bar - point the test at the production path (set_pref then pref_changed) rather than calling the form directly: it was green through the move while the wiring was broken, which is the half worth guarding - one more mutation entry for that wiring, caught - annotate the three new rates.py helpers - that module is on the mypy strictness ratchet, so an unannotated def there is a red job - trim the user-facing changelog entry under the 100-word ceiling Verified: 1373 passed, ruff and mypy clean, GUI smoke OK, and the crash log is EMPTY after a full run - no fake drifted from a real interface and no guarded path went silent. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 8 +++----- beantester/gui/app.py | 13 ------------- beantester/gui/pages/control.py | 9 +++++++++ beantester/gui/rates.py | 16 +++++++++++----- tests/test_mutation_registry.py | 13 +++++++++++++ tests/test_rate_units.py | 19 +++++++++++++++---- 6 files changed, 51 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4166370..a845af8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,11 +23,9 @@ The format follows [Keep a Changelog](https://keepachangelog.com/); versions fol nothing at all, in either language. It now finds the card and all four fields in it. - **A speed unit you can pick: `KB/s`, `Mbit/s` or `MB/s` (Settings window).** The Statistics page, the chart, the session peak and average, and a grey readout beside Download and Upload all - follow it. It changes what you READ, never what you type - the limits stay in KB/s, because that - is the number a saved config file, the throughput schedule, the shipped scenarios, `--down`, - `--up` and the NDJSON output all carry. `K` here is 1024 and a megabit is a decimal million, so - 1024 KB/s reads as 8.39 Mbit/s rather than 8, which is the honest conversion and not a rounding - error. + follow it. It changes what you READ, never what you type - the limits stay in KB/s, which is what + a saved config file, the schedule, the scenarios, `--down`/`--up` and the NDJSON output carry. + `K` here is 1024 and a megabit is a decimal million, so 1024 KB/s reads as 8.39 Mbit/s, not 8. ## [0.6.0] - 2026-09-03 diff --git a/beantester/gui/app.py b/beantester/gui/app.py index f55a437..ae84951 100644 --- a/beantester/gui/app.py +++ b/beantester/gui/app.py @@ -720,19 +720,6 @@ def set_pref(self, key, value): unclean exit, unlike session state that is written on close).""" self.ui.set(prefs.ui_key(key), value) self.ui.persist() - if key == "rate_unit": - # PUSHED, not polled. The readout sits on the Control page while the - # dropdown that moves it is in another window, so waiting for the next - # rebuild would make the preference look ignored. Only the Control - # form is told: the settings surface holds no rate field (its sections - # are the table limit and the scope card), so the Settings window's own - # form has nothing to rewrite - and `rate_hints` is keyed off the - # registry, so a rate field moved there later would start arriving here - # rather than needing this line changed. - form = getattr(self, "form", None) - if form is not None: - with crashlog.quiet("gui.app"): - form.sync_rate_hints() def chart_samples(self): """Chart history length in samples, derived from the seconds preference and diff --git a/beantester/gui/pages/control.py b/beantester/gui/pages/control.py index c50c9d3..6f2b9c6 100644 --- a/beantester/gui/pages/control.py +++ b/beantester/gui/pages/control.py @@ -178,6 +178,15 @@ def on_pref_changed(self, key): """A preference was written in the Settings window (see gui/pages).""" if key == "show_control_search": self._sync_search_visibility() + elif key == "rate_unit": + # The converted readout beside Download and Upload. PUSHED rather than + # left to the next rebuild: the dropdown that moves it lives in another + # window, and a label still naming the old unit reads as the preference + # having been ignored. It belongs on the PAGE rather than in + # ``App.set_pref`` for the reason ``pref_changed`` gives about itself - + # gui/app.py sits ON the size ratchet with no headroom, so a reaction + # put there has to come straight back out. + self.form.sync_rate_hints() def _sync_search_visibility(self): """Bring the bar in or out, once per actual change.""" diff --git a/beantester/gui/rates.py b/beantester/gui/rates.py index 740ff6d..017baf8 100644 --- a/beantester/gui/rates.py +++ b/beantester/gui/rates.py @@ -24,7 +24,7 @@ anchor the window, so the span is always >= WINDOW once the session is warm. """ from collections import deque -from typing import Optional +from typing import Any, Optional from ..fields import FIELD_DEFS @@ -57,8 +57,14 @@ RATE_FIELD_KEYS = tuple(f.key for f in FIELD_DEFS if f.unit == BASE_LABEL) -def in_unit(kbps, unit): - """A KB/s figure expressed in ``unit``. Unknown units read as the base one.""" +def in_unit(kbps: Any, unit: str) -> float: + """A KB/s figure expressed in ``unit``. Unknown units read as the base one. + + ``Any`` rather than ``float`` deliberately: one caller is a half-typed entry + box, so this takes whatever the widget holds and answers 0.0 for anything that + is not a number. Narrowing the annotation would push that decision out to + three call sites (see ``format_rate``). + """ try: value = float(kbps) except (TypeError, ValueError): @@ -66,7 +72,7 @@ def in_unit(kbps, unit): return value * UNIT_FACTOR.get(unit, 1.0) -def format_rate(kbps, unit): +def format_rate(kbps: Any, unit: str) -> str: """``kbps`` rendered in ``unit``, with the precision that unit needs. KB/s keeps the whole numbers it has always printed. The other two are smaller @@ -82,7 +88,7 @@ def format_rate(kbps, unit): return f"{value:.1f}" if value >= 10 else f"{value:.2f}" -def rate_with_unit(kbps, unit): +def rate_with_unit(kbps: Any, unit: str) -> str: """``format_rate`` plus the unit, for the places that print both together.""" return f"{format_rate(kbps, unit)} {UNIT_LABEL.get(unit, BASE_LABEL)}" diff --git a/tests/test_mutation_registry.py b/tests/test_mutation_registry.py index dcf9f16..a4d239b 100644 --- a/tests/test_mutation_registry.py +++ b/tests/test_mutation_registry.py @@ -2139,6 +2139,19 @@ "new": " if var is None:", "test": "test_the_converted_readout_appears_only_when_there_is_something_to_convert", }, + { + # The WIRING rather than the label: the page stops reacting to the + # preference, so the readout keeps naming the unit you just changed away + # from until something else rebuilds the form. Worth its own entry because + # this reaction has already moved once (out of App.set_pref, which sits on + # the size ratchet) and the test had to be pointed at the real path before + # it could see the difference. + "label": "units: the Control page stops reacting to the unit preference", + "file": "beantester/gui/pages/control.py", + "old": " self.form.sync_rate_hints()", + "new": " pass", + "test": "test_the_converted_readout_appears_only_when_there_is_something_to_convert", + }, ] # The runner's own check: a patch that cannot compile must be reported as BROKEN, not diff --git a/tests/test_rate_units.py b/tests/test_rate_units.py index f6ecfe8..b06dec7 100644 --- a/tests/test_rate_units.py +++ b/tests/test_rate_units.py @@ -14,7 +14,7 @@ """ from beantester import fields as F from beantester.gui import rates -from beantester.gui.prefs import CHOICE, PREFS, PREFS_BY_KEY, coerce +from beantester.gui.prefs import CHOICE, PREFS, coerce from fakes import check from gui_harness import run_gui @@ -154,20 +154,31 @@ def test_the_converted_readout_appears_only_when_there_is_something_to_convert() here would make it one. """ run_gui(""" + from beantester.gui.pages import pref_changed + app.select_page("control") form = app.form assert set(form.rate_hints) == {"down", "up"}, sorted(form.rate_hints) + def pick(unit): + # The production path: the Settings window persists and then TELLS the + # pages (SettingsWindow._store -> pages.pref_changed). Calling + # form.sync_rate_hints() directly here would test the label and leave + # the WIRING unguarded - which is the half that broke when the reaction + # moved out of App.set_pref. + app.set_pref("rate_unit", unit) + pref_changed(app, "rate_unit") + app.vars["down"].set("1024") - app.set_pref("rate_unit", "kb") + pick("kb") assert form.rate_hints["down"].kw.get("text") == "", \\ form.rate_hints["down"].kw.get("text") - app.set_pref("rate_unit", "mbit") + pick("mbit") shown = form.rate_hints["down"].kw.get("text") assert "8.39" in shown and "Mbit/s" in shown, shown - app.set_pref("rate_unit", "mb") + pick("mb") shown = form.rate_hints["down"].kw.get("text") assert "1.00" in shown and "MB/s" in shown, shown