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 support for JUnit reports #91

Merged
merged 5 commits into from
Dec 4, 2023
Merged

Conversation

onlineque
Copy link
Contributor

@onlineque onlineque commented Nov 18, 2023

fixes #10

@onlineque onlineque changed the title WIP: junit reporter Add support for JUnit reports Nov 19, 2023
@kehoecj kehoecj added the OSS Community Contribution Contributions from the OSS Community label Nov 20, 2023
@kehoecj
Copy link
Collaborator

kehoecj commented Nov 21, 2023

@onlineque Let me know when this is ready for review

@onlineque
Copy link
Contributor Author

@kehoecj It should be ready for the review, could you please take a look ?

Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

@onlineque Test coverage has dropped below the required coverage threshold (95%) which is causing the unit tests job to fail. Please improve the coverage so that is meets the threshold.

Copy link
Contributor

@jd4235 jd4235 left a comment

Choose a reason for hiding this comment

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

When I build and run locally to generate a junit output, I see the following entry that appears to use HTML entity codes in the XML. Why are those codes being used? They are not used in the other reporter output formats (json and standard)

     <testcase name="fixtures/subdir/bad.yml validation" classname="config-file-validator" file="fixtures/subdir/bad.yml">
       <failure>
         <message>yaml: line 3: did not find expected &#39;-&#39; indicator</message>
       </failure>
     </testcase>

@jd4235
Copy link
Contributor

jd4235 commented Nov 28, 2023

Please increase your test coverage so that the coverage is above 95% and the pipeline succeeds.

@onlineque
Copy link
Contributor Author

When I build and run locally to generate a junit output, I see the following entry that appears to use HTML entity codes in the XML. Why are those codes being used? They are not used in the other reporter output formats (json and standard)

     <testcase name="fixtures/subdir/bad.yml validation" classname="config-file-validator" file="fixtures/subdir/bad.yml">
       <failure>
         <message>yaml: line 3: did not find expected &#39;-&#39; indicator</message>
       </failure>
     </testcase>

it renders correctly once opened in any browser / tool which is able to render XML. From my point of view this is correct.

@kehoecj
Copy link
Collaborator

kehoecj commented Nov 30, 2023

When I build and run locally to generate a junit output, I see the following entry that appears to use HTML entity codes in the XML. Why are those codes being used? They are not used in the other reporter output formats (json and standard)

     <testcase name="fixtures/subdir/bad.yml validation" classname="config-file-validator" file="fixtures/subdir/bad.yml">
       <failure>
         <message>yaml: line 3: did not find expected &#39;-&#39; indicator</message>
       </failure>
     </testcase>

it renders correctly once opened in any browser / tool which is able to render XML. From my point of view this is correct.

It really only looks good in the browser. Viewing in VSCode, on the command line, or any other tool shows the HTML encoded output which is difficult to read. The primary intent for offering a Junit reporter - at least in my mind - was for use with CI tools like Gitlab CI that can take a JUnit XML as a report.

Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

Code looks good overall with a few comments. I tested locally and was able to generate valid JUnit XML. Tested the output in a sample Gitlab CI job using the JUnit reporter and which worked as expected to include config-file-validation reporting in the Gitlab CI pipeline testing section.

A couple code changes/questions and the remaining question on encoded XML

pkg/reporter/junit_reporter.go Outdated Show resolved Hide resolved
pkg/reporter/junit_reporter.go Outdated Show resolved Hide resolved
@kehoecj kehoecj added the pr-action-requested PR is awaiting feedback from the submitting developer label Nov 30, 2023
@onlineque
Copy link
Contributor Author

When I build and run locally to generate a junit output, I see the following entry that appears to use HTML entity codes in the XML. Why are those codes being used? They are not used in the other reporter output formats (json and standard)

     <testcase name="fixtures/subdir/bad.yml validation" classname="config-file-validator" file="fixtures/subdir/bad.yml">
       <failure>
         <message>yaml: line 3: did not find expected &#39;-&#39; indicator</message>
       </failure>
     </testcase>

it renders correctly once opened in any browser / tool which is able to render XML. From my point of view this is correct.

It really only looks good in the browser. Viewing in VSCode, on the command line, or any other tool shows the HTML encoded output which is difficult to read. The primary intent for offering a Junit reporter - at least in my mind - was for use with CI tools like Gitlab CI that can take a JUnit XML as a report.

I don't think a junit report will anyone read from a command line, it's not its intention, or do I miss something ?
It really looks good even in the Gitlab CI, it renders correctly, I made a test:
image

@kehoecj
Copy link
Collaborator

kehoecj commented Nov 30, 2023

When I build and run locally to generate a junit output, I see the following entry that appears to use HTML entity codes in the XML. Why are those codes being used? They are not used in the other reporter output formats (json and standard)

     <testcase name="fixtures/subdir/bad.yml validation" classname="config-file-validator" file="fixtures/subdir/bad.yml">
       <failure>
         <message>yaml: line 3: did not find expected &#39;-&#39; indicator</message>
       </failure>
     </testcase>

it renders correctly once opened in any browser / tool which is able to render XML. From my point of view this is correct.

It really only looks good in the browser. Viewing in VSCode, on the command line, or any other tool shows the HTML encoded output which is difficult to read. The primary intent for offering a Junit reporter - at least in my mind - was for use with CI tools like Gitlab CI that can take a JUnit XML as a report.

I don't think a junit report will anyone read from a command line, it's not its intention, or do I miss something ? It really looks good even in the Gitlab CI, it renders correctly, I made a test: image

if you look at Pytest and its junit output it does not HTML encode the failure message:

<?xml version="1.0" encoding="utf-8"?><testsuites><testsuite name="pytest" errors="0" failures="1" skipped="0" tests="1" time="0.070" timestamp="2023-11-30T14:32:30.709146" hostname="A6354835"><testcase classname="test" name="test_junitoutput_special_chars" time="0.001"><failure message="Exception: yaml: line 3: did not find expected '-' indicator">def test_junitoutput_special_chars():
&gt;       raise Exception("yaml: line 3: did not find expected '-' indicator")
E       Exception: yaml: line 3: did not find expected '-' indicator

test.py:2: Exception</failure></testcase></testsuite></testsuites>

It also renders just fine on HTML:

image

So it seems like it's possible to get HTML-based and non-html-based outputs to look the same which is what we want if possible

@kehoecj kehoecj removed the pr-action-requested PR is awaiting feedback from the submitting developer label Dec 1, 2023
Copy link
Contributor

@jd4235 jd4235 left a comment

Choose a reason for hiding this comment

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

Nice job. Thanks for fixing the XML output to be human readable everywhere.

Copy link
Collaborator

@kehoecj kehoecj left a comment

Choose a reason for hiding this comment

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

LGTM - thank you for the contribution!

@kehoecj kehoecj merged commit 336d2c5 into Boeing:main Dec 4, 2023
4 checks passed
@kehoecj kehoecj added this to the v1.6.0 milestone Dec 18, 2023
shiina4119 pushed a commit to shiina4119/config-file-validator that referenced this pull request Aug 23, 2024
* initial commit

* junit report test

* tests updated

* tests

* fix html entities encoding, split checkPropertyValidity into several functions

---------

Co-authored-by: Vladimir Siman <online@clew.cz>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
OSS Community Contribution Contributions from the OSS Community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Junit Reporter
3 participants