Toggle JUnit behavior with INI option #4511

jhunkeler · 2018-12-05T14:58:35Z

Continuation of #4493
cc: @nicoddemus @RonnyPfannschmidt

Known issues so far:

A few failing tests
Debugging output needs to be removed
There's no good way to inform either _NodeReporter or LogXML about which family they should choose. Not without horrifically breaking either of them.
Use of module-global variable

Note:
Despite the tests failing this does emit different/corrected XML for the selected junit_family=[(old|xunit1),xunit2].

jhunkeler · 2018-12-05T15:02:51Z

Also, I left #4493 intact in case this PR is considered "too crazy".

RonnyPfannschmidt · 2018-12-05T15:13:00Z

at first glance interesting approach - unfortunately i might not be able to review it in a timely manner this week

RonnyPfannschmidt

after doing a first deeper review i really like the approach,
it keeps things as they are reasonably simple

the reduction of the detail level of the testcases doesn't sit too well with me, but i would prefer if someone with a deep need for correct and detailed junitxml sorted that out

RonnyPfannschmidt · 2018-12-12T10:27:21Z

src/_pytest/junitxml.py

+    for kl, vl in left.items():
+        for kr, vr in right.items():
+            if not isinstance(vl, list):
+                raise NotImplementedError(type(vl))


should be a TypeError

RonnyPfannschmidt · 2018-12-12T10:29:43Z

src/_pytest/junitxml.py

+        # This includes custom attributes, because they are not valid here.
+        # TODO: Convert invalid attributes to properties to preserve "something"
+        temp_attrs = {}
+        for key in self.attrs.keys():


this should jsut use items() to avoid double lookup

RonnyPfannschmidt · 2018-12-12T10:30:29Z

src/_pytest/junitxml.py

+merge_family(families["xunit1"], families["_base_old"])
+
+# Alias "old" to xUnit 1.x
+families["old"] = families["xunit1"]


instead of old i propose the name legacy

nicoddemus · 2018-12-13T15:49:44Z

src/_pytest/junitxml.py

                tests=numtests,
                time="%.3f" % suite_time_delta,
            ).unicode(indent=0)
        )
        logfile.close()

+        # TODO: GET RID OF


nicoddemus · 2018-12-13T15:50:31Z

changelog/3547.bugfix.rst

@@ -0,0 +1 @@
+``--junitxml`` emits XML data compatible with JUnit's offical schema releases.


Please add a link to the official document

codecov · 2019-01-14T15:24:19Z

Codecov Report

Merging #4511 into features will decrease coverage by 3.75%.
The diff coverage is 95.23%.

@@             Coverage Diff             @@
##           features   #4511      +/-   ##
===========================================
- Coverage     95.76%     92%   -3.76%     
===========================================
  Files           111     111              
  Lines         24683   24636      -47     
  Branches       2446    2445       -1     
===========================================
- Hits          23637   22666     -971     
- Misses          739    1566     +827     
- Partials        307     404      +97

Flag	Coverage Δ
#docs	`29.54% <23.8%> (+0.05%)`	⬆️
#doctesting	`29.54% <23.8%> (+0.05%)`	⬆️
#linting	`29.54% <23.8%> (+0.05%)`	⬆️
#linux	`92% <95.23%> (-3.59%)`	⬇️
#nobyte	`?`
#numpy	`?`
#pexpect	`?`
#py27	`?`
#py34	`?`
#py35	`?`
#py36	`?`
#py37	`91.91% <95.23%> (-2.02%)`	⬇️
#trial	`?`
#windows	`?`
#xdist	`91.91% <95.23%> (-1.9%)`	⬇️

Impacted Files	Coverage Δ
testing/test_junitxml.py	`97.8% <100%> (-0.16%)`	⬇️
src/_pytest/junitxml.py	`93.92% <92.1%> (-0.48%)`	⬇️
src/_pytest/_code/_py2traceback.py	`20% <0%> (-71.12%)`	⬇️
testing/test_pdb.py	`48.9% <0%> (-50.13%)`	⬇️
src/_pytest/compat.py	`75.75% <0%> (-21.22%)`	⬇️
src/_pytest/unittest.py	`73.71% <0%> (-20.58%)`	⬇️
testing/python/approx.py	`80.45% <0%> (-19.18%)`	⬇️
src/_pytest/debugging.py	`62.83% <0%> (-18.92%)`	⬇️
src/_pytest/recwarn.py	`83.6% <0%> (-14.76%)`	⬇️
src/_pytest/python_api.py	`83.26% <0%> (-14.23%)`	⬇️
... and 51 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5dcb370...2a09992. Read the comment docs.

* Remove non-standard testcase elements: 'file' and 'line' * Replace testcase element 'skips' with 'skipped' * Time resolution uses the standard format: 0.000 * Tests use corrected XML output with proper attributes

* "legacy" is no longer a copy of "xunit1" * Attempts to use "legacy" will redirect to "xunit1" * record_xml_attribute is not compatible outside of legacy family * Replace call to method/override raw() with to_xml()

nicoddemus

Looks great, thanks @jhunkeler!

nicoddemus · 2019-01-15T22:46:09Z

Can we remove the WIP status from the title or is there still work to be done in this PR?

jhunkeler · 2019-01-16T04:04:26Z

Thanks :)

I do have one concern though. This PR tries to retain backwards compatibility but it did fall short in one place. Originally this code used skips to denote a skipped test, however now it has been renamed to skipped (to satisfy the xunit2 requirement).

That's to say with xunit1 enabled, and with the way the XML classes were written, you'll end up with skipped in the XML output rather than skips. Was skips ever a hard requirement for an older (ancient) version of JUnit? Or part of an official schema that no longer exists? I wasn't able to track down any documentation anywhere... If skips needs to be retained for perfect 1:1 compatibility I fear the if/else branches required to make that work (or any hacky solution) will make the code unreadable.

I'm not sure if this is a showstopper so I'd like to get an opinion on the matter.

RonnyPfannschmidt · 2019-01-16T06:09:45Z

a nicely self-contained if/else with a todo comment and followup issue is a good starting point - eventually we ought to investigate if it can be done nicely with inheritance or another mechanism

nicoddemus · 2019-01-16T08:03:22Z

I agree, if we move the logic to a free function I think the code still will be ok

nicoddemus · 2019-01-23T22:16:22Z

I wasn't able to track down any documentation anywhere...

After looking around at some xUnit schemas, indeed skips does not appear to be part of any of the xUnit "standards". I've been using Jenkins for years now and never seen it displaying a "skipped" test before, so I suspect our current xUnit format is faulty anyway.

I propose we merge this as is and deal with any problems later in a bug-fix release, if they ever happen.

ianling · 2019-03-08T18:21:42Z

Due to the XML file containing the word "skipped" instead of "skips", as of pytest version 4.2.0, the JUnit plugin in Jenkins fails to parse the XML file if you are using the post-build step "Publish JUnit reports generated with handlebars". I don't make use of the webpage that build step generates, so I just removed the step from my builds.

In case anyone happens to come across this error and spend several hours troubleshooting it, there's the solution. Downgrade Pytest or get rid of that build step in Jenkins.

nicoddemus · 2019-03-08T19:33:09Z

Hi @ianling,

Did you try to set the junit_family=legacy ini option?

RonnyPfannschmidt approved these changes Dec 12, 2018

View reviewed changes

nicoddemus reviewed Dec 13, 2018

View reviewed changes

nicoddemus mentioned this pull request Dec 13, 2018

Bugfix: junitxml violates Jenkins/xUnit JUnit schema #4493

Closed

jhunkeler mentioned this pull request Jan 14, 2019

test_environmetal is failing, but CI test suite passing spacetelescope/jwst#3028

Closed

jhunkeler force-pushed the junit-strict branch from bd44911 to 69c5fde Compare January 14, 2019 15:22

jhunkeler added 10 commits January 14, 2019 22:58

Emit JUnit compatible XML

af2ee1e

* Remove non-standard testcase elements: 'file' and 'line' * Replace testcase element 'skips' with 'skipped' * Time resolution uses the standard format: 0.000 * Tests use corrected XML output with proper attributes

Add junit_family config option

2e551c3

Handle backwards-compatiblity

335cc5d

Replace family "old" with "legacy"

343430c

Raise TypeError instead of NotImplementedError if not list type

8937e39

Change family behavior:

aaa7d36

* "legacy" is no longer a copy of "xunit1" * Attempts to use "legacy" will redirect to "xunit1" * record_xml_attribute is not compatible outside of legacy family * Replace call to method/override raw() with to_xml()

Adds xunit2 version of test_record_attribute

4ecf293

Update comment text

bcacc40

Ensure xml object is viable before testing family type

8967976

Update changelog

85c5fa9

jhunkeler force-pushed the junit-strict branch from 2a09992 to 85c5fa9 Compare January 15, 2019 03:58

nicoddemus approved these changes Jan 15, 2019

View reviewed changes

nicoddemus mentioned this pull request Jan 15, 2019

Use fixtures to invoke xunit-style fixtures #4091

Merged

nicoddemus added this to the 4.2 milestone Jan 23, 2019

nicoddemus changed the title ~~WIP: Toggle JUnit behavior with INI option~~ Toggle JUnit behavior with INI option Jan 24, 2019

nicoddemus merged commit 9905a73 into pytest-dev:features Jan 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Toggle JUnit behavior with INI option #4511

Toggle JUnit behavior with INI option #4511

jhunkeler commented Dec 5, 2018

jhunkeler commented Dec 5, 2018

RonnyPfannschmidt commented Dec 5, 2018

RonnyPfannschmidt left a comment

RonnyPfannschmidt Dec 12, 2018

RonnyPfannschmidt Dec 12, 2018

RonnyPfannschmidt Dec 12, 2018

nicoddemus Dec 13, 2018

nicoddemus Dec 13, 2018

codecov bot commented Jan 14, 2019 •

edited

Loading

nicoddemus left a comment

nicoddemus commented Jan 15, 2019

jhunkeler commented Jan 16, 2019

RonnyPfannschmidt commented Jan 16, 2019

nicoddemus commented Jan 16, 2019

nicoddemus commented Jan 23, 2019

ianling commented Mar 8, 2019 •

edited

Loading

nicoddemus commented Mar 8, 2019

		@@ -0,0 +1 @@
		``--junitxml`` emits XML data compatible with JUnit's offical schema releases.

Toggle JUnit behavior with INI option #4511

Toggle JUnit behavior with INI option #4511

Conversation

jhunkeler commented Dec 5, 2018

jhunkeler commented Dec 5, 2018

RonnyPfannschmidt commented Dec 5, 2018

RonnyPfannschmidt left a comment

Choose a reason for hiding this comment

RonnyPfannschmidt Dec 12, 2018

Choose a reason for hiding this comment

RonnyPfannschmidt Dec 12, 2018

Choose a reason for hiding this comment

RonnyPfannschmidt Dec 12, 2018

Choose a reason for hiding this comment

nicoddemus Dec 13, 2018

Choose a reason for hiding this comment

nicoddemus Dec 13, 2018

Choose a reason for hiding this comment

codecov bot commented Jan 14, 2019 • edited Loading

Codecov Report

nicoddemus left a comment

Choose a reason for hiding this comment

nicoddemus commented Jan 15, 2019

jhunkeler commented Jan 16, 2019

RonnyPfannschmidt commented Jan 16, 2019

nicoddemus commented Jan 16, 2019

nicoddemus commented Jan 23, 2019

ianling commented Mar 8, 2019 • edited Loading

nicoddemus commented Mar 8, 2019

codecov bot commented Jan 14, 2019 •

edited

Loading

ianling commented Mar 8, 2019 •

edited

Loading