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

Refactor pytest plugin statistics handling for strict JUnit xml validation #1975

Closed
Zac-HD opened this issue May 9, 2019 · 3 comments · Fixed by #2743
Closed

Refactor pytest plugin statistics handling for strict JUnit xml validation #1975

Zac-HD opened this issue May 9, 2019 · 3 comments · Fixed by #2743
Labels
enhancement it's not broken, but we want it to be better interop how to play nicely with other packages

Comments

@Zac-HD
Copy link
Member

Zac-HD commented May 9, 2019

pytest-dev/pytest#5202 indicates that using the record_property fixture leads Pytest to generate an XML file which is not compliant with some strict schemas. We don't actually use this fixture, but we do hook our statistics data into the underlying data structure so it gets passed between xdist workers correctly.

pytest-dev/pytest#5205 proposes to add a fixture with a better way, but we don't need to use the fixture when we can hook into the underlying method directly (which has the advantage of working on earlier versions of Pytest too).

So, in our pytest plugin, I think we could replace the use of user_properties with getattr(request.config, "_xml", None).add_global_property(name, value) if the _xml attribute exists. Needs some manual exploration and testing to confirm the idea is sound first, but why not add yet another path for statistics data?

(it will also need to be tested, for example, both with and without xdist and checking the resulting xml for content and strict schema compliance)

@Zac-HD Zac-HD added enhancement it's not broken, but we want it to be better interop how to play nicely with other packages labels May 9, 2019
@Zac-HD Zac-HD self-assigned this May 11, 2020
@Zac-HD
Copy link
Member Author

Zac-HD commented May 11, 2020

Proof of concept: master...Zac-HD:interoperable-stats

Valid against the xunit2 xml schema, but to compensate this introduces a bunch of string escaping issues which... there doesn't seem to be a nice way around 😿

Not planning to keep working on this, but PRs or sponsored features are always an option.

@twmr
Copy link

twmr commented Oct 5, 2020

We've just stumbled upon this issue, because we wanted to remove a custom xsltproc script, that we use for post-processing pytest.xml (xunit1) files for making the Jenkins CI happy ;)

Are there any updates on this issue besides a POC implementation, that AFAIC doesn't use the record_testsuite_property fixture introduced by pytest-4.5. @Zac-HD you mentioned that your POC implementation/fix has some string escaping issues. I guess that we can't magically solve these issues by using the new pytest fixture, right? (I haven't looked at the implementation of record_testsuite_property)

@Zac-HD
Copy link
Member Author

Zac-HD commented Oct 6, 2020

The POC already uses the (underlying implementation of) the new fixture; the problem is that XML-escaping " as " etc. causes problems when certain characters or escape sequences are part of the actual statistics to report. Which, come to think of it, we could get around just by base64-encoding the string before adding it to the xml... there would be opaque blobs in the report, but that's better than the status quo I guess!

So all we need is for someone to write a whole bunch of tests for this, which I am unmotivated to do at the moment while I'm busy getting HypoFuzz off the ground. Happy to help with and review a PR if someone on your team could write it (based on these tests and whatever xunit xml validation you think appropriate), or as above I could make time if sponsored.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement it's not broken, but we want it to be better interop how to play nicely with other packages
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants