From cd52260cafd29e465a5417b2e7ad9276f02ddecf Mon Sep 17 00:00:00 2001 From: =?utf8?q?J=C3=A9r=C3=B4me=20Benoit?= Date: Fri, 2 Oct 2026 10:42:40 +0200 Subject: [PATCH] fix(reforcexy): close the review findings on the gate and its harness MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Two of the guards could not fail on the defect they name. The measured-file guard filtered registrations by the dotted prefix, which discarded exactly the bare-stem duplicate its own comment called out as the threat, so it stayed green with the shadow copy live. It now counts every registration shape-agnostically, and a second guard rejects any measured file registered under a bare stem. Both walks were also one level too high: they scanned `ReforceXY/` and so matched the out-of-scope reward-space modules. The shuffle guard asserted the identity of the comparator the harness had just installed, which a constant comparator would satisfy. It now asserts the ORDER the loader produces — alphabetical exactly when no seed is set, and not alphabetical when one is — with the seeded comparator rebuilt rather than read out of the module, so the seed is actually exercised. Also from review: the hyperopt eval-frequency test was a fixed point that certified a reduction it never performed, so deleting the reduction left it green; the net-arch tests asserted a key set and a type where the names claimed widths; the numpy restore compared one element of a five-tuple and could not see an advanced stream position; the CI step was named for an ordering it cannot permute, since unittest sorts module and class order alphabetically and consults no hook for either; and the sampler-pin comment claimed a guarantee two wholesale call sites void. 92 tests green under four orderings; gate exits 0 at 57.1%; ruff clean; basedpyright snapshot unchanged. --- .github/workflows/quality.yml | 2 +- README.md | 8 +++ ReforceXY/tests/qa_support.py | 10 +-- .../test_model_schedules_params_and_net.py | 27 ++++++-- ReforceXY/tests/test_suite_contract.py | 69 ++++++++++++++++--- 5 files changed, 97 insertions(+), 19 deletions(-) diff --git a/.github/workflows/quality.yml b/.github/workflows/quality.yml index 668b0c3..a35a398 100644 --- a/.github/workflows/quality.yml +++ b/.github/workflows/quality.yml @@ -87,7 +87,7 @@ jobs: --entrypoint sh \ "${{ matrix.image }}" \ /workspace/scripts/run-coverage.sh -s "${{ matrix.context }}/tests" -v - - name: Run runtime regressions under three orderings + - name: Run runtime regressions under three method orderings if: matrix.context == 'ReforceXY' run: | docker run --rm \ diff --git a/README.md b/README.md index 757f4cd..b088739 100644 --- a/README.md +++ b/README.md @@ -564,6 +564,14 @@ denominator without the trade-off being visible: (`:2244-2245`), since `_Samplers` has exactly two members and `:2218` raises for anything else. +CI additionally runs the ReforceXY suite three times with +`FREQAI_QA_SHUFFLE_SEED=1|2|3`. The seed reorders test METHODS within each +class — `unittest` sorts both module and class order alphabetically and +consults no hook for either, so cross-module ordering is not permuted. A leaked +global is still caught: `QaTestCase` restores the process globals in `setUp` +and again in `addCleanup`, so every test starts from the import-time state +whatever ran before it. + `fail_under` is an absolute floor, never per-module. To change it, re-measure with the shipped configuration already in place — a measurement taken without `branch` and `include_namespace_packages` reports a different denominator and diff --git a/ReforceXY/tests/qa_support.py b/ReforceXY/tests/qa_support.py index c69a6c6..2de08e2 100644 --- a/ReforceXY/tests/qa_support.py +++ b/ReforceXY/tests/qa_support.py @@ -70,10 +70,12 @@ _BASE_CONFIG: dict[str, Any] = { "add_state_info": False, }, # `create_sampler` resolves `auto` through optunahub.load_module, which is an - # outbound fetch to hub.optuna.org, so the sampler is pinned for every test. The - # pin is only half the guarantee: `create_sampler` also defaults to "tpe" and - # raises for anything outside `_Samplers`, so no config value reaches the network. - # There is no `pruner` key: `create_pruner` is a staticmethod taking three ints. + # outbound fetch to hub.optuna.org, so the sampler is pinned here. The pin holds + # only while callers go through `**overrides`: two tests assign `rl_config_optuna` + # wholesale and would discard it. The guarantee that survives that is in the + # production default — `create_sampler` falls back to "tpe" and raises for anything + # outside `_Samplers`, so no config value reaches the network either way. There is + # no `pruner` key: `create_pruner` is a staticmethod taking three ints. "rl_config_optuna": {"enabled": False, "sampler": "tpe"}, }, } diff --git a/ReforceXY/tests/test_model_schedules_params_and_net.py b/ReforceXY/tests/test_model_schedules_params_and_net.py index 07f2a0d..ed8625e 100644 --- a/ReforceXY/tests/test_model_schedules_params_and_net.py +++ b/ReforceXY/tests/test_model_schedules_params_and_net.py @@ -40,7 +40,7 @@ class LinearScheduleTest(QaTestCase): self.assertEqual(0.0, schedule(-0.5)) def test_it_reports_the_value_it_was_built_from(self): - self.assertIn("0.5", repr(SimpleLinearSchedule(0.5))) + self.assertEqual("SimpleLinearSchedule(initial_value=0.5)", repr(SimpleLinearSchedule(0.5))) class ScheduleClassificationTest(QaTestCase): @@ -63,7 +63,10 @@ class ScheduleClassificationTest(QaTestCase): self.assertTrue(math.isnan(final)) def test_a_linear_request_builds_the_linear_schedule(self): - self.assertIsInstance(get_schedule(LINEAR, 0.1), SimpleLinearSchedule) + schedule = get_schedule(LINEAR, 0.1) + self.assertIsInstance(schedule, SimpleLinearSchedule) + self.assertEqual(0.1, schedule(1.0)) + self.assertEqual(0.0, schedule(0.0)) def test_every_other_request_builds_a_constant_schedule(self): for kind in (CONSTANT, UNKNOWN): @@ -108,16 +111,21 @@ class GradientStepTest(QaTestCase): class ArchitectureTest(QaTestCase): - def test_the_on_policy_family_gets_a_separate_actor_and_critic_width(self): + def test_the_on_policy_family_returns_an_entry_per_network_and_shares_one_width(self): + # Production gives pi and vf the SAME width at every size, so the invariant is that + # they agree, not that they differ; a test named for a separation that does not + # exist would pass on the wrong widths. for size in ReforceXY._NET_ARCH_SIZES: with self.subTest(size=size): arch = get_net_arch("MaskablePPO", size) self.assertEqual({"pi", "vf"}, set(arch)) + self.assertEqual(arch["pi"], arch["vf"]) + self.assertEqual([128, 128], arch["pi"]) if size == "small" else None def test_the_value_based_family_gets_a_shared_width_list(self): for model_type in ("DQN", "QR-DQN"): with self.subTest(model_type=model_type): - self.assertIsInstance(get_net_arch(model_type, "small"), list) + self.assertEqual([128, 128], get_net_arch(model_type, "small")) def test_a_grown_size_is_wider_than_a_small_one(self): self.assertGreater( @@ -175,11 +183,18 @@ class EvalFrequencyTest(QaTestCase): def test_the_value_based_family_uses_the_evaluation_step_budget(self): self.assertEqual(16, self._model(model_type="DQN", n_eval_steps=16).get_eval_freq(16_000)) - def test_hyperopt_reduces_the_interval_but_never_below_one(self): + def test_hyperopt_reduces_the_interval_by_the_configured_factor(self): + model = self._model(model_type="DQN", n_eval_steps=16) + self.assertEqual(16, model.get_eval_freq(16_000)) self.assertEqual( - 1, self._model(model_type="DQN", n_eval_steps=1).get_eval_freq(16_000, hyperopt=True) + round(16 / ReforceXY._HYPEROPT_EVAL_FREQ_REDUCTION_FACTOR), + model.get_eval_freq(16_000, hyperopt=True), ) + def test_the_reduced_interval_never_falls_below_one(self): + model = self._model(model_type="DQN", n_eval_steps=1) + self.assertEqual(1, model.get_eval_freq(16_000, hyperopt=True)) + if __name__ == "__main__": unittest.main() diff --git a/ReforceXY/tests/test_suite_contract.py b/ReforceXY/tests/test_suite_contract.py index cc5202b..3585a49 100644 --- a/ReforceXY/tests/test_suite_contract.py +++ b/ReforceXY/tests/test_suite_contract.py @@ -1,9 +1,12 @@ """Guards the import identity and coverage denominator of the measured source.; requires the Freqtrade QA image.""" +import hashlib import importlib +import os import random import sys import unittest +from functools import cmp_to_key from pathlib import Path import numpy as np @@ -44,6 +47,17 @@ for _name in PRODUCTION_NAMES: importlib.import_module(_name) +def _seeded_cmp(seed: str): + """The comparator qa_support builds for a given seed, rebuilt without touching _SEED.""" + + def compare(a: str, b: str) -> int: + ka = int.from_bytes(hashlib.blake2b(f"{seed}:{a}".encode(), digest_size=8).digest()) + kb = int.from_bytes(hashlib.blake2b(f"{seed}:{b}".encode(), digest_size=8).digest()) + return (ka > kb) - (ka < kb) + + return compare + + def _source_files_under(root: Path) -> list[tuple[str, Path]]: """Registered modules whose `__file__` is a `.py` under `root`, as (name, resolved path).""" found = [] @@ -58,22 +72,32 @@ def _source_files_under(root: Path) -> list[tuple[str, Path]]: class SuiteContractTest(unittest.TestCase): - def test_each_measured_source_file_is_registered_under_exactly_one_dotted_name(self): - registered = [ - path - for name, path in _source_files_under(MEASURED_TREE.parent) - if name.startswith("ReforceXY.user_data") - ] + def test_each_measured_source_file_is_registered_under_exactly_one_name(self): + # Shape-agnostic on purpose: filtering by the dotted prefix would discard the + # bare-stem registration this very comment calls out as the threat, and the guard + # would stay green with the shadow copy live. + registered = [path for _, path in _source_files_under(MEASURED_TREE)] for src in _SOURCES: with self.subTest(module=src.name): matches = [path for path in registered if path == src.resolve()] self.assertEqual( 1, len(matches), - "a measured file under exactly one dotted name; a second registration is a " + "a measured file under exactly one module name; a second registration is a " "shadow copy carrying its own module state", ) + def test_no_measured_file_is_registered_under_a_bare_stem(self): + # CI puts `user_data/strategies` on PYTHONPATH, so a bare `import RLAgentStrategy` + # would resolve. A dotted name is the only one the production import uses. + for name, _ in _source_files_under(MEASURED_TREE): + with self.subTest(module=name): + self.assertTrue( + name.startswith("ReforceXY.user_data."), + "a measured file registered under a bare stem has a second module object, " + "and every process-global restore would clean the copy under test never uses", + ) + def test_every_test_module_is_registered_under_its_bare_stem(self): # The half that catches a shadow copy of the test tree itself. A measured-tree walk # cannot observe `ReforceXY/tests`, so a dotted `from ReforceXY.tests.qa_support @@ -122,11 +146,34 @@ class SuiteContractTest(unittest.TestCase): "process globals it restores leak between tests", ) - def test_the_shuffle_is_installed_on_the_loader(self): + def test_the_order_is_alphabetical_exactly_when_no_seed_is_set(self): # `sortTestMethodsUsing` is a staticmethod on the LOADER; a TestCase has no such # hook, and a plain function would bind as a method and raise TypeError at discovery. + # What is asserted here is the ORDER the loader produces, not the identity of the + # function it holds — a constant comparator installed by the same line would satisfy + # an identity check and defeat the shuffle. + names = ["test_c", "test_a", "test_b", "test_d"] + ordered = sorted(names, key=cmp_to_key(_shuffle_cmp)) + if os.environ.get("FREQAI_QA_SHUFFLE_SEED", ""): + # A seeded run must NOT be alphabetical; the other guard in this class + # proves it is deterministic for a given seed. + self.assertNotEqual(sorted(names), ordered) + else: + self.assertEqual(sorted(names), ordered) self.assertIs(_shuffle_cmp, unittest.TestLoader.sortTestMethodsUsing) + def test_a_seeded_order_differs_from_alphabetical(self): + names = ["test_c", "test_a", "test_b", "test_d"] + alphabetical = sorted(names) + for seed in ("1", "2", "3"): + with self.subTest(seed=seed): + shuffled = sorted(names, key=cmp_to_key(_seeded_cmp(seed))) + self.assertNotEqual( + alphabetical, + shuffled, + f"seed {seed} left the method order alphabetical, so the shuffle is inert", + ) + def test_the_restore_puts_the_process_globals_back(self): cache = _PRODUCTION.ReforceXY._action_masks_cache # Dirty every global the way a test that trains a model would. @@ -147,6 +194,12 @@ class SuiteContractTest(unittest.TestCase): self.assertEqual( _NUMPY_STATE_AT_IMPORT[1].tolist(), np.random.get_state()[1].tolist(), + "the numpy bit generator was not rewound", + ) + self.assertEqual( + int(_NUMPY_STATE_AT_IMPORT[2]), + int(np.random.get_state()[2]), + "the numpy stream position advanced and was not restored", ) if _TORCH_STATE_AT_IMPORT is not None: self.assertTrue(torch.equal(torch.random.get_rng_state(), _TORCH_STATE_AT_IMPORT)) -- 2.53.0