TST: Add direct C-extension tests for iterparse.c by ReemHamraz · Pull Request #19922 · astropy/astropy

ReemHamraz · 2026-06-15T17:15:59Z

Description:

Continuing with my GSoC roadmap to harden Astropy's compiled extensions, this PR focuses on astropy/utils/xml/src/iterparse.c.

Previously, this module was only tested indirectly through the high-level _fast_iterparse Python wrapper. To safely prepare this low-level layer for the upcoming APE split, we need to prove that the compiled C-engine can handle memory sizing, encoding, and string parsing completely on its own.

What This PR Does

This introduces test_c_iterparse.py, a dedicated test suite that strictly targets the C-boundary in pure isolation:

Bypassed the public Python API entirely by directly importing and testing the _iterparser.so binary.
Replaced unreadable parameter tuples with a frozen, kw-only XMLTestData dataclass. This tightly links the input byte streams to the expected outputs.
Included tests for standard parsing, ISO-8859-1 decoding, and deliberately malformed byte streams to ensure libexpat correctly throws its no element found / unclosed token errors back over the C-boundary.

Testing

The tests pass cleanly locally within a strict uv virtual environment.

Looking forward to your feedback! Happy to adjust anything if needed.

github-actions · 2026-06-15T17:16:17Z

Thank you for your contribution to Astropy! 🌌 This checklist is meant to remind the package maintainers who will review this pull request of some common things to look for.

Do the proposed changes actually accomplish desired goals?
Do the proposed changes follow the Astropy coding guidelines?
Are tests added/updated as required? If so, do they follow the Astropy testing guidelines?
Are docs added/updated as required? If so, do they follow the Astropy documentation guidelines?
Is rebase and/or squash necessary? If so, please provide the author with appropriate instructions. Also see instructions for rebase and squash.
Did the CI pass? If no, are the failures related? If you need to run daily and weekly cron jobs as part of the PR, please apply the "Extra CI" label. Codestyle issues can be fixed by the bot.
Is a change log needed? If yes, did the change log check pass? If no, add the "no-changelog-entry-needed" label. If this is a manual backport, use the "skip-changelog-checks" label unless special changelog handling is necessary.
Is this a big PR that makes a "What's new?" entry worthwhile and if so, is (1) a "what's new" entry included in this PR and (2) the "whatsnew-needed" label applied?
At the time of adding the milestone, if the milestone set requires a backport to release branch(es), apply the appropriate "backport-X.Y.x" label(s) before merge.

neutrinoceros · 2026-06-16T06:21:43Z

a strict uv virtual environment

just curious what you mean by "strict" here.

ReemHamraz · 2026-06-16T08:48:16Z

a strict uv virtual environment

just curious what you mean by "strict" here.

I just meant a fully isolated virtual environment... it wasn't meant to signify anything special

neutrinoceros · 2026-06-17T09:12:06Z

+    for event in events:
+        assert type(event) is test_data.expected_event_type


I think we can do better here. Instead of just checking the type, please also check the contents (or absence thereof) match expectations.

neutrinoceros · 2026-06-17T09:14:05Z

+
+def test_c_iterparser_malformed_raises_error() -> None:
+    """Check that libexpat actively catches the error when we feed it broken XML."""
+    # intentionally cutting off the XML stream mid-tag here to make sure Expat panics.


I'd avoid the term "panic" as it doesn't hold any precise meaning in Python or C, but it does in Rust (panics are uncatchable runtime exceptions)

Suggested change

# intentionally cutting off the XML stream mid-tag here to make sure Expat panics.

# intentionally cutting off the XML stream mid-tag here to make sure Expat raises.

neutrinoceros · 2026-06-17T11:29:01Z

+        is_start = actual_event[0]
+        tag_name = actual_event[1]
+        node_data = actual_event[2]
+
+        assert is_start is expected_event[0]
+        assert tag_name == expected_event[1]
+        assert node_data == expected_event[2]


That's the same and more (it'll also fail if one of them grows larger than the other)

Suggested change

is_start = actual_event[0]

tag_name = actual_event[1]

node_data = actual_event[2]

assert is_start is expected_event[0]

assert tag_name == expected_event[1]

assert node_data == expected_event[2]

assert actual_event == excpected_event

Okay, so during the previous iteration i figured that the raw C-engine actually yields a 4-tuple (it tacks a (line, column) position tuple onto the very end), so a direct actual_event == expected_event will fail against the 3-item expected tuples. But to add what you've suggested, I've updated it to slice the runtime tuple: assert actual_event[:3] == expected_event

Thanks for the explanation, but then it's not clear to me why we wouldn't also match the fourth argument

I initially sliced it out just to keep the test matrix simple and avoid hardcoding line numbers, but I get why we should strictly validate every single piece of data it yields..... fixing it rn

neutrinoceros · 2026-06-17T11:31:23Z

+
+    assert len(events) == len(test_data.expected_events)
+
+    for actual_event, expected_event in zip(events, test_data.expected_events):


I suggest to always start with strict=True, and switch to an explicit strict=False only in the rarest cases where it's desirable.
For reference, the strict argument was only added in Python 3.10 and strict=False matches the 3.9 behavior, which is why it's still the default today, but I'd argue it's a really bad choice of a default, in retrospect.

Suggested change

for actual_event, expected_event in zip(events, test_data.expected_events):

for actual_event, expected_event in zip(events, test_data.expected_events, strict=True):

Right okay!

neutrinoceros · 2026-06-17T11:32:33Z

+    pytest.param(
+        XMLTestData(
+            xml_bytes=b'<?xml version="1.0"?><root><child>text</child></root>',
+            expected_event_type=tuple,


Now that you're doing a much deeper equality check, I don't think we need to check the output type separately

neutrinoceros

LGTM now
@pllim should probably give her blessing too before we merge

pllim · 2026-06-18T14:36:44Z

Doesn't hurt. Merging. Thanks, all.

- In get_xml_iterator, the end event of an element that contains other elements now has empty text. Before, it repeated the text of the last element inside it. This applies to both the C and pure-Python parsers. This dramatically reduces memory use for reading large XML files. - For a VOTable BINARY or BINARY2 table, the end events of BINARY2, DATA and TABLE elements no longer each hold a full copy of the base64 data stream. - The C parser frees a text buffer that has grown past 1 MB once its text is handed over, instead of keeping it until parsing ends. - VOTable reading decodes the stream with binascii.a2b_base64 directly, which skips a full copy of the stream text. Base64 text is always ASCII, so valid files decode exactly as before. Only the error for a malformed stream with a non-ASCII character changes: it is now ValueError ("string argument should contain only ASCII characters") instead of UnicodeEncodeError, which is a ValueError subclass. - Reading the 2.5 GB file from issue astropy#8946 now needs about 6.8 GB of memory at its peak, down from about 17 GB. - Update the one test that expected the repeated text, which was just a coverage test pinning current behavior from PR astropy#19922. - Add tests for both parsers and for elements holding more than 1 MB of text.

* Stop XML end events from repeating the text of nested elements - In get_xml_iterator, the end event of an element that contains other elements now has empty text. Before, it repeated the text of the last element inside it. This applies to both the C and pure-Python parsers. This dramatically reduces memory use for reading large XML files. - For a VOTable BINARY or BINARY2 table, the end events of BINARY2, DATA and TABLE elements no longer each hold a full copy of the base64 data stream. - The C parser frees a text buffer that has grown past 1 MB once its text is handed over, instead of keeping it until parsing ends. - VOTable reading decodes the stream with binascii.a2b_base64 directly, which skips a full copy of the stream text. Base64 text is always ASCII, so valid files decode exactly as before. Only the error for a malformed stream with a non-ASCII character changes: it is now ValueError ("string argument should contain only ASCII characters") instead of UnicodeEncodeError, which is a ValueError subclass. - Reading the 2.5 GB file from issue #8946 now needs about 6.8 GB of memory at its peak, down from about 17 GB. - Update the one test that expected the repeated text, which was just a coverage test pinning current behavior from PR #19922. - Add tests for both parsers and for elements holding more than 1 MB of text. * Update changelog numbers and mention more accurate warning positions - Give the memory figures in decimal GB, matching the PR description - Note that end events now report a position near their own closing tag, so VOTable warnings and validation output point at the right line. * Update performance changelog to be more general * Add an API changelog entry for the get_xml_iterator change

github-actions Bot added the utils label Jun 15, 2026

pllim added this to the v8.1.0 milestone Jun 15, 2026

pllim added no-changelog-entry-needed testing labels Jun 15, 2026

neutrinoceros self-assigned this Jun 15, 2026

neutrinoceros reviewed Jun 16, 2026

View reviewed changes

Comment thread astropy/utils/xml/tests/test_c_iterparse.py Resolved

Comment thread astropy/utils/xml/tests/test_c_iterparse.py Resolved

Comment thread astropy/utils/xml/tests/test_c_iterparse.py Resolved

Comment thread astropy/utils/xml/tests/test_c_iterparse.py Resolved

neutrinoceros reviewed Jun 16, 2026

View reviewed changes

Comment thread astropy/utils/xml/tests/test_c_iterparse.py Resolved

ReemHamraz force-pushed the test-iterparse-extension branch 2 times, most recently from 96571f1 to e378035 Compare June 16, 2026 14:57

ReemHamraz marked this pull request as ready for review June 16, 2026 15:12

neutrinoceros requested changes Jun 17, 2026

View reviewed changes

ReemHamraz force-pushed the test-iterparse-extension branch from e378035 to 660afbb Compare June 17, 2026 10:42

neutrinoceros reviewed Jun 17, 2026

View reviewed changes

TST: Add direct C-extension tests for iterparse.c

3fcf716

ReemHamraz force-pushed the test-iterparse-extension branch from cfbb23f to 3fcf716 Compare June 18, 2026 12:52

neutrinoceros approved these changes Jun 18, 2026

View reviewed changes

pllim merged commit 198a897 into astropy:main Jun 18, 2026
38 checks passed

		for event in events:
		assert type(event) is test_data.expected_event_type

	# intentionally cutting off the XML stream mid-tag here to make sure Expat panics.
	# intentionally cutting off the XML stream mid-tag here to make sure Expat raises.


		assert len(events) == len(test_data.expected_events)

		for actual_event, expected_event in zip(events, test_data.expected_events):

Conversation

ReemHamraz commented Jun 15, 2026

Description:

What This PR Does

Testing

Uh oh!

github-actions Bot commented Jun 15, 2026

Uh oh!

neutrinoceros commented Jun 16, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ReemHamraz commented Jun 16, 2026

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neutrinoceros left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pllim commented Jun 18, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

neutrinoceros left a comment •

edited

Loading