Add tests for external document adoption#1
Add tests for external document adoption#1scoder merged 2 commits intoscoder:LP1595781_adopt_external_docfrom kovidgoyal:LP1595781_adopt_external_doc
Conversation
adopt_external_document() should type check its argument
| xmlDoc *c_doc; | ||
| void *context; | ||
| *is_owned = 0; | ||
| if (unlikely_condition(!PyCapsule_IsValid(capsule, (const char*)"libxml2:xmlDoc"))) { |
There was a problem hiding this comment.
This is already done internally by PyCapsule_GetPointer(), so I left it out.
There was a problem hiding this comment.
I prefer to be explicit since the documentation of GetPointer does not state that it checks the type of its argument, I dont want to rely on it.
There was a problem hiding this comment.
There are two possibilities, the object is not a capsule, i.e. PyCapsule_CheckExact fails or the capsule name is not correct. The documentation of GetPointer only claims that it checks the latter, not the former.
| from common_imports import etree, HelperTestCase | ||
|
|
||
|
|
||
| @unittest.skipIf(sys.version_info[:2] < (2, 7), |
There was a problem hiding this comment.
unittest.skipIf() requires Python 2.7, though ;)
But there's a fallback for it in common_imports.
| self.assertEqual(DOC_NAME, self.get_capsule_name(capsule)) | ||
| # Create an lxml tree from the capsule (this is a move not a copy) | ||
| root = etree.adopt_external_document(capsule).getroot() | ||
| self.assertIsNone(self.get_capsule_name(capsule)) |
There was a problem hiding this comment.
Not sure what this means? The entire test case is conditioned to run only on py >= 2.7
| from ctypes import pythonapi | ||
| from ctypes.util import find_library | ||
|
|
||
| def wrap(func, restype, *argtypes): |
There was a problem hiding this comment.
I found it very confusing to see the return type as second argument here. Looks like the first argument to the function.
There was a problem hiding this comment.
Seems natural to me indeed ctypes itself uses this convention for instance in CFUNCTYPE()
There was a problem hiding this comment.
To expand on why it is natural -- when you read a function declaration in C the result type is before the argument types.
| self.assertIsNone(self.get_capsule_name(capsule)) | ||
| self.assertEqual(root.text, 't') | ||
| root.text = 'new text' | ||
| # Now reset the capsule so we can copy it |
There was a problem hiding this comment.
To keep the memory ownership clean, I'd test the copy first, then the move. It's sad that capsules do not allow setting their pointer to NULL in order to properly invalidate them when transferring ownership...
There was a problem hiding this comment.
The problem with doing copy first is you cannot then make modifications using the lxml api and check that they happen on the underlying tree as well, as is done on line 85. So you'd basically be reducing the amount of testing.
There was a problem hiding this comment.
The thing is, these modifications should not be done in the first place as they are illegal from an ownership perspective. The fact that the tree is not copied but transferred by pointer is an optimisation, not a feature meant to allow later modifications from two sides. I guess I should document that...
There was a problem hiding this comment.
Certainly, but this is a test suite, not a usage example. The intention of the test suite is to test as many code paths as possible. This is the only way to test that moving a tree is actually working as intended (well technically not the only way, one could check the original xmlDoc* pointer using libxml2 APIs but that just makes the test code a lot more complex.
There was a problem hiding this comment.
(Ok, the modifications are ok because they are done through the lxml API - it's the reuse of the capsule that is illegal.)
|
Ok, works for me. Thanks! |
|
You're welcome and thank you for implementing this feature :) |
2 participants
As requested here: https://bugs.launchpad.net/lxml/+bug/1595781
Uses ctypes to avoid the complications of trying to run cython in the test suite. Since this code is pretty trivial, there aren't a lot of tests to run. At least that I can think of.