fix issue 1074 and clean up junitxml by RonnyPfannschmidt · Pull Request #1091 · pytest-dev/pytest

RonnyPfannschmidt · 2015-09-29T20:40:41Z

i started by running yapf on junitxml to get my ide warning-free
did the same on its tests and will simplify the test api a bit (i want the xml api out of the picture)

Fix #1074
Fix #641

RonnyPfannschmidt · 2015-10-10T17:49:13Z

@alex can you perhaps take a look if that branch has positive effect on the memory usage of the junit reporting of the large testsuite?

nicoddemus · 2015-10-13T21:45:32Z

Sorry for taking so long to review this @RonnyPfannschmidt! 😅

Overall I really liked the changes you made. As a minor nitpick, I found some PEP-8 errors (mostly too much white-space between functions), but as I said those are minor.

It seems this PR would interest @icemac as well.

nicoddemus · 2015-10-13T21:46:31Z

This docstring no longer reflects reality (it doesn't clean properties).

nicoddemus · 2015-10-13T21:53:01Z

Aside from my minor comments, LGTM. 👍 Feel free to merge if you think my naming suggestions are not worthwhile to make right now. 😄

icemac · 2015-10-14T07:10:00Z

I will have a look into it, after this PR is merged and released. Thanks for looking into it.

RonnyPfannschmidt · 2015-10-14T10:20:25Z

@icemac please take a look at it beforehand, i don't have a good benchmark atm, and i'd like to be sure i'm delivering the right thing

icemac · 2015-10-15T06:07:06Z

@RonnyPfannschmidt I'll test today.

icemac · 2015-10-15T07:31:26Z

@RonnyPfannschmidt I get an exception after some tests are run (summing up the number of successes etc. in the summary line is less than the number of collected items), see the transcript:

bin/py.test -p no:sugar --junitxml=parts/tests.xml --cov-report html --cov-report xml --cov-report term --cov src
============================= test session starts ==============================
platform linux2 -- Python 2.7.9, pytest-2.8.2.dev1, py-1.4.30, pluggy-0.3.1
rootdir: .../instance, inifile: pytest.ini
plugins: gocept.pytestlayer-2.0, cov-1.8.1, remove-stale-bytecode-1.0
collected 4449 items
src/pytest/doc/en/example/assertion/test_failures.py .
src/pytest/doc/en/example/assertion/test_setup_flow_example.py ..
src/pytest/doc/en/example/assertion/global_testmodule_config/test_hello.py .
src/pytest/doc/en/example/costlysetup/sub1/test_quick.py .
src/pytest/doc/en/example/costlysetup/sub2/test_two.py ..
src/pytest/doc/en/example/py2py3/test_py2.py .
src/pytest/testing/test_argcomplete.py ..
src/pytest/testing/test_assertinterpret.py ...........................s
src/pytest/testing/test_assertion.py ..........................................F..F...
src/pytest/testing/test_assertrewrite.py ..........................FF..s........F.....
src/pytest/testing/test_cache.py .................
src/pytest/testing/test_capture.py ...........x.....FFFFF.......F.FF....F.......................................F..F...FFF...
src/pytest/testing/test_collection.py ............x....F.................
src/pytest/testing/test_config.py ........x......................................
src/pytest/testing/test_conftest.py .............F.................................
src/pytest/testing/test_doctest.py ..................................
src/pytest/testing/test_genscript.py s...ss.
src/pytest/testing/test_helpconfig.py .....FF
src/pytest/testing/test_junitxml.py .....F.........................s
src/pytest/testing/test_mark.py ....................................x.................x.
src/pytest/testing/test_monkeypatch.py ..............................
src/pytest/testing/test_nose.py ...................
src/pytest/testing/test_parseopt.py .........................s
src/pytest/testing/test_pastebin.py ...
src/pytest/testing/test_pdb.py ....ssssssssssssFs
src/pytest/testing/test_pluginmanager.py ..F......F................
src/pytest/testing/test_pytester.py x......
src/pytest/testing/test_recwarn.py ...................
src/pytest/testing/test_resultlog.py ...........
src/pytest/testing/test_runner.py ..................sssssss..ssss.......x.........F....
src/pytest/testing/test_runner_xunit.py .............
src/pytest/testing/test_session.py ................
src/pytest/testing/test_skipping.py .................................................
src/pytest/testing/test_terminal.py ..........s.........................s.......................................................
src/pytest/testing/test_tmpdir.py ...........s
src/pytest/testing/test_unittest.py ...................ssssssss...........
src/pytest/testing/cx_freeze/tests/test_doctest.txt .
src/pytest/testing/cx_freeze/tests/test_trivial.py ..
[… many tests of the project under test ran here without any failure …]
INTERNALERROR> Traceback (most recent call last):
INTERNALERROR>   File ".../src/pytest/_pytest/main.py", line 90, in wrap_session
INTERNALERROR>     session.exitstatus = doit(config, session) or 0
INTERNALERROR>   File ".../src/pytest/_pytest/main.py", line 121, in _main
INTERNALERROR>     config.hook.pytest_runtestloop(session=session)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 724, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 338, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 333, in <lambda>
INTERNALERROR>     _MultiCall(methods, kwargs, hook.spec_opts).execute()
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 596, in execute
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File ".../src/pytest/_pytest/main.py", line 146, in pytest_runtestloop
INTERNALERROR>     item.config.hook.pytest_runtest_protocol(item=item, nextitem=nextitem)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 724, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 338, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 333, in <lambda>
INTERNALERROR>     _MultiCall(methods, kwargs, hook.spec_opts).execute()
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 595, in execute
INTERNALERROR>     return _wrapped_call(hook_impl.function(*args), self.execute)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 253, in _wrapped_call
INTERNALERROR>     return call_outcome.get_result()
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 279, in get_result
INTERNALERROR>     _reraise(*ex)  # noqa
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 264, in __init__
INTERNALERROR>     self.result = func()
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 596, in execute
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File ".../src/pytest/_pytest/runner.py", line 65, in pytest_runtest_protocol
INTERNALERROR>     runtestprotocol(item, nextitem=nextitem)
INTERNALERROR>   File ".../src/pytest/_pytest/runner.py", line 72, in runtestprotocol
INTERNALERROR>     rep = call_and_report(item, "setup", log)
INTERNALERROR>   File ".../src/pytest/_pytest/runner.py", line 123, in call_and_report
INTERNALERROR>     hook.pytest_runtest_logreport(report=report)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 724, in __call__
INTERNALERROR>     return self._hookexec(self, self._nonwrappers + self._wrappers, kwargs)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 338, in _hookexec
INTERNALERROR>     return self._inner_hookexec(hook, methods, kwargs)
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 333, in <lambda>
INTERNALERROR>     _MultiCall(methods, kwargs, hook.spec_opts).execute()
INTERNALERROR>   File ".../src/pytest/_pytest/vendored_packages/pluggy.py", line 596, in execute
INTERNALERROR>     res = hook_impl.function(*args)
INTERNALERROR>   File ".../src/pytest/_pytest/junitxml.py", line 320, in pytest_runtest_logreport
INTERNALERROR>     self.update_testcase_duration(report)
INTERNALERROR>   File ".../src/pytest/_pytest/junitxml.py", line 329, in update_testcase_duration
INTERNALERROR>     reporter.duration += getattr(report, 'duration', 0.0)
INTERNALERROR> AttributeError: '_NodeReporter' object has no attribute 'duration'
28 failed, 2178 passed, 73 skipped, 10 xfailed, 9 pytest-warnings in 400.96 seconds

RonnyPfannschmidt · 2015-10-15T08:59:58Z

i see, good find, i'll investigate

RonnyPfannschmidt · 2015-11-24T19:58:43Z

after investigation i'm certain the issue cannot be replicated when running the correct version of pytest

@nicoddemus @hpk42 this one seems ready for merge

nicoddemus · 2015-11-24T20:07:48Z

Should this be node_reporters?

nicoddemus · 2015-11-24T20:10:22Z

Could be that the problem reported by @icemac be due to _NodeReporter.finalize() being called twice? That would explain the AttributeError.

RonnyPfannschmidt · 2015-11-24T20:13:55Z

i think its related to the issue about duplicate tests

nicoddemus · 2015-11-24T20:17:25Z

Good idea. But then it should be simple to verify it, just pass the same file name to pytest twice and you should see the same error. If that's the case we should fix this branch, as we just decided that running the same set of tests more than once is a valid use case.

RonnyPfannschmidt · 2015-11-24T20:19:28Z

i just tried, and it will err out

nicoddemus · 2015-11-25T01:41:30Z

The current master will repeat a new testcase tag when you execute the same test twice:

<?xml version="1.0" encoding="utf-8"?>
<testsuite errors="0" failures="0" name="pytest" skips="0" tests="2" time="0.013">
  <testcase classname="tmp.test_foo1" file="tmp\test_foo1.py" line="0" name="test_1" time="0.0010001659393310547"/>
  <testcase classname="tmp.test_foo1" file="tmp\test_foo1.py" line="0" name="test_1" time="0.0010001659393310547"/>
</testsuite>

I was thinking, to support the same case here, perhaps we could:

instead of a node_reporters_ordered list, just keep a list of xml strings which have been finished so far (_NodeReporter.to_xml()); lets call that xml_test_cases (just for exemplification, feel free to use a more appropriate name);
instead of calling _NodeReporter.finalize() when you see a "teardown" event in pytest_runtest_logreport, just call to_xml(), store it in xml_test_cases and discard the _NodeReporter instance for that nodeid. We won't even need a finalize method anymore.

This will change the order of the testsuite tags in the report, where previously they would appear in the order they were "setup" first, but now they will appear in the order they where "teardown" first, but I don't think this is an issue at all.

What do you think?

RonnyPfannschmidt · 2015-11-25T06:08:26Z

Will break on xdist

nicoddemus · 2015-11-25T06:10:25Z

Will break on xdist

You mean in the normal usage, or for duplicated test ids?

RonnyPfannschmidt · 2015-11-25T06:22:39Z

For duplicate tests, they can interleave wrt reports

nicoddemus · 2015-11-25T06:29:50Z

Thought about that too, but that happens today already IIUC (both master and this PR)... my proposal would at least avoid the internal error.

RonnyPfannschmidt · 2015-11-25T07:24:18Z

I plan to fix both by extending the key with the slave ID and implemening your proposal

nicoddemus · 2015-11-25T14:15:44Z

Sounds good, thanks!

But how will you know which item was executed in which slave? We don't have how to get that information now, I think.

nicoddemus · 2015-11-25T14:58:48Z

Oooh didn't know about this line:

    def slave_testreport(self, node, rep):
        if rep.when == "call" or (rep.when == "setup" and not rep.passed):
            self.sched.remove_item(node, rep.item_index, rep.duration)
        # self.report_line("testreport %s: %s" %(rep.id, rep.status))
        rep.node = node   # <----
        self.config.hook.pytest_runtest_logreport(report=rep)
        self._handlefailures(rep)

So the report object knows where each test executed, nice. Your suggestion seems perfect then. 😎

RonnyPfannschmidt · 2015-11-25T16:22:51Z

that little gem came directly from @hpk42 :)

nicoddemus · 2015-12-07T16:16:01Z

I think this change warrants a CHANGELOG entry

nicoddemus · 2015-12-07T20:49:53Z

I think after you add a CHANGELOG entry you can merge this @RonnyPfannschmidt

RonnyPfannschmidt · 2015-12-07T20:50:36Z

im on it

+ review: should we allow the same key multile times

…ixes issue pytest-dev#1074

RonnyPfannschmidt · 2015-12-07T20:59:34Z

rebased on to of master and added the entry, i'll wait for travis then push the button

nicoddemus · 2015-12-07T21:00:08Z

👍

fix issue 1074 and clean up junitxml

RonnyPfannschmidt changed the title ~~[wip] fix issue 1074 and clean up junitxml~~ fix issue 1074 and clean up junitxml Oct 10, 2015

RonnyPfannschmidt assigned nicoddemus Oct 10, 2015

nicoddemus reviewed Oct 13, 2015
View reviewed changes

Comment thread _pytest/junitxml.py Outdated

nicoddemus Oct 13, 2015

Copy link
Copy Markdown

Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This docstring no longer reflects reality (it doesn't clean properties).

nicoddemus reviewed Nov 24, 2015
View reviewed changes

RonnyPfannschmidt added 19 commits December 7, 2015 21:54

yapf junitxml

02f5def

junitxml: reverse the if/else logic for failure appending

2a31df0

junitxml: yapf clean the tests

b8df544

junitxml: simplify tests by creating a api wrapper

80bcf8d

junitxml: simplify the api used for testing junitml

9128fec

junitxml restrucutre stat generation - use node tags

d0107c8

junitxml: remove debug print

0664ae1

junitxml: introduce nodereporter and track durations/properties there

1f609f9

junitxml: use node.warn to ensure fslocations

faed54d

junitxml: keep track of custom property insert order

9b04958

+ review: should we allow the same key multile times

junitxml: intermediate, move testcase generation to NodeReporter

0528e5b

reintroduce junitxml report order and debug cleanups

5f5a799

finalize nodereporters by throwing away the intermediate xml nodes, f…

518c88f

…ixes issue pytest-dev#1074

junitxml: fix python3 compat of the tests

5eef6a2

junitxml: follow Bruno's attribute/method naming hint from the review

0aa5410

junitxml: correct docstring of make_properties_node

09cc45b

Junitxml: correct node reporter attribute names

9ea7826

add xfailing test for double test id failure

04e9ae7

handle duplicate test ids via collection and xdist each reporting

7b7737b

RonnyPfannschmidt force-pushed the fix-1074 branch from 21c9979 to 7b7737b Compare December 7, 2015 20:54

add changelog entry

fde2a6f

RonnyPfannschmidt added a commit that referenced this pull request Dec 7, 2015

Merge pull request #1091 from RonnyPfannschmidt/fix-1074

ffa5725

fix issue 1074 and clean up junitxml

RonnyPfannschmidt merged commit ffa5725 into pytest-dev:master Dec 7, 2015

RonnyPfannschmidt deleted the fix-1074 branch April 23, 2016 10:22

RonnyPfannschmidt mentioned this pull request Feb 2, 2017

PR 1091 generates changes in junitxml output which cause incompatibility with Jenkins Junit plugin #2228

Closed

Conversation

RonnyPfannschmidt commented Sep 29, 2015

Uh oh!

RonnyPfannschmidt commented Oct 10, 2015

Uh oh!

nicoddemus commented Oct 13, 2015

Uh oh!

nicoddemus Oct 13, 2015

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Oct 13, 2015

Uh oh!

icemac commented Oct 14, 2015

Uh oh!

RonnyPfannschmidt commented Oct 14, 2015

Uh oh!

icemac commented Oct 15, 2015

Uh oh!

icemac commented Oct 15, 2015

Uh oh!

RonnyPfannschmidt commented Oct 15, 2015

Uh oh!

RonnyPfannschmidt commented Nov 24, 2015

Uh oh!

nicoddemus Nov 24, 2015

Choose a reason for hiding this comment

Uh oh!

RonnyPfannschmidt Nov 24, 2015

Choose a reason for hiding this comment

Uh oh!

nicoddemus commented Nov 24, 2015

Uh oh!

RonnyPfannschmidt commented Nov 24, 2015

Uh oh!

nicoddemus commented Nov 24, 2015

Uh oh!

RonnyPfannschmidt commented Nov 24, 2015

Uh oh!

nicoddemus commented Nov 25, 2015

Uh oh!

RonnyPfannschmidt commented Nov 25, 2015

Uh oh!

nicoddemus commented Nov 25, 2015

Uh oh!

RonnyPfannschmidt commented Nov 25, 2015

Uh oh!

nicoddemus commented Nov 25, 2015

Uh oh!

RonnyPfannschmidt commented Nov 25, 2015

Uh oh!

nicoddemus commented Nov 25, 2015

Uh oh!

nicoddemus commented Nov 25, 2015

Uh oh!

RonnyPfannschmidt commented Nov 25, 2015

Uh oh!

nicoddemus commented Dec 7, 2015

Uh oh!

nicoddemus commented Dec 7, 2015

Uh oh!

RonnyPfannschmidt commented Dec 7, 2015

Uh oh!

RonnyPfannschmidt commented Dec 7, 2015

Uh oh!

nicoddemus commented Dec 7, 2015

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants