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

Add captured log msgs to junit xml file #3156

Merged
merged 2 commits into from Feb 6, 2018

Conversation

@thisch
Copy link
Contributor

@thisch thisch commented Jan 26, 2018

For each tests this adds the captured log msgs to the respective
'system-out' tags in the junit xml output file.

Thanks for submitting a PR, your contribution is really appreciated!

Here's a quick checklist that should be present in PRs:

  • Add a new news fragment into the changelog folder
    • name it $issue_id.$type for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of removal, feature, bugfix, vendor, doc or trivial
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."
  • Target: for bugfix, vendor, doc or trivial fixes, target master; for removals or features target features;
  • Make sure to include reasonable tests for your change if necessary

Unless your change is a trivial or a documentation fix (e.g., a typo or reword of a small section) please:

  • Add yourself to AUTHORS, in alphabetical order;
@thisch thisch force-pushed the thisch:junit_xml_log_fix branch 2 times, most recently from 1ec8860 to b6bb49a Jan 26, 2018
@coveralls
Copy link

@coveralls coveralls commented Jan 26, 2018

Coverage Status

Coverage decreased (-0.006%) to 92.622% when pulling 2d0c1e9 on thisch:junit_xml_log_fix into f2fb841 on pytest-dev:features.

@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Jan 27, 2018

Thanks @thisch, looks good so far.

I think we should have an option to control that though, I can imagine use cases where users don't want their JUnitXML files to contain logging information, or want their logging to be directed to stderr instead. How about an ini option: junitxml_logging=no/stdout/stderr, defaulting to no to keep backward compatibility?

@thisch thisch force-pushed the thisch:junit_xml_log_fix branch from b6bb49a to 2e6ffbb Feb 2, 2018
@thisch
Copy link
Contributor Author

@thisch thisch commented Feb 2, 2018

@nicoddemus, yes, adding an ini flag makes sense. I don't know why one would want to write the captured logs to the <system-err> tag, though. Anyway I've added the junit_logging ini option in my latest commit.

For each test this adds the captured log msgs to a system-* tag in the junit
xml output file. The destination of the system-* tag is specified by
junit_logging ini option.
@thisch thisch force-pushed the thisch:junit_xml_log_fix branch from 2e6ffbb to c0ef4a4 Feb 3, 2018
@thisch
Copy link
Contributor Author

@thisch thisch commented Feb 3, 2018

I think we should have an option to control that though, I can imagine use cases where users don't want their JUnitXML files to contain logging information, or want their logging to be directed to stderr instead. How about an ini option: junitxml_logging=no/stdout/stderr, defaulting to no to keep backward compatibility?

@KKoukiou, what do you think?

@KKoukiou
Copy link
Contributor

@KKoukiou KKoukiou commented Feb 5, 2018

@thisch I definitely find it useful. Do you think it might worth to add option about captured log level as well? With my custom fixture for capturing logs, I end up in out memory in my Jenkins when I was capturing all logs including DEBUG. So, without having ability to set default captured log level, unfortunately I can't use this change.

@thisch
Copy link
Contributor Author

@thisch thisch commented Feb 5, 2018

We already have the log_level option, which is taken into account in the junitxml plugin. Have you already tried to set the log_level param to INFO or don't specify it at all (then WARNING is used)?

Copy link
Member

@nicoddemus nicoddemus left a comment

Looks good, thanks @thisch!

@KKoukiou
Copy link
Contributor

@KKoukiou KKoukiou commented Feb 6, 2018

@thisch right, thanks :)

@nicoddemus nicoddemus merged commit 29a074e into pytest-dev:features Feb 6, 2018
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nicoddemus
Copy link
Member

@nicoddemus nicoddemus commented Feb 6, 2018

Thanks again @thisch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants