From a2cb09244ec9926b9227f0623eab6eec7f6c1c89 Mon Sep 17 00:00:00 2001 From: Anthony Volk Date: Wed, 8 Jul 2026 20:56:41 +0200 Subject: [PATCH 1/6] Add characterization tests for Variable.clone() Lock the existing clone() contract for normal variables (attributes preserved, same class, independent mutable containers) before fixing #502. Co-Authored-By: Claude Opus 4.8 --- tests/core/variables/test_variable_clone.py | 37 +++++++++++++++++++++ 1 file changed, 37 insertions(+) create mode 100644 tests/core/variables/test_variable_clone.py diff --git a/tests/core/variables/test_variable_clone.py b/tests/core/variables/test_variable_clone.py new file mode 100644 index 00000000..53177673 --- /dev/null +++ b/tests/core/variables/test_variable_clone.py @@ -0,0 +1,37 @@ +"""Tests for ``Variable.clone()`` — policyengine-core#502. + +Characterization of the existing contract for normal (non-reform) variables: +a clone preserves the variable's attributes, is the same class, and is +independent of the original. These pass on the current implementation and +guard the behaviour a later fix must keep. +""" + +import policyengine_core.country_template as country_template + +tax_benefit_system = country_template.CountryTaxBenefitSystem() + + +def test_clone_preserves_attributes(): + original = tax_benefit_system.get_variable("disposable_income") + clone = original.clone() + assert clone.value_type == original.value_type + assert clone.entity.key == original.entity.key + assert clone.definition_period == original.definition_period + assert clone.label == original.label + assert list(clone.formulas.keys()) == list(original.formulas.keys()) + + +def test_clone_is_same_class(): + original = tax_benefit_system.get_variable("disposable_income") + assert type(original.clone()) is type(original) + + +def test_clone_formulas_are_independent(): + # Fresh system so a (buggy) shared-container clone can't leak into other + # tests: mutating the clone must not affect the original. + system = country_template.CountryTaxBenefitSystem() + original = system.get_variable("disposable_income") + assert len(original.formulas) > 0 + clone = original.clone() + clone.formulas.clear() + assert len(original.formulas) > 0 From b1909a1ab927264325175c311bc73b19ec6111e4 Mon Sep 17 00:00:00 2001 From: Anthony Volk Date: Wed, 8 Jul 2026 20:58:14 +0200 Subject: [PATCH 2/6] Add xfail regression tests for Variable.clone() on update_variable (#502) Cloning a variable registered via a reform's update_variable currently raises "Missing attribute 'value_type'" because clone() re-runs __init__ without the baseline. Documented as strict xfail (label-only and adds-only overrides) until the fix lands. Co-Authored-By: Claude Opus 4.8 --- tests/core/variables/test_variable_clone.py | 44 +++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/tests/core/variables/test_variable_clone.py b/tests/core/variables/test_variable_clone.py index 53177673..4327b935 100644 --- a/tests/core/variables/test_variable_clone.py +++ b/tests/core/variables/test_variable_clone.py @@ -6,7 +6,11 @@ guard the behaviour a later fix must keep. """ +import pytest + import policyengine_core.country_template as country_template +from policyengine_core.model_api import Reform, Variable +from policyengine_core.periods import MONTH tax_benefit_system = country_template.CountryTaxBenefitSystem() @@ -35,3 +39,43 @@ def test_clone_formulas_are_independent(): clone = original.clone() clone.formulas.clear() assert len(original.formulas) > 0 + + +@pytest.mark.xfail(strict=True, reason="policyengine-core#502") +def test_clone_update_variable_label_only(): + # A reform that overrides only the label inherits value_type/entity/ + # formula from the baseline; cloning it must keep that merged state. + class disposable_income(Variable): + label = "Updated label only" + + class reform(Reform): + def apply(self): + self.update_variable(disposable_income) + + system = reform(country_template.CountryTaxBenefitSystem()) + clone = system.get_variable("disposable_income").clone() + + assert clone.value_type == float # inherited from baseline + assert clone.entity.key == "person" + assert clone.definition_period == MONTH + assert clone.label == "Updated label only" # the override + assert len(clone.formulas) > 0 # inherited formula preserved + + +@pytest.mark.xfail(strict=True, reason="policyengine-core#502") +def test_clone_update_variable_adds_only(): + # A reform that redeclares a formula variable with adds only inherits the + # baseline formula; cloning must keep the merged state. + class disposable_income(Variable): + adds = ["salary"] + + class reform(Reform): + def apply(self): + self.update_variable(disposable_income) + + system = reform(country_template.CountryTaxBenefitSystem()) + clone = system.get_variable("disposable_income").clone() + + assert clone.value_type == float + assert clone.adds == ["salary"] + assert len(clone.formulas) > 0 # inherited formula preserved From 6d646be878459ccee0f3355726af0e9d61044c27 Mon Sep 17 00:00:00 2001 From: Anthony Volk Date: Wed, 8 Jul 2026 21:01:11 +0200 Subject: [PATCH 3/6] Fix Variable.clone() to preserve update_variable-inherited attributes (#502) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clone() re-ran __init__ with baseline_variable=None, dropping every attribute a reform's update_variable merged in from the baseline (value_type, entity, formulas, ...) and raising "Missing attribute 'value_type'". Cloning a reformed tax-benefit system — which the YAML test runner and branch calculations do for every variable — therefore crashed. Clone now mirrors TaxBenefitSystem.clone(): empty_clone + __dict__ copy, preserving the variable's merged state, with formulas/metadata copied so the clone stays independent. The two #502 regression tests are un-xfailed. Fixes #502. Co-Authored-By: Claude Opus 4.8 --- policyengine_core/variables/variable.py | 16 +++++++++++++++- tests/core/variables/test_variable_clone.py | 4 ---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/policyengine_core/variables/variable.py b/policyengine_core/variables/variable.py index 7f55c3ea..a07a771f 100644 --- a/policyengine_core/variables/variable.py +++ b/policyengine_core/variables/variable.py @@ -8,6 +8,7 @@ import sortedcontainers from policyengine_core import periods, tools +from policyengine_core.commons.misc import empty_clone from policyengine_core.entities import Entity from policyengine_core.enums import Enum, EnumArray from policyengine_core.periods import Period @@ -660,7 +661,20 @@ def get_formula(self, period=None): return None def clone(self): - clone = self.__class__() + """Return an independent copy that preserves the variable's merged state. + + Uses ``empty_clone`` + a ``__dict__`` copy (the ``TaxBenefitSystem.clone`` + pattern) rather than re-running ``__init__``, so a variable registered + via a reform's ``update_variable`` keeps the attributes it inherited + from its baseline (``value_type``, ``entity``, ``formulas``, ...). + Mutable containers are copied so the clone stays independent of the + original, as the previous ``__init__``-based clone did. + """ + clone = empty_clone(self) + clone.__dict__.update(self.__dict__) + clone.formulas = self.formulas.copy() + if self.metadata is not None: + clone.metadata = dict(self.metadata) return clone def check_set_value(self, value): diff --git a/tests/core/variables/test_variable_clone.py b/tests/core/variables/test_variable_clone.py index 4327b935..bcabd344 100644 --- a/tests/core/variables/test_variable_clone.py +++ b/tests/core/variables/test_variable_clone.py @@ -6,8 +6,6 @@ guard the behaviour a later fix must keep. """ -import pytest - import policyengine_core.country_template as country_template from policyengine_core.model_api import Reform, Variable from policyengine_core.periods import MONTH @@ -41,7 +39,6 @@ def test_clone_formulas_are_independent(): assert len(original.formulas) > 0 -@pytest.mark.xfail(strict=True, reason="policyengine-core#502") def test_clone_update_variable_label_only(): # A reform that overrides only the label inherits value_type/entity/ # formula from the baseline; cloning it must keep that merged state. @@ -62,7 +59,6 @@ def apply(self): assert len(clone.formulas) > 0 # inherited formula preserved -@pytest.mark.xfail(strict=True, reason="policyengine-core#502") def test_clone_update_variable_adds_only(): # A reform that redeclares a formula variable with adds only inherits the # baseline formula; cloning must keep the merged state. From 1c3c7ea38041cfd2e7b9dfc74510642a9a2cce94 Mon Sep 17 00:00:00 2001 From: Anthony Volk Date: Wed, 8 Jul 2026 21:02:45 +0200 Subject: [PATCH 4/6] Guard clone() on the reformed-system path and runtime attributes (#502) Add an end-to-end test that clones a whole reformed TaxBenefitSystem (the path the YAML test runner and branch calculations take), plus a test that a runtime-set attribute (metadata) is preserved by clone() and stays independent of the original. Co-Authored-By: Claude Opus 4.8 --- tests/core/variables/test_variable_clone.py | 33 +++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/core/variables/test_variable_clone.py b/tests/core/variables/test_variable_clone.py index bcabd344..51edd684 100644 --- a/tests/core/variables/test_variable_clone.py +++ b/tests/core/variables/test_variable_clone.py @@ -75,3 +75,36 @@ def apply(self): assert clone.value_type == float assert clone.adds == ["salary"] assert len(clone.formulas) > 0 # inherited formula preserved + + +def test_clone_reformed_system_with_update_variable(): + # The real "kills deployment upon test" path: TaxBenefitSystem.clone() + # (used by the YAML test runner and branch calculations) clones every + # variable, including one registered via update_variable. + class disposable_income(Variable): + label = "Updated label only" + + class reform(Reform): + def apply(self): + self.update_variable(disposable_income) + + system = reform(country_template.CountryTaxBenefitSystem()) + cloned_system = system.clone() # clones every variable; must not raise + + cloned = cloned_system.get_variable("disposable_income") + assert cloned.value_type == float + assert cloned.label == "Updated label only" + assert len(cloned.formulas) > 0 + + +def test_clone_preserves_and_isolates_runtime_metadata(): + # A runtime-set attribute (metadata) is preserved by the clone (the old + # __init__-based clone dropped it) and stays independent of the original. + system = country_template.CountryTaxBenefitSystem() + variable = system.get_variable("disposable_income") + variable.metadata = {"note": "original"} + + clone = variable.clone() + assert clone.metadata == {"note": "original"} # preserved + clone.metadata["note"] = "changed" + assert variable.metadata["note"] == "original" # independent From 28cd5e235d046f38a915d395f083d1e4677c28ce Mon Sep 17 00:00:00 2001 From: Anthony Volk Date: Wed, 8 Jul 2026 21:03:18 +0200 Subject: [PATCH 5/6] Add changelog fragment for the Variable.clone() #502 fix Co-Authored-By: Claude Opus 4.8 --- .../variable-clone-preserves-update-variable-attributes.fixed.md | 1 + 1 file changed, 1 insertion(+) create mode 100644 changelog.d/variable-clone-preserves-update-variable-attributes.fixed.md diff --git a/changelog.d/variable-clone-preserves-update-variable-attributes.fixed.md b/changelog.d/variable-clone-preserves-update-variable-attributes.fixed.md new file mode 100644 index 00000000..082c7d59 --- /dev/null +++ b/changelog.d/variable-clone-preserves-update-variable-attributes.fixed.md @@ -0,0 +1 @@ +`Variable.clone()` now preserves attributes inherited from a baseline variable via a reform's `update_variable` (using `empty_clone` + a `__dict__` copy instead of re-running `__init__`), so cloning a reformed tax-benefit system — as the YAML test runner and branch calculations do for every variable — no longer raises `Missing attribute 'value_type'`. From 7deaf47e982e8ca56fdf71af7b9d4148d9374969 Mon Sep 17 00:00:00 2001 From: Anthony Volk Date: Wed, 8 Jul 2026 21:16:45 +0200 Subject: [PATCH 6/6] Tighten clone() docstring and widen clone test coverage (#502) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Docstring now states that formulas and metadata are copied (adds/subtracts are shared, as before) and that baseline_variable is retained. Add tests for the real reform paths that clone under the hood — get_neutralized_variable and get_annualized_variable of an update_variable variable — plus an Enum variable clone and an assertion documenting adds sharing. Co-Authored-By: Claude Opus 4.8 --- policyengine_core/variables/variable.py | 9 ++-- tests/core/variables/test_variable_clone.py | 50 +++++++++++++++++++++ 2 files changed, 55 insertions(+), 4 deletions(-) diff --git a/policyengine_core/variables/variable.py b/policyengine_core/variables/variable.py index a07a771f..06343f1a 100644 --- a/policyengine_core/variables/variable.py +++ b/policyengine_core/variables/variable.py @@ -661,14 +661,15 @@ def get_formula(self, period=None): return None def clone(self): - """Return an independent copy that preserves the variable's merged state. + """Return a copy that preserves the variable's merged state. Uses ``empty_clone`` + a ``__dict__`` copy (the ``TaxBenefitSystem.clone`` pattern) rather than re-running ``__init__``, so a variable registered via a reform's ``update_variable`` keeps the attributes it inherited - from its baseline (``value_type``, ``entity``, ``formulas``, ...). - Mutable containers are copied so the clone stays independent of the - original, as the previous ``__init__``-based clone did. + from its baseline (``value_type``, ``entity``, ``formulas``, + ``baseline_variable``, ...). ``formulas`` and ``metadata`` are copied so + mutating them on the clone does not affect the original; other + containers (``adds``/``subtracts``) are shared, as they were before. """ clone = empty_clone(self) clone.__dict__.update(self.__dict__) diff --git a/tests/core/variables/test_variable_clone.py b/tests/core/variables/test_variable_clone.py index 51edd684..54571938 100644 --- a/tests/core/variables/test_variable_clone.py +++ b/tests/core/variables/test_variable_clone.py @@ -7,8 +7,13 @@ """ import policyengine_core.country_template as country_template +from policyengine_core.enums import Enum from policyengine_core.model_api import Reform, Variable from policyengine_core.periods import MONTH +from policyengine_core.variables.helpers import ( + get_annualized_variable, + get_neutralized_variable, +) tax_benefit_system = country_template.CountryTaxBenefitSystem() @@ -108,3 +113,48 @@ def test_clone_preserves_and_isolates_runtime_metadata(): assert clone.metadata == {"note": "original"} # preserved clone.metadata["note"] = "changed" assert variable.metadata["note"] == "original" # independent + + +def _reform_updated_disposable_income(**attrs): + disposable_income = type("disposable_income", (Variable,), attrs) + + class reform(Reform): + def apply(self): + self.update_variable(disposable_income) + + return reform(country_template.CountryTaxBenefitSystem()).get_variable( + "disposable_income" + ) + + +def test_neutralize_reform_updated_variable(): + # get_neutralized_variable clones the variable; the real "reform then + # neutralize" path must not crash for an update_variable variable. + variable = _reform_updated_disposable_income(label="Updated label only") + neutralized = get_neutralized_variable(variable) + assert neutralized.value_type == float + assert neutralized.is_neutralized is True + + +def test_annualize_reform_updated_variable(): + # get_annualized_variable clones the variable; must work for a reform var. + variable = _reform_updated_disposable_income(label="Updated label only") + annualized = get_annualized_variable(variable) + assert annualized.value_type == float + assert len(annualized.formulas) > 0 # inherited formula preserved + + +def test_clone_enum_variable(): + variable = tax_benefit_system.get_variable("housing_occupancy_status") + assert variable.value_type is Enum + clone = variable.clone() + assert clone.value_type is Enum + assert clone.possible_values is variable.possible_values + assert clone.default_value == variable.default_value + + +def test_clone_shares_adds(): + # Documented behavior: adds/subtracts are shared with the original (as + # before the fix); only formulas and metadata are copied. + variable = _reform_updated_disposable_income(adds=["salary"]) + assert variable.clone().adds is variable.adds