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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01F4oEE99rkKVU8L8tkzXWdG

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,

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()

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": []
+}

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
+		}
+	]
+}

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
+			}
+		}
+	}
+}

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"
+			}
+		}
+	]
+}

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"
+	]
+}

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"
+		}
+	]
+}

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
+			}
+		}
+	]
+}

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
+			}
+		}
+	]
+}

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):
+
+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__':

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()

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))