From fbcf558e8457a52044c82fc85f18f910398318b8 Mon Sep 17 00:00:00 2001 From: James Williams Date: Mon, 23 Mar 2026 11:13:10 -0500 Subject: [PATCH 1/2] Fix __hash__/__eq__ inconsistency in HConfigChild (#185) __hash__ included new_in_config and order_weight but __eq__ intentionally excluded them, violating the Python invariant that a == b implies hash(a) == hash(b). __eq__ also checked tags but __hash__ did not include them. Align __hash__ to use the same fields as __eq__: text, tags, and children. Add five tests covering each dimension of the inconsistency and its practical impact on set deduplication and dict key lookup. Co-Authored-By: Claude Sonnet 4.6 --- hier_config/child.py | 5 +- tests/unit/test_child.py | 110 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 111 insertions(+), 4 deletions(-) diff --git a/hier_config/child.py b/hier_config/child.py index 2d50ae4..2945da8 100644 --- a/hier_config/child.py +++ b/hier_config/child.py @@ -69,10 +69,7 @@ def __hash__(self) -> int: return hash( ( self.text, - # self.tags, - # self.comments, - self.new_in_config, - self.order_weight, + self.tags, *self.children, ), ) diff --git a/tests/unit/test_child.py b/tests/unit/test_child.py index ea296c6..db2d35a 100644 --- a/tests/unit/test_child.py +++ b/tests/unit/test_child.py @@ -1144,3 +1144,113 @@ def test_idempotency_key_regex_trimmed_to_no_match() -> None: key = driver._idempotency_key(child, (MatchRule(re_search=r"interface.*"),)) # noqa: SLF001 # pyright: ignore[reportPrivateUsage] # Since "interface.*" doesn't match "logging console", should fall back to text assert key == ("text|logging console",) + + +def test_child_hash_eq_consistency_new_in_config() -> None: + """Test that equal HConfigChild objects have equal hashes regardless of new_in_config. + + Validates the bug in issue #185: __hash__ includes new_in_config but __eq__ does not, + violating the Python invariant that a == b implies hash(a) == hash(b). + """ + platform = Platform.CISCO_IOS + config = get_hconfig(platform) + child1 = config.add_child("interface GigabitEthernet0/0") + config2 = get_hconfig(platform) + child2 = config2.add_child("interface GigabitEthernet0/0") + + child1.new_in_config = False + child2.new_in_config = True + + # These two children compare as equal (same text, no tags, no children) + assert child1 == child2 + # Python invariant: equal objects must have equal hashes + assert hash(child1) == hash(child2) + + +def test_child_hash_eq_consistency_order_weight() -> None: + """Test that equal HConfigChild objects have equal hashes regardless of order_weight. + + Validates the bug in issue #185: __hash__ includes order_weight but __eq__ does not, + violating the Python invariant that a == b implies hash(a) == hash(b). + """ + platform = Platform.CISCO_IOS + config = get_hconfig(platform) + child1 = config.add_child("interface GigabitEthernet0/0") + config2 = get_hconfig(platform) + child2 = config2.add_child("interface GigabitEthernet0/0") + + child1.order_weight = 0 + child2.order_weight = 100 + + # These two children compare as equal (same text, no tags, no children) + assert child1 == child2 + # Python invariant: equal objects must have equal hashes + assert hash(child1) == hash(child2) + + +def test_child_hash_eq_consistency_tags() -> None: + """Test that __hash__ and __eq__ agree on whether tags affect equality. + + Validates the bug in issue #185: __eq__ checks tags but __hash__ does not include + tags, meaning two objects that compare unequal could have the same hash (not a + correctness violation, but inconsistent) while also raising the question of whether + tags should be part of the hash. + """ + platform = Platform.CISCO_IOS + config = get_hconfig(platform) + child1 = config.add_child("interface GigabitEthernet0/0") + config2 = get_hconfig(platform) + child2 = config2.add_child("interface GigabitEthernet0/0") + + child1.tags = frozenset({"safe"}) + child2.tags = frozenset() + + # __eq__ considers tags, so these are unequal + assert child1 != child2 + # Since they are unequal, their hashes should differ to avoid excessive collisions + # (not strictly required by the invariant, but required for correctness in reverse: + # if hash(a) != hash(b) then a != b must hold — currently tags are in __eq__ but + # not __hash__, so unequal objects can share a hash, which means dict/set lookup + # will fall back to __eq__ unexpectedly) + assert hash(child1) != hash(child2) + + +def test_child_set_deduplication_with_new_in_config() -> None: + """Test that equal HConfigChild objects are deduplicated correctly in sets. + + Validates the practical impact of issue #185: when new_in_config differs, + two logically equal children occupy different set buckets, causing duplicates. + """ + platform = Platform.CISCO_IOS + config = get_hconfig(platform) + child1 = config.add_child("interface GigabitEthernet0/0") + config2 = get_hconfig(platform) + child2 = config2.add_child("interface GigabitEthernet0/0") + + child1.new_in_config = False + child2.new_in_config = True + + assert child1 == child2 + # Equal objects must collapse to one entry in a set + assert len({child1, child2}) == 1 + + +def test_child_dict_key_lookup_with_order_weight() -> None: + """Test that HConfigChild objects with differing order_weight work as dict keys. + + Validates the practical impact of issue #185: when order_weight differs, a + logically equal child cannot be found as a dict key. + """ + platform = Platform.CISCO_IOS + config = get_hconfig(platform) + child1 = config.add_child("interface GigabitEthernet0/0") + config2 = get_hconfig(platform) + child2 = config2.add_child("interface GigabitEthernet0/0") + + child1.order_weight = 0 + child2.order_weight = 100 + + assert child1 == child2 + lookup: dict[HConfigChild, str] = {child1: "found"} + # child2 is equal to child1, so it must find the same dict entry + assert lookup[child2] == "found" From 985cd4b8ac970090db9c927e0fe4bd3b7fd4e99e Mon Sep 17 00:00:00 2001 From: James Williams Date: Mon, 23 Mar 2026 11:29:45 -0500 Subject: [PATCH 2/2] Fix pre-existing lint errors in next branch - test_benchmarks.py: replace append loops with extend (PERF401), add @staticmethod to methods that don't use self (PLR6301), suppress intentional print calls with noqa: T201 - test_child.py: suppress pylint too-many-lines (C0302) Co-Authored-By: Claude Sonnet 4.6 --- tests/benchmarks/test_benchmarks.py | 62 ++++++++++++++++++----------- tests/unit/test_child.py | 1 + 2 files changed, 39 insertions(+), 24 deletions(-) diff --git a/tests/benchmarks/test_benchmarks.py b/tests/benchmarks/test_benchmarks.py index 076b965..934e8d1 100644 --- a/tests/benchmarks/test_benchmarks.py +++ b/tests/benchmarks/test_benchmarks.py @@ -54,8 +54,10 @@ def _generate_large_ios_config(num_interfaces: int = 1000) -> str: " auto-cost reference-bandwidth 100000", ] ) - for i in range(num_interfaces): - lines.append(f" network 10.{i // 256}.{i % 256}.0 0.0.0.3 area 0") + lines.extend( + f" network 10.{i // 256}.{i % 256}.0 0.0.0.3 area 0" + for i in range(num_interfaces) + ) lines.extend( [ "!", @@ -125,8 +127,10 @@ def _generate_large_xr_config(num_interfaces: int = 1000) -> str: " router-id 10.0.0.1", ] ) - for i in range(num_interfaces): - lines.append(f" area 0 interface GigabitEthernet0/0/0/{i} cost 100") + lines.extend( + f" area 0 interface GigabitEthernet0/0/0/{i} cost 100" + for i in range(num_interfaces) + ) lines.append("!") return "\n".join(lines) @@ -145,33 +149,37 @@ def _time_fn(fn: Callable[[], object], iterations: int = 3) -> float: class TestParsingBenchmarks: """Benchmarks for config parsing.""" - def test_parse_large_ios_config(self) -> None: + @staticmethod + def test_parse_large_ios_config() -> None: """Parse a ~10k line IOS config via get_hconfig.""" config_text = _generate_large_ios_config() elapsed = _time_fn(lambda: get_hconfig(Platform.CISCO_IOS, config_text)) line_count = config_text.count("\n") - print(f"\nget_hconfig: {line_count} lines in {elapsed:.4f}s") + print(f"\nget_hconfig: {line_count} lines in {elapsed:.4f}s") # noqa: T201 assert elapsed < 5.0, f"Parsing took {elapsed:.2f}s, expected < 5s" - def test_parse_large_xr_config(self) -> None: + @staticmethod + def test_parse_large_xr_config() -> None: """Parse a ~10k line XR config via get_hconfig.""" config_text = _generate_large_xr_config() elapsed = _time_fn(lambda: get_hconfig(Platform.CISCO_XR, config_text)) line_count = config_text.count("\n") - print(f"\nget_hconfig (XR): {line_count} lines in {elapsed:.4f}s") + print(f"\nget_hconfig (XR): {line_count} lines in {elapsed:.4f}s") # noqa: T201 assert elapsed < 5.0, f"Parsing took {elapsed:.2f}s, expected < 5s" - def test_fast_load_large_ios_config(self) -> None: + @staticmethod + def test_fast_load_large_ios_config() -> None: """Parse a ~10k line IOS config via get_hconfig_fast_load.""" config_text = _generate_large_ios_config() config_lines = tuple(config_text.splitlines()) elapsed = _time_fn( lambda: get_hconfig_fast_load(Platform.CISCO_IOS, config_lines), ) - print(f"\nget_hconfig_fast_load: {len(config_lines)} lines in {elapsed:.4f}s") + print(f"\nget_hconfig_fast_load: {len(config_lines)} lines in {elapsed:.4f}s") # noqa: T201 assert elapsed < 5.0, f"Fast load took {elapsed:.2f}s, expected < 5s" - def test_fast_load_vs_get_hconfig(self) -> None: + @staticmethod + def test_fast_load_vs_get_hconfig() -> None: """get_hconfig_fast_load should be faster than get_hconfig.""" config_text = _generate_large_ios_config() config_lines = tuple(config_text.splitlines()) @@ -181,7 +189,7 @@ def test_fast_load_vs_get_hconfig(self) -> None: lambda: get_hconfig_fast_load(Platform.CISCO_IOS, config_lines), ) ratio = time_full / time_fast if time_fast > 0 else float("inf") - print( + print( # noqa: T201 f"\nget_hconfig: {time_full:.4f}s, " f"fast_load: {time_fast:.4f}s, " f"ratio: {ratio:.1f}x" @@ -195,7 +203,8 @@ def test_fast_load_vs_get_hconfig(self) -> None: class TestRemediationBenchmarks: """Benchmarks for config_to_get_to remediation.""" - def test_remediation_small_diff(self) -> None: + @staticmethod + def test_remediation_small_diff() -> None: """Remediation with ~5% of interfaces changed.""" running_text = _generate_large_ios_config() running = get_hconfig(Platform.CISCO_IOS, running_text) @@ -207,10 +216,11 @@ def test_remediation_small_diff(self) -> None: generated = get_hconfig(Platform.CISCO_IOS, generated_text) elapsed = _time_fn(lambda: running.config_to_get_to(generated)) - print(f"\nRemediation (10% diff): {elapsed:.4f}s") + print(f"\nRemediation (10% diff): {elapsed:.4f}s") # noqa: T201 assert elapsed < 5.0, f"Remediation took {elapsed:.2f}s, expected < 5s" - def test_remediation_large_diff(self) -> None: + @staticmethod + def test_remediation_large_diff() -> None: """Remediation with ~100% of interfaces changed.""" running = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config()) generated_text = _generate_large_ios_config().replace( @@ -219,10 +229,11 @@ def test_remediation_large_diff(self) -> None: generated = get_hconfig(Platform.CISCO_IOS, generated_text) elapsed = _time_fn(lambda: running.config_to_get_to(generated)) - print(f"\nRemediation (100% diff): {elapsed:.4f}s") + print(f"\nRemediation (100% diff): {elapsed:.4f}s") # noqa: T201 assert elapsed < 10.0, f"Remediation took {elapsed:.2f}s, expected < 10s" - def test_remediation_completely_different(self) -> None: + @staticmethod + def test_remediation_completely_different() -> None: """Remediation between two entirely different configs.""" running = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config(500)) # Generate a completely different config @@ -238,35 +249,38 @@ def test_remediation_completely_different(self) -> None: generated = get_hconfig(Platform.CISCO_IOS, "\n".join(lines)) elapsed = _time_fn(lambda: running.config_to_get_to(generated)) - print(f"\nRemediation (completely different): {elapsed:.4f}s") + print(f"\nRemediation (completely different): {elapsed:.4f}s") # noqa: T201 assert elapsed < 10.0, f"Remediation took {elapsed:.2f}s, expected < 10s" class TestIterationBenchmarks: """Benchmarks for tree traversal and iteration.""" - def test_all_children_sorted(self) -> None: + @staticmethod + def test_all_children_sorted() -> None: """Iterate all_children_sorted on a large config.""" config = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config()) elapsed = _time_fn(lambda: list(config.all_children_sorted())) child_count = len(list(config.all_children())) - print(f"\nall_children_sorted: {child_count} nodes in {elapsed:.4f}s") + print(f"\nall_children_sorted: {child_count} nodes in {elapsed:.4f}s") # noqa: T201 assert elapsed < 2.0, f"Iteration took {elapsed:.2f}s, expected < 2s" - def test_dump_simple(self) -> None: + @staticmethod + def test_dump_simple() -> None: """Dump a large config to simple text.""" config = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config()) elapsed = _time_fn(config.dump_simple) line_count = len(config.dump_simple()) - print(f"\ndump_simple: {line_count} lines in {elapsed:.4f}s") + print(f"\ndump_simple: {line_count} lines in {elapsed:.4f}s") # noqa: T201 assert elapsed < 2.0, f"dump_simple took {elapsed:.2f}s, expected < 2s" - def test_deep_copy(self) -> None: + @staticmethod + def test_deep_copy() -> None: """Deep copy a large config tree.""" config = get_hconfig(Platform.CISCO_IOS, _generate_large_ios_config()) elapsed = _time_fn(config.deep_copy) - print(f"\ndeep_copy: {elapsed:.4f}s") + print(f"\ndeep_copy: {elapsed:.4f}s") # noqa: T201 assert elapsed < 5.0, f"deep_copy took {elapsed:.2f}s, expected < 5s" diff --git a/tests/unit/test_child.py b/tests/unit/test_child.py index db2d35a..c1a5e23 100644 --- a/tests/unit/test_child.py +++ b/tests/unit/test_child.py @@ -1,4 +1,5 @@ """Tests for HConfigChild functionality.""" +# pylint: disable=too-many-lines import types