Fix junitxml #3277
Fix junitxml #3277
Conversation
|
Thanks for opening a PR for this! I'll be happy to accept it once all the tests are passing 🙂 Looking at |
| # always be the key for a list of _pytest.reports.TestReport objects | ||
| for test_report in terminalreporter.stats.get("", []): | ||
| if test_report.when == "teardown": | ||
| report(test_report.user_properties) |
There was a problem hiding this comment.
I'm not sure what user_properties's primary purpose is, but it's automatically included in the <testcase> node of a junit report:
https://github.com/pytest-dev/pytest/blob/00ad12b9db8d3309c361061fe1c4878b2e0cd51c/src/_pytest/junitxml.py#L599-L600
This seems wrong, as <testcase> doesn't allow any arbitrary node in xunit2. I think the proper upstream fix should distinguish between report-properties-to-add-to-junit and report-properties-metadata-to-carry-around.
If user_properties were distinguished as such, we could use that instead of the semi-hacky report.__dict__ storing.
| def test_outputs_valid_xunit2_with_xdist(testdir): | ||
| junit_xml = _run_and_get_junit(testdir, "-n2") | ||
|
|
||
| testcase_props = _findall_from_root(junit_xml, "testcase/properties") | ||
| assert len(testcase_props) == 0 |
There was a problem hiding this comment.
Right now, the only thing we really care about with pytest-xdist + junitxml is that <properties> is not inside <testcase> (making it incompatible with xunit2). Currently, the behavior is that <properties> is also not included in <testsuite>, but we don't particularly care about that right now. In fact, if that behavior changes and <properties> starts being added in <testsuite> that would be a win! In which case, we should add the same testsuite_props assertions from the other test. But we don't want to fail the test if <properties> starts being added to <testsuite>
| # < pytest 5.1.0 | ||
| # https://github.com/pytest-dev/pytest/commit/a43ba78d3bde1630a42aaa95776687d3886891e6 | ||
| return junit_xml.findall(f"./{path}") |
There was a problem hiding this comment.
if/when hypothesis drops support for pytest < 5.1, this helper can go away and just inline junit_xml.findall("./testsuite/properties") directly
| return junit_xml.findall(f"./{path}") | ||
|
|
||
|
|
||
| def test_outputs_valid_xunit2(testdir): |
There was a problem hiding this comment.
Ideally, this test (and everything else using pytester) should use the pytester fixture instead of testdir, which is the newer recommended API. But that's blocked on hypothesis dropping support for pytest < 6.2 (pytester was added in 6.2.0)
pytest-dev/pytest@69419cb
|
|
||
| .. code-block:: | ||
|
|
||
| ./build.sh tox py38-custom 3.8.13 -- tests/nocover/test_conjecture_engine.py |
There was a problem hiding this comment.
I didn't see a way to run a single testsuite in existing build.sh tasks. If this can already be done, I can remove the build.sh tox task. This made my local testing iteration much much faster
add_global_property was added in pytest 3, which is probably sufficiently old to drop support pytest-dev/pytest@fa6acdc
|
✨ This is an old work account. Please reference @brandonchinn178 for all future communication ✨ @Zac-HD FYI this should be ready for review! The commits are organized into atomic units of work, so it should be fairly self-explanatory. But as a quick overview, in addition to the actual feature work, i also did the following:
|
|
Thanks again Brandon - this looks great. I've got a conference coming up this Friday-Monday, so I expect to merge the main body of this next week. Alternatively if I have some free time I might cherry-pick pieces of it (with proper credit, of course) which are easier to deal with in isolation and see if breaking it up lets me get it in faster 🙂 |
|
✨ _**This is an old work account. Please reference @brandonchinn178 for all future communication**_ ✨
<!-- updated by mention_personal_account_in_comments.py -->
…---
Sure thing, do whatever works best for you!
On Wed, Mar 30, 2022, 9:30 PM Zac Hatfield-Dodds ***@***.***> wrote:
Thanks again Brandon - this looks great.
I've got a conference coming up this Friday-Monday, so I expect to merge
the main body of this next week. Alternatively if I have some free time I
might cherry-pick pieces of it (with proper credit, of course) which are
easier to deal with in isolation and see if breaking it up lets me get it
in faster 🙂
—
Reply to this email directly, view it on GitHub
<#3277 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AGUC75JR6OMIP2H6B7YFE7TVCUS5RANCNFSM5R7PAMWQ>
.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
✨ This is an old work account. Please reference @brandonchinn178 for all future communication ✨
Pytest 5.4 changed the API such that
item.config._xmlis no longer set directly, but rather set initem.config._store.get(xml_key).