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

JUnit reporter XML output missing required classname attribute on testcase element #11068

Closed
micheee opened this issue Nov 8, 2018 · 15 comments

Comments

Projects
None yet
8 participants
@micheee
Copy link

commented Nov 8, 2018

When producing JUnit XML output, the resulting XML file seems to be missing required attributes.

I found the following XML Schema Definition that indicates the required fields.

While I don't think it is necessary to support every optional attribute, I think it would make sense to provide those that are mandatory.

I stumbled across this, as I tried to integrate ESlint output in GitLab CI, but GitLab CI will not display the report, as it requires the classname attribute to be present.

There is also an issue filed over at GitLab, but maybe it was more obvious to fix the issue in ESlint.

If you think here’s the right place to fix this, I'd be happy to contribute a Pull Request.

Tell us about your environment

  • ESLint Version: v5.8.0
  • Node Version: v10.12.0
  • npm Version: 6.4.1

What parser (default, Babel-ESLint, etc.) are you using? default

Please show your full configuration: ø

Configuration
#  cat eslintconfig.json
{
  "plugins": [],
  "parserOptions": {
    "ecmaVersion": 6,
    "sourceType": "module"
  },
  "extends": ["eslint:recommended"]
}
#  cat test.js
console.log('foo');

What did you expect to happen?

Expected the attribute classname to be present on element <testcase /> — I suggest that classname will be set to basename of the parents <testsuite/> file attribute.

<testsuites>
<testsuite package="org.eslint" time="0" tests="1" errors="1" name="/path/to/test.js">
  <testcase time="0" classname="test" name="org.eslint.no-console">…</testcase>
</testsuite>
</testsuites>

What actually happened? Please include the actual, raw output from ESLint.

<!-- ⇒  ./node_modules/.bin/eslint -c eslintconfig.json -f junit test.js -->
<testsuites>
<testsuite package="org.eslint" time="0" tests="1" errors="1" name="/path/to/test.js">
  <testcase time="0" name="org.eslint.no-console">… </testcase>
</testsuite>
</testsuites>

Are you willing to submit a pull request to fix this bug?

👍

@eslint eslint bot added the triage label Nov 8, 2018

@platinumazure

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

Hi @micheee, thanks for the issue.

I agree this seems like a bug, but I'm not sure what value we should put in there. Do you have any thoughts?

@not-an-aardvark

This comment has been minimized.

Copy link
Member

commented Nov 8, 2018

How about an empty string?

@evanlucas

This comment has been minimized.

Copy link

commented Nov 8, 2018

We ran into this also. To me, the filename would be useful

@micheee

This comment has been minimized.

Copy link
Author

commented Nov 9, 2018

We ran into this also. To me, the filename would be useful

The Filename — maybe without the extension — would be my proposal as well.

Any other thoughts?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Nov 9, 2018

Thanks everyone for your suggestions!

Re: filename: It looks like the filename is captured in the name attribute of the <testsuite> element. Would adding the file name as a "classname" (which seems to expect a Java class) really be useful?

And I guess I'm a bit skeptical about looking at other test runners for examples. Not sure if those are really apples-to-apples comparisons.

If empty string will work (i.e., if the attribute being "required" in the XSD means the attribute just needs to be present, but it doesn't have to have a non-empty value), I could get behind that. I could also support a value like "Linter", since that's the name of our (ES6) class that is doing the linting.

@micheee Can you test out whether classname="" will work in GitLab CI? Thanks!

@micheee

This comment has been minimized.

Copy link
Author

commented Nov 12, 2018

@platinumazure

@micheee Can you test out whether classname="" will work in GitLab CI? Thanks!

Sorry I've been in the mountains for the weekend — I will check this out today and report back.

@micheee

This comment has been minimized.

Copy link
Author

commented Nov 13, 2018

Long story short:

Setting classname to empty string would be valid, but won't help GitLab as it uses classname + name as its grouping key when displaying the errors.

So I'd propose to change the value of classname to the name attribute of its parent testsuite element, ore more profane: the full path to the file under consideration.

My example report contains 18 errors of the category org.eslint.no-extra-boolean-cast and 1 error of category org.eslint.no-unused-var however only 2 total failures are reported in the UI:

image


If I however change the classname to the file path (I'd vote for that because filenames like index.js are very likely to show up more than once in any given codebase) the results are shown mostly correct in the UI — multiple errors of the same category per File will only show up once — but I don't think that's something eslint should tackle.

image


Feedback appreciated.

@cbleslie

This comment has been minimized.

Copy link

commented Nov 13, 2018

mostly correct in the UI — multiple errors of the same category per File will only show up once —

@micheee,
90% of something, is better than 0% of nothing, hombré. I'll take what I can get!

@micheee

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

What do you think, I could send a PR containing the required change?

@platinumazure

This comment has been minimized.

Copy link
Member

commented Nov 15, 2018

@micheee What value would you want to use? I don't think we came to consensus on that, did I miss something?

@micheee

This comment has been minimized.

Copy link
Author

commented Nov 15, 2018

@platinumazure Sorry, I did not want to rush things.
The empty string turned out to be not too useful in terms of GitLab’s CI Panel (though valid w.r.t. to the schema).

I am locally playing around with using the filepath, but trimmed relative to the project root and without the filename suffix to have a things a little more readable for humans.

Currently it I imagine something like this:

<?xml version="1.0" encoding="utf-8"?>
<testsuites>
  <testsuite package="org.eslint" time="0" tests="1" errors="0" name="/Users/michael/Code/foo/foo-dev/src/frontend/controllers/PageController.js">
    <testcase time="0" name="org.eslint.no-console" classname="src/frontend/controllers/PageController">
      <failure message="Unexpected console statement."><![CDATA[line 41, col 9, Error - Unexpected console statement. (no-console)]]></failure>
    </testcase>
  </testsuite>
</testsuites>

Still this decision is kind of arbitrary as obviously the reporting format was invented for Java.

Feedback is very welcome. :)

@ChristianGruen

This comment has been minimized.

Copy link

commented Nov 28, 2018

Sounds like a reasonable proposal. Any news here?

@iMarv

This comment has been minimized.

Copy link

commented Nov 29, 2018

If I am not mistaken by the JUnit schema, package should be a property of testsuites rather than testsuite

@eslint eslint bot closed this Dec 30, 2018

@eslint eslint bot added the auto closed label Dec 30, 2018

@eslint

This comment has been minimized.

Copy link
Contributor

commented Dec 30, 2018

Unfortunately, it looks like there wasn't enough interest from the team
or community to implement this change. While we wish we'd be able to
accommodate everyone's requests, we do need to prioritize. We've found
that issues failing to reach accepted status after 21 days tend to
never be accepted, and as such, we close those issues.
This doesn't mean the idea isn't interesting or useful, just that it's
not something the team can commit to.

Thanks for contributing to ESLint and we appreciate your understanding.

@iMarv iMarv referenced this issue Jan 23, 2019

Merged

Improve JUnit formatter #4327

2 of 4 tasks complete

hspazio added a commit to hspazio/eslint that referenced this issue May 7, 2019

hspazio added a commit to hspazio/eslint that referenced this issue May 7, 2019

hspazio added a commit to hspazio/eslint that referenced this issue May 7, 2019

hspazio added a commit to hspazio/eslint that referenced this issue May 27, 2019

ilyavolodin added a commit that referenced this issue May 27, 2019

New: Add classname attribute to JUnit testcase (refs #11068) (#11683)
* New: Add classname attribute to JUnit testcase (refs #11068)

* Enforce POSIX paths for more test consistency

* Enforce path.posix.join for classname attribute
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.