From 4d59e11c1e1bfbca025b78deb63270e466373cc2 Mon Sep 17 00:00:00 2001 From: Jens Ahrensfeld Date: Sat, 11 Jul 2026 13:27:20 +0200 Subject: [PATCH] sud: fail fast on malformed sud.json documents, one test per fixture Sud.load() now calls utils/sud_validate.py's validate_sud() before parsing: requires a non-empty 'steps' list, and for every resolved step that sets its own 'temperature', a numeric ramp.rate - the one genuinely unguarded crash site (ramp['rate'], no .get() fallback) hit in tasks/sud.py, components/sud_forecast.py and demo_sud.py the moment that step is reached, which for SudForecastEstimator can be as early as the Load itself. A refused load is now logged with the specific reason (previously silent). validate_sud() reuses Sud._parse_data() directly rather than reimplementing default-merging/step-building. tests/utils/test_sud_validate_fixtures.py covers 8 fixture docs plus all 6 real sude/*.json files; tests/components/sud/test_sud_load.py exercises the Sud.load() wiring itself (rejects, keeps prior schedule). Also reworked both this and test_config_fixtures.py to generate one dedicated test method per fixture file (dynamic setattr) instead of looping with subTest inside a single method - subTest iterations don't show up individually in `unittest -v`/IDE test runners, which made it look like far fewer fixtures were covered than actually are. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01F4oEE99rkKVU8L8tkzXWdG --- components/sud.py | 22 +++++- tests/components/sud/__init__.py | 0 tests/components/sud/test_sud_load.py | 48 +++++++++++ .../utils/sud_fixtures/empty_steps_list.json | 17 ++++ .../utils/sud_fixtures/missing_ramp_rate.json | 24 ++++++ .../utils/sud_fixtures/missing_steps_key.json | 16 ++++ .../sud_fixtures/ramp_rate_wrong_type.json | 29 +++++++ tests/utils/sud_fixtures/step_not_dict.json | 19 +++++ tests/utils/sud_fixtures/valid_minimal.json | 25 ++++++ .../utils/sud_fixtures/valid_multi_step.json | 43 ++++++++++ .../utils/sud_fixtures/valid_with_target.json | 32 ++++++++ tests/utils/test_config_fixtures.py | 32 +++++--- tests/utils/test_sud_validate_fixtures.py | 79 +++++++++++++++++++ utils/sud_validate.py | 33 ++++++++ 14 files changed, 408 insertions(+), 11 deletions(-) create mode 100644 tests/components/sud/__init__.py create mode 100644 tests/components/sud/test_sud_load.py create mode 100644 tests/utils/sud_fixtures/empty_steps_list.json create mode 100644 tests/utils/sud_fixtures/missing_ramp_rate.json create mode 100644 tests/utils/sud_fixtures/missing_steps_key.json create mode 100644 tests/utils/sud_fixtures/ramp_rate_wrong_type.json create mode 100644 tests/utils/sud_fixtures/step_not_dict.json create mode 100644 tests/utils/sud_fixtures/valid_minimal.json create mode 100644 tests/utils/sud_fixtures/valid_multi_step.json create mode 100644 tests/utils/sud_fixtures/valid_with_target.json create mode 100644 tests/utils/test_sud_validate_fixtures.py create mode 100644 utils/sud_validate.py diff --git a/components/sud.py b/components/sud.py index 83e8b1e..c28f7ac 100644 --- a/components/sud.py +++ b/components/sud.py @@ -140,12 +140,30 @@ class Sud(AttributeChange): a schedule does so explicitly itself, e.g. by writing save()'s result to one of the sude/*.json files), resetting to IDLE as if freshly loaded. Refused while a brew is in progress, or if data is - malformed - in either case the current schedule is left untouched.""" + malformed - in either case the current schedule is left untouched. + + validate_sud() catches the same malformed-document cases _parse_data() + itself would raise on, plus content _parse_data() happily resolves + but that would crash much later - a step setting its own 'temperature' + with no ramp.rate anywhere to inherit only blows up when tasks/sud.py + or SudForecastEstimator actually reaches that step (ramp['rate'] has + no .get() fallback), which for the forecast estimator can be as soon + as this very Load. Refusing it here instead, with the reason logged + (nothing else here logs *why* a load was refused), is strictly better + than a bare KeyError from inside a run or a worker-thread forecast.""" + # Imported here, not at module level: utils/sud_validate.py imports + # Sud from this very module to reuse _parse_data(), so a top-level + # import here would be circular. + from utils.sud_validate import validate_sud + from utils.config_validate import ConfigError + if self.state not in (SudState.IDLE, SudState.DONE): return False try: + validate_sud(data) parsed = self._parse_data(data, self._pot_config) - except (KeyError, TypeError): + except (KeyError, TypeError, ConfigError) as e: + self.log.warning("Refusing to load malformed sud document: {}".format(e)) return False (self.name, self.description, self.schedule, self.pot_mass, diff --git a/tests/components/sud/__init__.py b/tests/components/sud/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/tests/components/sud/test_sud_load.py b/tests/components/sud/test_sud_load.py new file mode 100644 index 0000000..91f6925 --- /dev/null +++ b/tests/components/sud/test_sud_load.py @@ -0,0 +1,48 @@ +import json +import os +import unittest + +from components.sud import Sud + +FIXTURES_DIR = os.path.join(os.path.dirname(__file__), '..', '..', 'utils', 'sud_fixtures') + + +def _load_fixture(name): + with open(os.path.join(FIXTURES_DIR, name)) as f: + return json.load(f) + + +class TestSudLoadRejectsMalformedDocs(unittest.TestCase): + """Sud.load() wires in utils/sud_validate.py's validate_sud() so a + malformed sud.json is refused (same False-returning contract as any + other load() failure - see components/sud.py's own docstring) instead + of parsing cleanly and only crashing much later, mid-run or during a + forecast simulation, on the unguarded ramp['rate'] lookup.""" + + def test_valid_doc_loads(self): + sud = Sud() + self.assertTrue(sud.load(_load_fixture('valid_multi_step.json'))) + self.assertEqual(sud.name, 'MultiStep') + + def test_missing_ramp_rate_is_refused(self): + sud = Sud() + self.assertFalse(sud.load(_load_fixture('missing_ramp_rate.json'))) + + def test_wrong_type_ramp_rate_is_refused(self): + sud = Sud() + self.assertFalse(sud.load(_load_fixture('ramp_rate_wrong_type.json'))) + + def test_empty_steps_is_refused(self): + sud = Sud() + self.assertFalse(sud.load(_load_fixture('empty_steps_list.json'))) + + def test_failed_load_leaves_previous_schedule_untouched(self): + sud = Sud() + self.assertTrue(sud.load(_load_fixture('valid_multi_step.json'))) + self.assertFalse(sud.load(_load_fixture('missing_ramp_rate.json'))) + self.assertEqual(sud.name, 'MultiStep') + self.assertEqual(len(sud.schedule), 3) + + +if __name__ == '__main__': + unittest.main() diff --git a/tests/utils/sud_fixtures/empty_steps_list.json b/tests/utils/sud_fixtures/empty_steps_list.json new file mode 100644 index 0000000..2ee9038 --- /dev/null +++ b/tests/utils/sud_fixtures/empty_steps_list.json @@ -0,0 +1,17 @@ +{ + "Name": "EmptySteps", + "default": { + "step": { + "descr": "Put description here", + "user_wait_for_continue": false, + "temperature": 0, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 0 + } + } + }, + "steps": [] +} diff --git a/tests/utils/sud_fixtures/missing_ramp_rate.json b/tests/utils/sud_fixtures/missing_ramp_rate.json new file mode 100644 index 0000000..1465890 --- /dev/null +++ b/tests/utils/sud_fixtures/missing_ramp_rate.json @@ -0,0 +1,24 @@ +{ + "Name": "MissingRampRate", + "pot": { + "grain_mass": 0, + "water_mass": 20 + }, + "default": { + "step": { + "descr": "Put description here", + "user_wait_for_continue": false, + "temperature": 0, + "hold": { + "duration": 0 + }, + "ramp": {} + } + }, + "steps": [ + { + "descr": "Heat up", + "temperature": 65 + } + ] +} diff --git a/tests/utils/sud_fixtures/missing_steps_key.json b/tests/utils/sud_fixtures/missing_steps_key.json new file mode 100644 index 0000000..f829cd9 --- /dev/null +++ b/tests/utils/sud_fixtures/missing_steps_key.json @@ -0,0 +1,16 @@ +{ + "Name": "NoSteps", + "default": { + "step": { + "descr": "Put description here", + "user_wait_for_continue": false, + "temperature": 0, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 0 + } + } + } +} diff --git a/tests/utils/sud_fixtures/ramp_rate_wrong_type.json b/tests/utils/sud_fixtures/ramp_rate_wrong_type.json new file mode 100644 index 0000000..45a45f9 --- /dev/null +++ b/tests/utils/sud_fixtures/ramp_rate_wrong_type.json @@ -0,0 +1,29 @@ +{ + "Name": "WrongType", + "pot": { + "grain_mass": 0, + "water_mass": 20 + }, + "default": { + "step": { + "descr": "Put description here", + "user_wait_for_continue": false, + "temperature": 0, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 0 + } + } + }, + "steps": [ + { + "descr": "Heat up", + "temperature": 65, + "ramp": { + "rate": "fast" + } + } + ] +} diff --git a/tests/utils/sud_fixtures/step_not_dict.json b/tests/utils/sud_fixtures/step_not_dict.json new file mode 100644 index 0000000..75d6c77 --- /dev/null +++ b/tests/utils/sud_fixtures/step_not_dict.json @@ -0,0 +1,19 @@ +{ + "Name": "StepNotDict", + "default": { + "step": { + "descr": "Put description here", + "user_wait_for_continue": false, + "temperature": 0, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 0 + } + } + }, + "steps": [ + "not a step object" + ] +} diff --git a/tests/utils/sud_fixtures/valid_minimal.json b/tests/utils/sud_fixtures/valid_minimal.json new file mode 100644 index 0000000..a09922b --- /dev/null +++ b/tests/utils/sud_fixtures/valid_minimal.json @@ -0,0 +1,25 @@ +{ + "Name": "Minimal", + "pot": { + "grain_mass": 0, + "water_mass": 20 + }, + "default": { + "step": { + "descr": "Put description here", + "user_wait_for_continue": false, + "temperature": 0, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 0 + } + } + }, + "steps": [ + { + "descr": "Only step" + } + ] +} diff --git a/tests/utils/sud_fixtures/valid_multi_step.json b/tests/utils/sud_fixtures/valid_multi_step.json new file mode 100644 index 0000000..b6387d5 --- /dev/null +++ b/tests/utils/sud_fixtures/valid_multi_step.json @@ -0,0 +1,43 @@ +{ + "Name": "MultiStep", + "pot": { + "grain_mass": 0, + "water_mass": 22 + }, + "default": { + "step": { + "descr": "Put description here", + "user_wait_for_continue": false, + "temperature": 0, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 0 + } + } + }, + "steps": [ + { + "descr": "Aufheizen", + "temperature": 57, + "ramp": { + "rate": 1.5 + } + }, + { + "descr": "Rast", + "temperature": 63, + "hold": { + "duration": 40 + } + }, + { + "descr": "Ausmaischen", + "temperature": 78, + "ramp": { + "rate": 1.0 + } + } + ] +} diff --git a/tests/utils/sud_fixtures/valid_with_target.json b/tests/utils/sud_fixtures/valid_with_target.json new file mode 100644 index 0000000..8e7733e --- /dev/null +++ b/tests/utils/sud_fixtures/valid_with_target.json @@ -0,0 +1,32 @@ +{ + "Name": "WithTarget", + "pot": { + "grain_mass": 0, + "water_mass": 20 + }, + "default": { + "step": { + "descr": "Put description here", + "user_wait_for_continue": false, + "temperature": 0, + "ramp": { + "rate": 1.0 + }, + "hold": { + "duration": 0 + } + } + }, + "steps": [ + { + "descr": "Heat up", + "temperature": 65 + }, + { + "descr": "Hold", + "hold": { + "duration": 20 + } + } + ] +} diff --git a/tests/utils/test_config_fixtures.py b/tests/utils/test_config_fixtures.py index 73cad2d..b7310f5 100644 --- a/tests/utils/test_config_fixtures.py +++ b/tests/utils/test_config_fixtures.py @@ -1,5 +1,6 @@ import json import os +import re import unittest from utils.config_validate import ConfigError, validate_temp_ctrl @@ -27,20 +28,33 @@ def _load(name): return json.load(f) +def _test_name(filename): + return 'test_' + re.sub(r'\W', '_', filename[:-len('.json')]) + + class TestConfigFixtures(unittest.TestCase): def test_every_fixture_file_is_covered(self): on_disk = {f for f in os.listdir(FIXTURES_DIR) if f.endswith('.json')} self.assertEqual(on_disk, set(CASES), "fixtures/ and CASES have drifted apart") - def test_fixtures_match_expected_outcome(self): - for name, expected_error in CASES.items(): - with self.subTest(fixture=name): - config = _load(name) - if expected_error is None: - validate_temp_ctrl(config) # must not raise - else: - with self.assertRaisesRegex(ConfigError, expected_error): - validate_temp_ctrl(config) + +def _make_fixture_test(name, expected_error): + def test(self): + config = _load(name) + if expected_error is None: + validate_temp_ctrl(config) # must not raise + else: + with self.assertRaisesRegex(ConfigError, expected_error): + validate_temp_ctrl(config) + return test + + +# One dedicated test method per fixture file - not a single method looping +# over CASES with subTest - so `unittest discover -v` (and any IDE test +# runner) lists and can re-run each fixture individually instead of folding +# them all into one opaque "did any of these fail" result. +for _name, _expected_error in CASES.items(): + setattr(TestConfigFixtures, _test_name(_name), _make_fixture_test(_name, _expected_error)) if __name__ == '__main__': diff --git a/tests/utils/test_sud_validate_fixtures.py b/tests/utils/test_sud_validate_fixtures.py new file mode 100644 index 0000000..4638e31 --- /dev/null +++ b/tests/utils/test_sud_validate_fixtures.py @@ -0,0 +1,79 @@ +import json +import os +import re +import unittest + +from utils.config_validate import ConfigError +from utils.sud_validate import validate_sud + +FIXTURES_DIR = os.path.join(os.path.dirname(__file__), 'sud_fixtures') +SUDE_DIR = os.path.join(os.path.dirname(__file__), '..', '..', 'sude') + +# filename -> None if the fixture should pass validation, otherwise a regex +# matched against the raised ConfigError's message. +CASES = { + 'valid_minimal.json': None, + 'valid_with_target.json': None, + 'valid_multi_step.json': None, + 'missing_steps_key.json': r"missing required config key: steps", + 'empty_steps_list.json': r"'steps' must be a non-empty list", + 'missing_ramp_rate.json': r'step 1 \(Heat up\): .*ramp\.rate', + 'ramp_rate_wrong_type.json': r'step 1 \(Heat up\): .*ramp\.rate', + 'step_not_dict.json': r'malformed sud document', +} + + +def _load(directory, name): + with open(os.path.join(directory, name)) as f: + return json.load(f) + + +def _test_name(filename): + return 'test_' + re.sub(r'\W', '_', filename[:-len('.json')]) + + +class TestSudValidateFixtures(unittest.TestCase): + def test_every_fixture_file_is_covered(self): + on_disk = {f for f in os.listdir(FIXTURES_DIR) if f.endswith('.json')} + self.assertEqual(on_disk, set(CASES), "sud_fixtures/ and CASES have drifted apart") + + +def _make_fixture_test(name, expected_error): + def test(self): + data = _load(FIXTURES_DIR, name) + if expected_error is None: + validate_sud(data) # must not raise + else: + with self.assertRaisesRegex(ConfigError, expected_error): + validate_sud(data) + return test + + +# One dedicated test method per fixture file - not a single method looping +# over CASES with subTest - so `unittest discover -v` (and any IDE test +# runner) lists and can re-run each fixture individually instead of folding +# them all into one opaque "did any of these fail" result. +for _name, _expected_error in CASES.items(): + setattr(TestSudValidateFixtures, _test_name(_name), _make_fixture_test(_name, _expected_error)) + + +class TestRealSudFiles(unittest.TestCase): + """The actual sude/*.json schedules must always pass - a failure here + means either a real schedule is broken, or validate_sud() itself has + drifted from what Sud._parse_data() actually accepts.""" + + +def _make_real_file_test(name): + def test(self): + validate_sud(_load(SUDE_DIR, name)) + return test + + +_real_sud_files = [f for f in os.listdir(SUDE_DIR) if f.endswith('.json')] +assert _real_sud_files, "expected at least one sude/*.json file" +for _name in _real_sud_files: + setattr(TestRealSudFiles, _test_name(_name), _make_real_file_test(_name)) + + +if __name__ == '__main__': + unittest.main() diff --git a/utils/sud_validate.py b/utils/sud_validate.py new file mode 100644 index 0000000..8d202ad --- /dev/null +++ b/utils/sud_validate.py @@ -0,0 +1,33 @@ +from utils.config_validate import ConfigError, require +from components.sud import Sud + + +def validate_sud(data): + """Fail-fast structural check for a sud.json document - same rationale + as config_validate.py's validate_temp_ctrl(): a malformed field here + doesn't surface until deep inside a run, or worse, during + SudForecastEstimator's very first Load-time simulation (tasks/sud.py, + components/sud_forecast.py and scripts/demos/sud/demo_sud.py all read + ramp['rate'] directly, no .get() fallback, the moment any step actually + sets its own 'temperature') - as a bare KeyError with no indication of + which schedule step or field is wrong. + + Reuses Sud._parse_data() itself (rather than re-implementing default- + merging/step-building) so what's checked here can never drift from what + actually gets parsed at runtime.""" + require(data, 'steps', type=list) + if not data['steps']: + raise ConfigError("'steps' must be a non-empty list") + + try: + schedule = Sud._parse_data(data)[2] + except (KeyError, TypeError, AttributeError) as e: + raise ConfigError("malformed sud document: {}".format(e)) + + for step in schedule: + if step['temperature'] is None: + continue + try: + require(step, 'ramp.rate', type=(int, float)) + except ConfigError as e: + raise ConfigError("step {} ({}): {}".format(step['number'], step.get('descr'), e))