feat: Increase test coverage and strengthen dicttoxml typing #246
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…me handling Adds coverage for CDATA edge, numeric and namespaced keys, list header behavior, @Flat handling, Decimal typing, cdata conversion, and id attributes when ids provided.
…get_unique_id API - Accept optional ids list to avoid collisions deterministically in tests - Replace legacy type name checks with direct 'str'/'int' - Update tests to use monkeypatch for duplicate id simulation
Reviewer's GuideThis PR refactors the unique ID generation and XML type detection in dicttoxml for stronger typing and controlled uniqueness, and adds a suite of targeted tests to cover edge cases like CDATA splitting, namespaced/numeric keys, list wrapping behavior, Decimal handling, and ID attribute injection. Class diagram for test coverage improvements in test_additional_coverage.pyclassDiagram
class test_additional_coverage
test_additional_coverage : +test_cdata_end_sequence()
test_additional_coverage : +test_namespaced_numeric_keys()
test_additional_coverage : +test_list_header_flat_handling()
test_additional_coverage : +test_decimal_typing()
test_additional_coverage : +test_ids_attribute_behavior()
test_additional_coverage --> dicttoxml : tests
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #246 +/- ##
===========================================
+ Coverage 99.30% 100.00% +0.69%
===========================================
Files 3 3
Lines 288 289 +1
===========================================
+ Hits 286 289 +3
+ Misses 2 0 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @vinitkumar - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/test_additional_coverage.py:16` </location>
<code_context>
+ wrapped = dicttoxml.wrap_cdata(text)
+ assert wrapped == "<![CDATA[a]]]]><![CDATA[>b]]>"
+
+ def test_make_valid_xml_name_with_int_key(self) -> None:
+ # Int keys should be converted to n<digits>
+ key, attr = dicttoxml.make_valid_xml_name(123, {}) # type: ignore[arg-type]
+ assert key == "n123"
+ assert attr == {}
+
+ def test_make_valid_xml_name_namespace_flat(self) -> None:
</code_context>
<issue_to_address>
Consider adding a test for negative integer keys in make_valid_xml_name.
Negative integer keys may be handled differently and could cause issues with XML naming. Please add a test for a negative integer key to verify correct behavior.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_make_valid_xml_name_namespace_flat(self) -> None:
# Namespaced key with @flat suffix should be considered valid as-is
key_in = "ns:key@flat"
key_out, attr = dicttoxml.make_valid_xml_name(key_in, {})
assert key_out == key_in
assert attr == {}
=======
def test_make_valid_xml_name_with_negative_int_key(self) -> None:
# Negative int keys should be converted to a valid XML name, e.g., n-123
key, attr = dicttoxml.make_valid_xml_name(-123, {}) # type: ignore[arg-type]
assert key.startswith("n")
assert "-" in key
assert "123" in key
assert attr == {}
def test_make_valid_xml_name_namespace_flat(self) -> None:
# Namespaced key with @flat suffix should be considered valid as-is
key_in = "ns:key@flat"
key_out, attr = dicttoxml.make_valid_xml_name(key_in, {})
assert key_out == key_in
assert attr == {}
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `tests/test_additional_coverage.py:29` </location>
<code_context>
+ assert key_out == key_in
+ assert attr == {}
+
+ def test_dict2xml_str_parent_list_with_attrs_and_no_wrap(self) -> None:
+ # When inside list context with list_headers=True and item_wrap=False,
+ # attributes belong to the parent element header
+ item = {"@attrs": {"a": "b"}, "@val": "X"}
+ xml = dicttoxml.dict2xml_str(
+ attr_type=False,
+ attr={},
+ item=item,
+ item_func=lambda _p: "item",
+ cdata=False,
+ item_name="ignored",
+ item_wrap=False,
+ parentIsList=True,
+ parent="Parent",
+ list_headers=True,
+ )
+ assert xml == '<Parent a="b">X</Parent>'
+
+ def test_dict2xml_str_with_flat_flag_in_item(self) -> None:
</code_context>
<issue_to_address>
Consider adding a test for multiple attributes in parent list context.
Please add a test case with multiple attributes to verify correct serialization in the parent element.
</issue_to_address>
### Comment 3
<location> `tests/test_additional_coverage.py:62` </location>
<code_context>
+ )
+ assert xml == "text"
+
+ def test_list2xml_str_returns_subtree_when_list_headers_true(self) -> None:
+ # list_headers=True should return subtree directly from convert_list
+ xml = dicttoxml.list2xml_str(
+ attr_type=False,
+ attr={},
+ item=["a"],
+ item_func=lambda _p: "item",
+ cdata=False,
+ item_name="test",
+ item_wrap=True,
+ list_headers=True,
+ )
+ assert xml == "<item>a</item>"
+
+ def test_get_xml_type_with_decimal_number(self) -> None:
</code_context>
<issue_to_address>
Consider testing list2xml_str with an empty list.
Testing with an empty list helps ensure list2xml_str handles this edge case without errors or unexpected output. Please add a corresponding test.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_get_xml_type_with_decimal_number(self) -> None:
# Decimal is a numbers.Number but not int/float
value = decimal.Decimal("5")
assert dicttoxml.get_xml_type(value) == "number"
# And convert_kv should mark it as type="number"
out = dicttoxml.convert_kv("key", value, attr_type=True)
assert out == '<key type="number">5</key>'
=======
def test_list2xml_str_with_empty_list(self) -> None:
# Should handle empty list without error and return empty string
xml = dicttoxml.list2xml_str(
attr_type=False,
attr={},
item=[],
item_func=lambda _p: "item",
cdata=False,
item_name="test",
item_wrap=True,
list_headers=True,
)
assert xml == ""
def test_get_xml_type_with_decimal_number(self) -> None:
# Decimal is a numbers.Number but not int/float
value = decimal.Decimal("5")
assert dicttoxml.get_xml_type(value) == "number"
# And convert_kv should mark it as type="number"
out = dicttoxml.convert_kv("key", value, attr_type=True)
assert out == '<key type="number">5</key>'
>>>>>>> REPLACE
</suggested_fix>
### Comment 4
<location> `tests/test_additional_coverage.py:84` </location>
<code_context>
+ out = dicttoxml.convert_kv("key", value, attr_type=True)
+ assert out == '<key type="number">5</key>'
+
+ def test_dicttoxml_cdata_with_cdata_end_sequence(self) -> None:
+ data = {"key": "a]]>b"}
+ out = dicttoxml.dicttoxml(data, root=False, attr_type=False, cdata=True).decode()
+ assert out == "<key><![CDATA[a]]]]><![CDATA[>b]]></key>"
+
+ def test_convert_dict_with_ids_adds_id_attributes(self) -> None:
</code_context>
<issue_to_address>
Consider adding a test for multiple CDATA end sequences in a single value.
Please include a test case where the value contains more than one ']]>' sequence to verify correct handling in all instances.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
def test_convert_dict_with_ids_adds_id_attributes(self) -> None:
=======
def test_dicttoxml_cdata_with_multiple_cdata_end_sequences(self) -> None:
data = {"key": "a]]>b]]>c"}
out = dicttoxml.dicttoxml(data, root=False, attr_type=False, cdata=True).decode()
assert out == "<key><![CDATA[a]]]]><![CDATA[>b]]]]><![CDATA[>c]]></key>"
def test_convert_dict_with_ids_adds_id_attributes(self) -> None:
>>>>>>> REPLACE
</suggested_fix>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+22
to
+27
| def test_make_valid_xml_name_namespace_flat(self) -> None: | ||
| # Namespaced key with @flat suffix should be considered valid as-is | ||
| key_in = "ns:key@flat" | ||
| key_out, attr = dicttoxml.make_valid_xml_name(key_in, {}) | ||
| assert key_out == key_in | ||
| assert attr == {} |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for negative integer keys in make_valid_xml_name.
Negative integer keys may be handled differently and could cause issues with XML naming. Please add a test for a negative integer key to verify correct behavior.
Suggested change
| def test_make_valid_xml_name_namespace_flat(self) -> None: | |
| # Namespaced key with @flat suffix should be considered valid as-is | |
| key_in = "ns:key@flat" | |
| key_out, attr = dicttoxml.make_valid_xml_name(key_in, {}) | |
| assert key_out == key_in | |
| assert attr == {} | |
| def test_make_valid_xml_name_with_negative_int_key(self) -> None: | |
| # Negative int keys should be converted to a valid XML name, e.g., n-123 | |
| key, attr = dicttoxml.make_valid_xml_name(-123, {}) # type: ignore[arg-type] | |
| assert key.startswith("n") | |
| assert "-" in key | |
| assert "123" in key | |
| assert attr == {} | |
| def test_make_valid_xml_name_namespace_flat(self) -> None: | |
| # Namespaced key with @flat suffix should be considered valid as-is | |
| key_in = "ns:key@flat" | |
| key_out, attr = dicttoxml.make_valid_xml_name(key_in, {}) | |
| assert key_out == key_in | |
| assert attr == {} |
Comment on lines
+29
to
+38
| def test_dict2xml_str_parent_list_with_attrs_and_no_wrap(self) -> None: | ||
| # When inside list context with list_headers=True and item_wrap=False, | ||
| # attributes belong to the parent element header | ||
| item = {"@attrs": {"a": "b"}, "@val": "X"} | ||
| xml = dicttoxml.dict2xml_str( | ||
| attr_type=False, | ||
| attr={}, | ||
| item=item, | ||
| item_func=lambda _p: "item", | ||
| cdata=False, |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for multiple attributes in parent list context.
Please add a test case with multiple attributes to verify correct serialization in the parent element.
tests/test_additional_coverage.py
Outdated
Comment on lines
76
to
82
| def test_get_xml_type_with_decimal_number(self) -> None: | ||
| # Decimal is a numbers.Number but not int/float | ||
| value = decimal.Decimal("5") | ||
| assert dicttoxml.get_xml_type(value) == "number" | ||
| # And convert_kv should mark it as type="number" | ||
| out = dicttoxml.convert_kv("key", value, attr_type=True) | ||
| assert out == '<key type="number">5</key>' |
There was a problem hiding this comment.
suggestion (testing): Consider testing list2xml_str with an empty list.
Testing with an empty list helps ensure list2xml_str handles this edge case without errors or unexpected output. Please add a corresponding test.
Suggested change
| def test_get_xml_type_with_decimal_number(self) -> None: | |
| # Decimal is a numbers.Number but not int/float | |
| value = decimal.Decimal("5") | |
| assert dicttoxml.get_xml_type(value) == "number" | |
| # And convert_kv should mark it as type="number" | |
| out = dicttoxml.convert_kv("key", value, attr_type=True) | |
| assert out == '<key type="number">5</key>' | |
| def test_list2xml_str_with_empty_list(self) -> None: | |
| # Should handle empty list without error and return empty string | |
| xml = dicttoxml.list2xml_str( | |
| attr_type=False, | |
| attr={}, | |
| item=[], | |
| item_func=lambda _p: "item", | |
| cdata=False, | |
| item_name="test", | |
| item_wrap=True, | |
| list_headers=True, | |
| ) | |
| assert xml == "" | |
| def test_get_xml_type_with_decimal_number(self) -> None: | |
| # Decimal is a numbers.Number but not int/float | |
| value = decimal.Decimal("5") | |
| assert dicttoxml.get_xml_type(value) == "number" | |
| # And convert_kv should mark it as type="number" | |
| out = dicttoxml.convert_kv("key", value, attr_type=True) | |
| assert out == '<key type="number">5</key>' |
| out = dicttoxml.dicttoxml(data, root=False, attr_type=False, cdata=True).decode() | ||
| assert out == "<key><![CDATA[a]]]]><![CDATA[>b]]></key>" | ||
|
|
||
| def test_convert_dict_with_ids_adds_id_attributes(self) -> None: |
There was a problem hiding this comment.
suggestion (testing): Consider adding a test for multiple CDATA end sequences in a single value.
Please include a test case where the value contains more than one ']]>' sequence to verify correct handling in all instances.
Suggested change
| def test_convert_dict_with_ids_adds_id_attributes(self) -> None: | |
| def test_dicttoxml_cdata_with_multiple_cdata_end_sequences(self) -> None: | |
| data = {"key": "a]]>b]]>c"} | |
| out = dicttoxml.dicttoxml(data, root=False, attr_type=False, cdata=True).decode() | |
| assert out == "<key><![CDATA[a]]]]><![CDATA[>b]]]]><![CDATA[>c]]></key>" | |
| def test_convert_dict_with_ids_adds_id_attributes(self) -> None: |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| import decimal | ||
| from typing import Any | ||
|
|
||
| import pytest |
Check notice
Code scanning / CodeQL
Unused import Note test
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 1 month ago
To fix the problem, simply remove the unused import statement import pytest from line 4 of tests/test_additional_coverage.py. This will clean up the code by eliminating an unnecessary dependency and improving readability. No other changes are required, as the rest of the code does not reference pytest directly.
Suggested changeset
1
tests/test_additional_coverage.py
| @@ -3,3 +3,3 @@ | ||
|
|
||
| import pytest | ||
|
|
||
|
|
Copilot is powered by AI and may make mistakes. Always verify output.
vinitkumar
changed the title
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
dicttoxml(CDATA end sequence, namespaced/numeric keys, list header and @Flat handling, Decimal typing, ids attribute behavior)get_unique_idto accept an optionalidsparameter for controllable uniqueness and simplify type checks inget_xml_typeTest plan
python -m pip install -r requirements-dev.txtpytest -q --cov=json2xml --cov-report=term-missingdict2xml_str,list2xml_str, and CDATA handlingSummary by Sourcery
Allow external ID tracking in get_unique_id, refine XML type detection, and add targeted tests to cover CDATA handling, XML naming, list flattening, Decimal typing, and auto-generated IDs.
Enhancements:
Tests: