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 XmlReport (JUnit) #190

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add XmlReport (JUnit) #190

wants to merge 2 commits into from

Conversation

hesstobi
Copy link

@hesstobi hesstobi commented Feb 1, 2019

Add an XML output format with --xml. This creates a JUnit xml formated output. This makes it easy to display the test result for CI.

@rbernand
Copy link

Nice MR.
Any update about a merge?

Copy link
Contributor

@rafaelpivato rafaelpivato left a comment

Choose a reason for hiding this comment

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

Amazing work, @hesstobi -- I would urge you to use Python built-in XML instead of lxml. For this project, it is better to keep dependencies at bare minimum. Also, main advantages of lxml would not make sense for us. As we are not parsing XML as well but just generating, we should be safe.

key=key)

if output:
with open(output, 'w+') as output_file:
output_file.write(output_report)
if xml:
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this logic must be part of safety.formatter.report. My understanding is that you did put this here because then we could have more than one output, which sounds great, but then we would need some extra refactoring.

@@ -22,7 +22,8 @@
'Click>=6.0',
'requests',
'packaging',
'dparse>=0.4.1'
'dparse>=0.4.1',
'lxml'
Copy link
Contributor

Choose a reason for hiding this comment

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

I dislike on how lxml depends on a binary library to work. 😕

This file available under the terms of the MIT License as follows:

*******************************************************************************
* Copyright (c) 2010 Thales Corporate Services SAS *
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, but it doesn't sounds necessary to merge this file with such license. I think we can abdicate the schema validation as part of our tests.

@Bokoblin
Copy link

Bokoblin commented Mar 9, 2021

Hi @rafaelpivato , is there any chance to see this PR being finished and merged (without the author)?
We would also have a need for this output in Gitlab CI in my team

@rafaelpivato
Copy link
Contributor

@harlekeyn can you look at this, please?

@0xjjoyy
Copy link

0xjjoyy commented Aug 11, 2021

Hi, any updates regarding the merge?

@yeisonvargasf yeisonvargasf self-assigned this Aug 11, 2021
@yeisonvargasf
Copy link
Member

Hi @0xjjoyy would you like fix/resolve in the code the comment reviews done by Rafael? I can review/test and merge the PR ASAP if those comments are resolved in the code.

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

Successfully merging this pull request may close these issues.

None yet

6 participants