Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Improve JUnit formatter #4327

Merged
merged 1 commit into from
Dec 12, 2018
Merged

Improve JUnit formatter #4327

merged 1 commit into from
Dec 12, 2018

Conversation

iMarv
Copy link
Contributor

@iMarv iMarv commented Nov 29, 2018

PR checklist

  • Addresses an existing issue: #0000
  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

Overview of change:

This updates the JUnit formatter to use the required property classname.
This change is related to an issue for Gitlab-CE which has problems with displaying JUnit results if they are missing the classname property.

Is there anything you'd like reviewers to focus on?

Not sure if we should add other properties which are required, but cannot be filled with a proper value like time or tests and errors. These could be filled with a dummy value, but I would rather leave them out.

CHANGELOG.md entry:

[enhancement] Improve JUnit formatter

Fix documentation
@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @iMarv! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Hi @iMarv, thanks for sending this! The change looks fine but I'm not familiar with JUnit formatted reports. Could you please link to documentation in this PR thread that shows this change to be correct?

Side note: normally, for functional changes, it's normally good to discuss them in a TSLint issue. This seems straightforward but if it's actually not we should really move discussion to an issue.

@iMarv
Copy link
Contributor Author

iMarv commented Dec 2, 2018

Hey @JoshuaKGoldberg , thank you for looking into it. This is the XML-Schema for JUnit.

You are right regarding an issue, would definitly create one for bigger changes

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

LGTM 😊 though someone with more knowledge on JUnit should weigh in (@mikcsabee?)

@mikcsabee
Copy link
Contributor

As far as I see, the major difference is the testcase name field:
<testcase name="Line 1, Column 14: semicolon">
vs
<testcase name="semicolon" classname="myFile.ts">
The reason why I used the lineAndCharacter + rule instead of the rule only, is because makes the testcases looks a bit unique and it gives a better overview. If you have the same error multiple times, (like 'semicolon') you have to open the failure for every single testcase to get the line and character information. Some applications are not really good to present this case.

@iMarv
Copy link
Contributor Author

iMarv commented Dec 3, 2018

Good point @mikcsabee, thought about that aswell but I feel like this fits the schema better and additionally if there really is a case that someone introduces such an incredible amount of lint issues that opening them becomes tedious, the problem might lie somewhere beyond JUnit

@ericanderson ericanderson merged commit cd5d7d0 into palantir:master Dec 12, 2018
@nfm
Copy link

nfm commented Jan 23, 2019

@iMarv I'm experimenting with using the JUnit formatter on GitLab CI (and saw you commented on this GitLab issue) but it seems to be generating absolute paths in the classname for me, which it looks like doesn't play nicely with the merge request summary.

I see this (note the lack of filenames!):

image

And the generated JUnit XML is:

<testsuites package="tslint"><testsuite name="/absolute/path/to/legal-document/index.tsx"><testcase name="no-console" classname="/absolute/path/to/legal-document/index.tsx"><failure type="error">Calls to 'console.log' are not allowed. Line 36, Column 5</failure></testcase></testsuite></testsuites>

Have you run into this, or are your name and classname values relative (and behaving in the GitLab UI)?

Thanks ❤️

Edit: Possibly related to #3183? I am invoking tslint with yarn run tslint --project . --format junit --out tmp/tslint.xml.

@iMarv
Copy link
Contributor Author

iMarv commented Jan 23, 2019

I'll be honest @nfm, I copied the format from the related issue in eslint which as you said does not seem to be working correctly in gitlab. I did not catch this in time as I did not come around to add this to our pipelines.

Nevertheless, I still think that the format I added with this merge request fits the JUnit Schema, which means I am going to blame gitlab for parsing it incorrectly will mention it in the issue for gitlab-ce

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

Successfully merging this pull request may close these issues.

6 participants