Feature request: root-level attributes for counts

[kwaping]

Please update your code to include counts of tests, failures, etc. at the root level.

Example of desired output:

<testsuites disabled="0" errors="0" failures="3" name="Example" tests="17" time="1.089s">

Thank you!

[davidparsson]

Hi, and thanks for your request!

What is the reason behind it? Are there any tools that require or use these attributes?

[kwaping]

Hmm, I thought there was until you asked that... I'll get some concrete evidence and get back to you on this. Thanks!

[kwaping]

Okay, I found this page which implies that those attributes are NOT needed. Thank you for questioning my request!

http://nelsonwells.net/2012/09/how-jenkins-ci-parses-and-displays-junit-output/

You are free to close this issue.

[davidparsson]

I'm happy to do so. 😎 Thanks!

[richardeschloss]

Hi, it seems as though the Github action for junit reporting dorny/test-reporter may want the attributes on the root level. It currently fails on an error where it's unable to read the "time" property for what I'm guessing is all testsuites. Can you please support this?

[davidparsson]

I'd be perfectly fine adding support for these attributes on the <testsuites> level

However, I think the issue should also be fixed in https://github.com/dorny/test-reporter since these attributes are in fact optional according to the specification.

[richardeschloss]

Hi, thanks for reconsidering this! I did put in a request here https://github.com/dorny/test-reporter/issues/187 to ask them to better handle the error if that's indeed the case.

[SimeonC]

I was thinking I would pick up and do this as it would improve some of our outputs without having to grep or do other shenanigans in the CI. (Semaphoreci also uses the testsuites name attribute and defaults to an unhelpful "Generic Suite" when not provided)

From what I can see the attributes for testsuites are effectively the same as for testsuite just that the tag name and "children" are different.

@davidparsson how do you think it should be implemented?

• copy-pasting everything from the TestSuite class to the Builder class
• Make Builder and TestSuite polymorphic to share the shared functions
• Create a new TestSuites file that Builder will invoke
• just add _attributes to the Builder class and not bother with replicating the other methods

[davidparsson]

@SimeonC, that's much appreciated! I would prefer introducing a TestSuites "class" that calculates these properties, and ideally de-duplicate any logic that would be shared with TestSuite with inheritance (if possible, I've forgotten).

I realize that this would probably be a lot simpler if I would have finished the TypeScript conversion (#24), but things got in the way. 😞

[SimeonC]

Cool, as for the TypeScript conversion, I could just do it with // @ts-check comments and JSDOC comments for the types? That wouldn't require any change to the build process or switching from jshint but would give us type errors when editing in our IDE of choice and provide inbuilt type hints for consumers without the extra build step (and all the TS config hell needed to get those to work 😭 ).

class and extends have been available since node v6 so we should be fine to just use those.

What do you think about that? Just switching to class + extends from prototype would make the change a lot easier to introduce inheritance.

[davidparsson]

Sure, that sounds good. I'd appreciate the help! I've been wanting to do the switch, and there is nothing wrong with doing it incrementally in small steps.

This package has node >= 8 in the "engines" section of package.json so yes, we should be fine.

Please try to keep the conversion in a separate commit that does not add any logic, to make the review easier. Thanks!

[SimeonC]

Interesting point I discovered, class fields are only supported in node 12+ and not by JSHint at all. They're not necessary as we can type all the methods, but do make the type-checking stricter and extended classes easier to work with a bit.

I couldn't find any other way of defining types for the this._xxx properties unless we do go straight to typescript.

class SomeClass {
  /**
    * @type {string} _prop
    */
  _prop; // this is not supported so can't make `this._prop` have nice types
}

[davidparsson]

Ok, I did not know that. But feel free to continue anyway. There is no typing information today, so leaving some out is at least not worse than what we've got.

[davidparsson]

This is now released in 3.2.0 (and properly described in the documentation in 3.2.1). Thanks @SimeonC!