Both test_flush_reparse_deferral_disabled tests fail with older libexpat (2.4.4)

Bug report

Bug description:

Running the test suite for 3.9.19 (and other currently released versions) breaks with the old libexpat (of course, patched for the security issues):

[  684s] 0:04:10 load avg: 0.64 Re-running test_sax in verbose mode (matching: test_flush_re
parse_deferral_disabled)
[  684s] test_flush_reparse_deferral_disabled (test.test_sax.ExpatReaderTest) ... FAIL
[  684s] 
[  684s] ======================================================================
[  684s] FAIL: test_flush_reparse_deferral_disabled (test.test_sax.ExpatReaderTest)
[  684s] ----------------------------------------------------------------------
[  684s] Traceback (most recent call last):
[  684s]   File "/home/abuild/rpmbuild/BUILD/Python-3.9.19/Lib/test/test_sax.py", line 1251,
 in test_flush_reparse_deferral_disabled
[  684s]     self.assertEqual(result.getvalue(), start)  # i.e. no elements started
[  684s] AssertionError: b'<?xml version="1.0" encoding="iso-8859-1"?>\n<doc>' != b'<?xml ve
rsion="1.0" encoding="iso-8859-1"?>\n'
[  684s] 
[  684s] ----------------------------------------------------------------------
[  684s] Ran 1 test in 0.001s
[  684s] 
[  684s] FAILED (failures=1)
[  684s] test test_sax failed
[  684s] 0:04:10 load avg: 0.64 Re-running test_xml_etree in verbose mode (matching: test_fl
ush_reparse_deferral_disabled)
[  684s] test_flush_reparse_deferral_disabled (test.test_xml_etree.XMLPullParserTest) ... FA
IL
[  684s] 
[  684s] ======================================================================
[  684s] FAIL: test_flush_reparse_deferral_disabled (test.test_xml_etree.XMLPullParserTest)
[  684s] ----------------------------------------------------------------------
[  684s] Traceback (most recent call last):
[  684s]   File "/home/abuild/rpmbuild/BUILD/Python-3.9.19/Lib/test/test_xml_etree.py", line
 1659, in test_flush_reparse_deferral_disabled
[  684s]     self.assert_event_tags(parser, [])  # i.e. no elements started
[  684s]   File "/home/abuild/rpmbuild/BUILD/Python-3.9.19/Lib/test/test_xml_etree.py", line
 1395, in assert_event_tags
[  684s]     self.assertEqual([(action, elem.tag) for action, elem in events],
[  684s] AssertionError: Lists differ: [('start', 'doc')] != []
[  684s] 
[  684s] First list contains 1 additional elements.
[  684s] First extra element 0:
[  684s] ('start', 'doc')
[  684s] 
[  684s] - [('start', 'doc')]
[  684s] + []
[  684s] 
[  684s] ----------------------------------------------------------------------
[  684s] Ran 1 test in 0.001s

Complete build log with all package versions and steps taken recorded.

CPython versions tested on:

3.8, 3.9, 3.10

Operating systems tested on:

Linux

Linked PRs

I am rather confused by the coding style of #115623 … isn’t there a preference for @unittest.skipIf instead of randomly included self.SkipTest commands, which are even applied rather inconsistently (as this issue shows)? Shouldn’t all tests including SetReparseDeferralEnabled and GetReparseDeferralEnabled be just covered by @unittest.skipIf(pyexpat.version_info < (2, 6, 0)) and be done with it, if those methods (as mentioned in the documentation) were introduced just in libexpat 2.6.0?

Wouldn’t something like this be more appropriate?

---
 Lib/test/test_sax.py       |   10 +++++-----
 Lib/test/test_xml_etree.py |   17 ++++++++---------
 2 files changed, 13 insertions(+), 14 deletions(-)

--- a/Lib/test/test_sax.py
+++ b/Lib/test/test_sax.py
@@ -1211,10 +1211,9 @@ class ExpatReaderTest(XmlTestBase):
 
         self.assertEqual(result.getvalue(), start + b"<doc>text</doc>")
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_enabled(self):
-        if pyexpat.version_info < (2, 6, 0):
-            self.skipTest(f'Expat {pyexpat.version_info} does not support reparse deferral')
-
         result = BytesIO()
         xmlgen = XMLGenerator(result)
         parser = create_parser()
@@ -1236,6 +1235,8 @@ class ExpatReaderTest(XmlTestBase):
 
         self.assertEqual(result.getvalue(), start + b"<doc></doc>")
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_disabled(self):
         result = BytesIO()
         xmlgen = XMLGenerator(result)
@@ -1245,8 +1246,7 @@ class ExpatReaderTest(XmlTestBase):
         for chunk in ("<doc", ">"):
             parser.feed(chunk)
 
-        if pyexpat.version_info >= (2, 6, 0):
-            parser._parser.SetReparseDeferralEnabled(False)
+        parser._parser.SetReparseDeferralEnabled(False)
 
         self.assertEqual(result.getvalue(), start)  # i.e. no elements started
         self.assertFalse(parser._parser.GetReparseDeferralEnabled())
--- a/Lib/test/test_xml_etree.py
+++ b/Lib/test/test_xml_etree.py
@@ -1619,11 +1619,9 @@ class XMLPullParserTest(unittest.TestCas
         with self.assertRaises(ValueError):
             ET.XMLPullParser(events=('start', 'end', 'bogus'))
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_enabled(self):
-        if pyexpat.version_info < (2, 6, 0):
-            self.skipTest(f'Expat {pyexpat.version_info} does not '
-                          'support reparse deferral')
-
         parser = ET.XMLPullParser(events=('start', 'end'))
 
         for chunk in ("<doc", ">"):
@@ -1644,17 +1642,18 @@ class XMLPullParserTest(unittest.TestCas
 
         self.assert_event_tags(parser, [('end', 'doc')])
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_disabled(self):
         parser = ET.XMLPullParser(events=('start', 'end'))
 
         for chunk in ("<doc", ">"):
             parser.feed(chunk)
 
-        if pyexpat.version_info >= (2, 6, 0):
-            if not ET is pyET:
-                self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled '
-                              'methods not available in C')
-            parser._parser._parser.SetReparseDeferralEnabled(False)
+        if not ET is pyET:
+            self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled '
+                          'methods not available in C')
+        parser._parser._parser.SetReparseDeferralEnabled(False)
 
         self.assert_event_tags(parser, [])  # i.e. no elements started
         if ET is pyET:

@hartwork

cc @hartwork

@mcepl

breaks with the old libexpat (of course, patched for the security issues):

@mcepl that's expected and distros backporting libexpat security fixed need to adjust the tests (and C pre-processor checks!) of CPython at the same time if doing so. This is a downstream issue, not an upstream one. I'm not alone in this view, e.g. see #115623 (review) .

I am rather confused by the coding style of #115623 … isn’t there a preference for @unittest.skipIf instead of randomly included self.SkipTest commands, which are even applied rather inconsistently (as this issue shows)? Shouldn’t all tests including SetReparseDeferralEnabled and GetReparseDeferralEnabled be just covered by @unittest.skipIf(pyexpat.version_info < (2, 6, 0)) and be done with it, if those methods (as mentioned in the documentation) were introduced just in libexpat 2.6.0?

Wouldn’t something like this be more appropriate?

@mcepl hunk 1 and 3 are purely cosmetic, and hence of limited interest to me:

Hunk 1:

---
 Lib/test/test_sax.py       |   10 +++++-----
 Lib/test/test_xml_etree.py |   17 ++++++++---------
 2 files changed, 13 insertions(+), 14 deletions(-)

--- a/Lib/test/test_sax.py
+++ b/Lib/test/test_sax.py
@@ -1211,10 +1211,9 @@ class ExpatReaderTest(XmlTestBase):
 
         self.assertEqual(result.getvalue(), start + b"<doc>text</doc>")
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_enabled(self):
-        if pyexpat.version_info < (2, 6, 0):
-            self.skipTest(f'Expat {pyexpat.version_info} does not support reparse deferral')
-
         result = BytesIO()
         xmlgen = XMLGenerator(result)
         parser = create_parser()

Hunk 3:

--- a/Lib/test/test_xml_etree.py
+++ b/Lib/test/test_xml_etree.py
@@ -1619,11 +1619,9 @@ class XMLPullParserTest(unittest.TestCas
         with self.assertRaises(ValueError):
             ET.XMLPullParser(events=('start', 'end', 'bogus'))
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_enabled(self):
-        if pyexpat.version_info < (2, 6, 0):
-            self.skipTest(f'Expat {pyexpat.version_info} does not '
-                          'support reparse deferral')
-
         parser = ET.XMLPullParser(events=('start', 'end'))
 
         for chunk in ("<doc", ">"):

Hunk 2 and 4 are a very different story:

Hunk 2:

@@ -1236,6 +1235,8 @@ class ExpatReaderTest(XmlTestBase):
 
         self.assertEqual(result.getvalue(), start + b"<doc></doc>")
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_disabled(self):
         result = BytesIO()
         xmlgen = XMLGenerator(result)
@@ -1245,8 +1246,7 @@ class ExpatReaderTest(XmlTestBase):
         for chunk in ("<doc", ">"):
             parser.feed(chunk)
 
-        if pyexpat.version_info >= (2, 6, 0):
-            parser._parser.SetReparseDeferralEnabled(False)
+        parser._parser.SetReparseDeferralEnabled(False)
 
         self.assertEqual(result.getvalue(), start)  # i.e. no elements started
         self.assertFalse(parser._parser.GetReparseDeferralEnabled())

Hunk 4:

@@ -1644,17 +1642,18 @@ class XMLPullParserTest(unittest.TestCas
 
         self.assert_event_tags(parser, [('end', 'doc')])
 
+    @unittest.skipIf(pyexpat.version_info < (2, 6, 0),
+                     "Reparse deferral not defined for libexpat < 2.6.0")
     def test_flush_reparse_deferral_disabled(self):
         parser = ET.XMLPullParser(events=('start', 'end'))
 
         for chunk in ("<doc", ">"):
             parser.feed(chunk)
 
-        if pyexpat.version_info >= (2, 6, 0):
-            if not ET is pyET:
-                self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled '
-                              'methods not available in C')
-            parser._parser._parser.SetReparseDeferralEnabled(False)
+        if not ET is pyET:
+            self.skipTest(f'XMLParser.(Get|Set)ReparseDeferralEnabled '
+                          'methods not available in C')
+        parser._parser._parser.SetReparseDeferralEnabled(False)
 
         self.assert_event_tags(parser, [])  # i.e. no elements started
         if ET is pyET:

@mcepl both hunk 2 and 4 seem to change semantics for the worse, as tests that were previously running with <2.6.0 fine are now skipped without need to. This inconsistency was done on full purpose.

I'm not against simplifying the two of the four skip markers that keep semantics intact, but I don't consider it of much value, personally.

My vote for closing this issue as "works as expected, no upstream issue to fix".

CC @serhiy-storchaka @gpshead

@mcepl

Shouldn’t all tests including SetReparseDeferralEnabled and GetReparseDeferralEnabled be just covered by @unittest.skipIf(pyexpat.version_info < (2, 6, 0)) and be done with it, if those methods (as mentioned in the documentation) were introduced just in libexpat 2.6.0?

@mcepl PS: Please note that these functions are exposed by CPython even with libexpat <2.6.0. They just don't do that much then.

So, you admit that the documentation is wrong. Also, why then these tests fail with older libexpat?

@mcepl

Hi @mcepl,

@mcepl PS: Please note that these functions are exposed by CPython even with libexpat <2.6.0. They just don't do that much then.
So, you admit that the documentation is wrong.

I don't, and I don't see how it would be wrong. What does the documentation state that is incorrect and which documentation exactly?

Also, why then these tests fail with older libexpat?

Because of the security patches that distros backported into older releases of libexpat that got you into unsupported territory between old and new releases. The failing version is not vanilla 2.4.4 or it would not fail. It's 2.4.4 with backported patches.

@mcepl that's expected and distros backporting libexpat security fixed need to adjust the tests (and C pre-processor checks!) of CPython at the same time if doing so. This is a downstream issue, not an upstream one. I'm not alone in this view, e.g. see #115623 (review) .

I am still confused, but it probably doesn’t matter. Either you expect everybody to use the latest and greatest libraries (which is kind of strange for the universal interpreter), or you would like to support even older libraries. For the first I don’t understand why you have those `if pyexpat.version_info` spread over the code, for the latter I don’t understand why you are not interested when I report you that the support for the older libraries is broken. Anyway, I will just use my patch in *SUSE and let it be.

@mcepl this seems to change semantics for the worse, as a test that was previously running with <2.6.0 fine is now skipped without need to.

No it wasn’t running, as this ticket is all about tests not running with older libexpat.

My vote for closing this issue as "works as expected, no upstream issue to fix".

Sure, I will just patch the Python for ourselves as long as you don’t care.

@mcepl

@mcepl I have sent you an e-mail offering a voice call in the mean time, about 30 minutes ago. I will refrain from replying here more for now, before we had a real chance to talk. Thanks for your consideration.

@mcepl

@mcepl I have sent you an e-mail offering a voice call in the mean time, about 30 minutes ago. I will refrain from replying here more for now, before we had a real chance to talk. Thanks for your consideration.

I am in the Central European Time zone, and it is Sunday night here. As I see you are in Berlin, we can talk tomorrow (mcepl on Libera.chat (preferable), or @mcepl:one.ems.host on Matrix).

@hartwork

I completely agree with @hartwork. Python tests work pretty well with both new and old versions of libexpat. They do not work with a distro-specific build of libexpat with distro-specific patches applied. This is not supported case. You perhaps need to apply a distro-specific patch for Python tests to make them consistent with the rest of of the distro. Or what is the standard way to solve such issues in your distro.

@mcepl

@mcepl Matrix and IRC do not work for me, but we'll find something else. Let's take call org to e-mail to keep the issue on topic. See you on Monday, have a good start to the week.

Hi, glad you two are chatting.

Looking at the build log you link to from the opening message, the important bit is:

This was a ./configure --with-system-expat build. (not our default build mode)
The system expat is version 2.4.4.

There were a pile of distro patches on top of that expat and Python applied, but none of those matter. (No backport of the related expat 2.6.0 security improvement feature was involved).

This is easy to reproduce on current non-bleeding-edge Debian and Ubuntu today (sporting expat 2.5.0 or earlier), make sure libexpat1-dev is installed, and presumbly a bunch of other distros (like opensuse or SuSE as was being used):

./configure --with-system-expat && ./python -m test test_sax test_xml_etree

Unfortunately, we do not test --with-system-expat in any way as part of the CPython project. So this failing right now isn't too surprising. That flag exists primarily for use by external distributors who unvendor libraries.

A good way to help prevent this kind of building against legacy libraries failure from happening in the future is to contribute & maintain a relevant builtbot. https://devguide.python.org/testing/new-buildbot-worker/

Python tests work pretty well with both new and old versions of libexpat.

This currently appears to unfortunately be false because we don't test this anywhere.

Thanks! main merged, 3.12 and 3.11 set to automerge, and PRs assigned to a RM with releaseblocker status for the security only branches that received the expat security fix leading to this issue.

No decision has been made on whether to try and handle the "expat with local backported behavior changes" case that causes test suite failures as found with Ubuntu's expat_2.5.0-2ubuntu0.1 package. Ideally a distro needing that would contribute that?

[3.12] gh-117187: Fix XML tests for vanilla Expat <2.6.0 (GH-117203) (G…

[3.11] gh-117187: Fix XML tests for vanilla Expat <2.6.0 (GH-117203) (G…

Update python39 to version 3.9.19 / rev 56 via SR 1161042

Update python310 to version 3.10.14 / rev 42 via SR 1161074

@gpshead

Thanks! main merged, 3.12 and 3.11 set to automerge, and PRs assigned to a RM with releaseblocker status for the security only branches that received the expat security fix leading to this issue.

@gpshead thank you!

No decision has been made on whether to try and handle the "expat with local backported behavior changes" case that causes test suite failures as found with Ubuntu's expat_2.5.0-2ubuntu0.1 package. Ideally a distro needing that would contribute that?

I would argue that support for downstream backports would be out of place upstream. Supporting multiple versions of vanilla Expat is complex enough. So instead of asking for these patches for upstream, let's not patch upstream and keep the whole backporting cake downstream.

[3.10] gh-117187: Fix XML tests for vanilla Expat <2.6.0 (GH-117203) (G…

[3.9] gh-117187: Fix XML tests for vanilla Expat <2.6.0 (GH-117203) (G…

[3.8] gh-117187: Fix XML tests for vanilla Expat <2.6.0 (GH-117203) (#…

Squashed commit of the following:

pythongh-117187: Fix XML tests for vanilla Expat <2.6.0 (pythonGH-117203

Navigation Menu

Repository navigation

Both test_flush_reparse_deferral_disabled tests fail with older libexpat (2.4.4) #117187

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

10 remaining items

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Both test_flush_reparse_deferral_disabled tests fail with older libexpat (2.4.4) #117187

Description

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Activity

mcepl commented on Mar 24, 2024

Eclips4 commented on Mar 24, 2024

hartwork commented on Mar 24, 2024

hartwork commented on Mar 24, 2024

mcepl commented on Mar 24, 2024

hartwork commented on Mar 24, 2024

mcepl commented on Mar 24, 2024

hartwork commented on Mar 24, 2024

mcepl commented on Mar 24, 2024

serhiy-storchaka commented on Mar 24, 2024

hartwork commented on Mar 24, 2024

gpshead commented on Mar 24, 2024

gpshead commented on Mar 24, 2024

10 remaining items

gpshead commented on Mar 26, 2024

hartwork commented on Mar 26, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions