You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Follow-up from the code review on #224 (issue #219, Battery/Home settings unification). Two pairs of near-identical test classes were added to backend/tests/test_settings_contracts.py to mirror what #216 did for Price settings — flagged by the review as copy-paste that should eventually be a shared helper, but not worth blocking #224 on.
The problem
TestBatteryRequiredFieldsConsistency and TestHomeRequiredFieldsConsistency are copy-paste of each other, differing only in the dataclass name, the exclusion set's name/contents (has_default vs internal_only), and the error string.
TestBatterySettingsRoundTrip and TestHomeSettingsRoundTrip repeat the same three-step shape (mutate a fresh store dict → run through build_system_settings → build the "patch" side manually → compare BSM state) in two ~30-line classes. Genuinely different in one place (battery filters through _BATTERY_MODEL_ATTRS, home passes through unfiltered), but the store-setup/instantiate/compare boilerplate around that difference is duplicated.
When a third settings class needs the same "required fields match dataclass" or "startup/PATCH round-trip" guard (e.g. once #216 merges Price into this pattern), the copy-paste is likely to happen a third time, and any improvement to the assertion/error message has to be repeated in every copy.
Proposed fix
Extract a parametrized helper (or a single test taking (dataclass, required_fields, exclusions)) for the *RequiredFieldsConsistency pair.
Extract shared setup/compare helpers for the *SettingsRoundTrip pair, keeping the one genuinely different line (battery's _BATTERY_MODEL_ATTRS filter vs home's unfiltered passthrough) inline per class.
Background
Follow-up from the code review on #224 (issue #219, Battery/Home settings unification). Two pairs of near-identical test classes were added to
backend/tests/test_settings_contracts.pyto mirror what #216 did for Price settings — flagged by the review as copy-paste that should eventually be a shared helper, but not worth blocking #224 on.The problem
TestBatteryRequiredFieldsConsistencyandTestHomeRequiredFieldsConsistencyare copy-paste of each other, differing only in the dataclass name, the exclusion set's name/contents (has_defaultvsinternal_only), and the error string.TestBatterySettingsRoundTripandTestHomeSettingsRoundTriprepeat the same three-step shape (mutate a fresh store dict → run throughbuild_system_settings→ build the "patch" side manually → compare BSM state) in two ~30-line classes. Genuinely different in one place (battery filters through_BATTERY_MODEL_ATTRS, home passes through unfiltered), but the store-setup/instantiate/compare boilerplate around that difference is duplicated.TestBatterySettingsRoundTrip/TestHomeSettingsRoundTripsays "mirrors arch: unify settings update paths to eliminate PRICE_STORE_TO_API translation registry #197'sTestPriceSettingsRoundTrip" — that class doesn't exist anywhere in this repo yet (verified via full-tree grep). It only exists on the separate, unmerged#216branch. Not factually wrong (it's a valid reference to prior art on another branch) but worth rewording once fix: unify Battery/Home/Price settings startup/PATCH paths, fix live efficiency-field bug (#197) #216 merges so a reader grepping for it doesn't waste time wondering if it was deleted.Why it matters
When a third settings class needs the same "required fields match dataclass" or "startup/PATCH round-trip" guard (e.g. once #216 merges Price into this pattern), the copy-paste is likely to happen a third time, and any improvement to the assertion/error message has to be repeated in every copy.
Proposed fix
(dataclass, required_fields, exclusions)) for the*RequiredFieldsConsistencypair.*SettingsRoundTrippair, keeping the one genuinely different line (battery's_BATTERY_MODEL_ATTRSfilter vs home's unfiltered passthrough) inline per class.TestPriceSettingsRoundTripcomment reference (or fold Price into the same parametrized structure).Files affected
backend/tests/test_settings_contracts.py