Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

6.2.0: pytest is failing #2035

Closed
kloczek opened this issue Jul 18, 2022 · 6 comments · Fixed by #2044
Closed

6.2.0: pytest is failing #2035

kloczek opened this issue Jul 18, 2022 · 6 comments · Fixed by #2044
Assignees
Labels
bug Something isn't working testing

Comments

@kloczek
Copy link

kloczek commented Jul 18, 2022

I'm packaging your module as an rpm package so I'm using the typical PEP517 based build, install and test cycle used on building packages from non-root account.

  • python3 -sBm build -w --no-isolation
  • because I'm calling build with --no-isolation I'm using during all processes only locally installed modules
  • install .whl file in </install/prefix>
  • run pytest with PYTHONPATH pointing to sitearch and sitelib inside </install/prefix>
@kloczek
Copy link
Author

kloczek commented Jul 18, 2022

Here is pytest output
python-rdflib-pytest-log.txt

@aucampia
Copy link
Member

aucampia commented Jul 18, 2022

@kloczek this is a known issue: #1847

To work around it, don't run tests with lxml installed.

@aucampia aucampia added bug Something isn't working duplicate This issue or pull request already exists labels Jul 18, 2022
@aucampia
Copy link
Member

I will update our CI to run with lxml at least on some versions of python and then mark the failing tests with xfail.

@kloczek
Copy link
Author

kloczek commented Jul 18, 2022

@kloczek this is a known issue: #1847

To work around it, don't run tests with lxml installed.

I'm not sure do I understand that ticket correctly.
Does it mean that it is something wrong with lxml or rdflib needs to be updated? 🤔

@aucampia
Copy link
Member

RDFLib has a bug which is tracked in #1847 - and because of this bug, using lxml with XML based SPARQL results fail.

We should fix the bug, but we should also mark the tests in our test suite that are known to fail under some conditions with xfail under those conditions, so that the test suite still passes.

For example:

if os.name == "nt":
for test in ["turtle-subm-15", "turtle-subm-16"]:
EXPECTED_FAILURES[test] = "Issue with nt parser and line endings on windows"

This test is known to fail on windows, so we mark it with xfail on windows.

Similarly, with lxml installed, the failing tests here should have an xfail marker.

@aucampia aucampia added testing and removed duplicate This issue or pull request already exists labels Jul 18, 2022
@kloczek
Copy link
Author

kloczek commented Jul 18, 2022

Thank you very much for the explanation 😄

@aucampia aucampia self-assigned this Jul 20, 2022
aucampia added a commit that referenced this issue Jul 26, 2022
Fixed the following problems with the SPARQL XML result parsing:

- Both the parse method of both the lxml and `xml` modules does not work
  work well with `TextIO` objects, the `xml` module works with `TextIO`
  objects if the XML encoding is `utf-8`, but not if it is `utf-16`, and
  with `lxml` parse fails for both `utf-8` and `utf-16`. To fix this I
  changed the XML result parser to first convert `TextIO` to `bytes` and
  then feed the `bytes` to `parse()` using `BytesIO`.
- The parser was operating on all elements inside `results` and
  `result` elements, even if those elements were not `result` and `binding`
  elements respectively. This was causing problems with `lxml`, as `lxml`
  also returns comments when iterating over elements. To fix this I added
  a check for the element tags so that only the correct elements are
  considered.

Other changes:

- Added type hints to `rdflib.plugins.sparql.results.xmlresults`.
- Run with `lxml` one some permutations in the test matrix.
- Removed `rdflib.compat.etree`, as this was not very helpful for the
  SPARQL XML Result parser and it was not used elsewhere.
- Added an `lxml` environment to tox which installs `lxml` and
  `lxml-stubs`.
- Expanded SPARQL result testing by adding some additional
  parameters.

Related issues:

- Fixes #2035
- Fixes #1847
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working testing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants