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 #349

Closed
wants to merge 16 commits into
base: master
from

Conversation

Projects
None yet
7 participants
@jon077
Copy link

jon077 commented Mar 27, 2012

I added a junit reporter for integration with Bamboo, Hudson and other CI servers. The code is closely modeled after the xunit reporter.

Please review. If you like what you see, please request changes or merge as is.

Thanks for contributing the library. We hope to get a lot of mileage out of it.

--jon

.gitignore Outdated
@@ -1,7 +1,4 @@
coverage.html

This comment has been minimized.

@tj

tj Mar 27, 2012

Contributor

why remove these other ones?

This comment has been minimized.

@jon077

jon077 Apr 25, 2012

Author

I didn't intentionally remove the other ones.

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Mar 27, 2012

doesn't xunit work with hudson?

@jon077

This comment has been minimized.

Copy link
Author

jon077 commented Apr 9, 2012

It looks like it requires a plugin:
http://wiki.hudson-ci.org/display/HUDSON/xUnit+Plugin

I mostly use Bamboo, which is easy to setup with the junit format.

--jon

On Tuesday, March 27, 2012 at 11:03 AM, TJ Holowaychuk wrote:

doesn't xunit work with hudson?


Reply to this email directly or view it on GitHub:
#349 (comment)

}else{
console.log('test.state not known, test not recorded: ' + test);
}

This comment has been minimized.

@tj

tj Apr 25, 2012

Contributor

style-wise this is all way off the rest of the project, try and copy what the rest is like. For example there's no need to have the clean function in scope since it's not enclosing anything, and things like if (test.failed == true can just be if (test.failed, } else { vs }else{ etc

@wolfeidau

This comment has been minimized.

Copy link

wolfeidau commented Jun 14, 2012

Do these patches still need some tidy before the push request will be accepted?

I am chasing something similar, if required I am happy to tidy them up if that is all that is required.

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Jun 14, 2012

yeah it's just quite different from the rest of the project's style, i'll comment some more

.gitignore Outdated
testing
_mocha.js
.project
.settings

This comment has been minimized.

@tj

tj Jun 14, 2012

Contributor

these changes need reverting


var tests = [];

var clean = function(str) {

This comment has been minimized.

@tj

tj Jun 14, 2012

Contributor

I think 3 reporters use this now, so we should put it in the utils file

tests.push(test);
});

runner.on('end', function(suite){

This comment has been minimized.

@tj

tj Jun 14, 2012

Contributor

unless 100% necessary we should stream the output, instead of collecting an array and outputting at the end. Also use 'pass' / 'fail' events like the other reporters to differentiate

This comment has been minimized.

@jon077

jon077 Jun 15, 2012

Author

Thanks for the feedback. I apologize for dropping the ball.

I made the recommended changes, except for the refactor to the util. I am going to tackle it next.

--jon

On Thursday, June 14, 2012 at 1:00 AM, TJ Holowaychuk wrote:

  • .replace(/\s+}$/, '');
    +
  • var spaces = str.match(/^\n?( *)/)[1].length
  • , re = new RegExp('^ {' + spaces + '}', 'gm');
    +
  • str = str.replace(re, '');
    +
  • return str;
  • };
    +
  • runner.on('test end', function(test){
  • tests.push(test);
  • });
    +
  • runner.on('end', function(suite){

unless 100% necessary we should stream the output, instead of collecting an array and outputting at the end.


Reply to this email directly or view it on GitHub:
https://github.com/visionmedia/mocha/pull/349/files#r982317

This comment has been minimized.

@jon077

jon077 Jun 15, 2012

Author

My changes are complete. Please review.

Thanks for taking the time to review the changes and let me know if there is anything else I can do to help.

--jon

On Thursday, June 14, 2012 at 11:45 PM, Jonathan Stockdill wrote:

Thanks for the feedback. I apologize for dropping the ball.

I made the recommended changes, except for the refactor to the util. I am going to tackle it next.

--jon

On Thursday, June 14, 2012 at 1:00 AM, TJ Holowaychuk wrote:

  • .replace(/\s+}$/, '');
    +
  • var spaces = str.match(/^\n?( *)/)[1].length
  • , re = new RegExp('^ {' + spaces + '}', 'gm');
    +
  • str = str.replace(re, '');
    +
  • return str;
  • };
    +
  • runner.on('test end', function(test){
  • tests.push(test);
  • });
    +
  • runner.on('end', function(suite){

unless 100% necessary we should stream the output, instead of collecting an array and outputting at the end.


Reply to this email directly or view it on GitHub:
https://github.com/visionmedia/mocha/pull/349/files#r982317

@wolfeidau

This comment has been minimized.

Copy link

wolfeidau commented Jun 15, 2012

Thanks for that!

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Jun 15, 2012

good enough :D i'll merge and refactor

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Jun 15, 2012

hmm might have to rebase it's not applying to master

@wolfeidau

This comment has been minimized.

Copy link

wolfeidau commented Jun 16, 2012

Thanks for trying to get his into mocha will make it much easier to integrate into existing CI servers.

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Jun 16, 2012

cleaning things up a bit, can junit output have multiple testsuite tags or what's the best way to describe the hierarchy? or does it just expect a flat list of test cases?

@wolfeidau

This comment has been minimized.

Copy link

wolfeidau commented Jun 16, 2012

The xsd for the xml is located at

https://svn.jenkins-ci.org/trunk/hudson/dtkit/dtkit-format/dtkit-junit-model/src/main/resources/com/thalesgroup/dtkit/junit/model/xsd/junit-4.xsd

This illustrates that you can have many testsuite tags wrapped in a testsuites tag.

This thread on stack overflow has some more examples.

http://stackoverflow.com/questions/4922867/junit-xml-format-specification-that-hudson-supports

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Jun 16, 2012

hmm not a very stream-friendly format, i'll see what i can do

@wolfeidau

This comment has been minimized.

Copy link

wolfeidau commented Jun 16, 2012

Yeah I am just having a look over the ruby gem.

https://github.com/nicksieger/ci_reporter

Not sure if it helps with ideas on how to architect the code.

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Jun 16, 2012

something like 703cef4 ?

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Jun 16, 2012

if we want to add durations to the testsuite tags then we'll have to do our own looping and stacks which is kinda lame, I like to avoid that when possible since the events take care of that for us

@wolfeidau

This comment has been minimized.

Copy link

wolfeidau commented Jun 16, 2012

Yeah it doesn't seem to fit the stream model with the summary being written before the testsuite / testsuites items have been written.

Would it be wiser to write a intermediate flattened file then transform it, only other alternative is to store this in a data structure which is then has the report built from it.

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Jun 16, 2012

yeah I guess that explains why Jon was building it at the end. That's too bad

@jon077

This comment has been minimized.

Copy link
Author

jon077 commented Jun 18, 2012

I did start with something like:
703cef4

but ended up moving everything to the end in commit:
kenzanmedia@36c1379

@knewter

This comment has been minimized.

Copy link

knewter commented Aug 15, 2012

This never got merged? It's super useful to me fwiw, big +1 and thanks @jon077 :)

@rauchg

This comment has been minimized.

Copy link
Contributor

rauchg commented Aug 15, 2012

Aside from dot, spec, json, json-stream, nyan, browser and TAP, I think most reporters belong in userland. It should also be the case for this one.

@knewter

This comment has been minimized.

Copy link

knewter commented Aug 15, 2012

Alright. I just saw above where @visionmedia said a merge was forthcoming, so I assumed that was still the case. For a userland reporter like this, should it just be released as a standalone reporter npm module? Also, how does it plug in to mocha as an available reporter? Is this a built-in API? I'm new to mocha...but I love me some jenkins graphs :)

@rauchg

This comment has been minimized.

Copy link
Contributor

rauchg commented Aug 15, 2012

That's just my opinon, @visionmedia might still merge this

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Aug 15, 2012

yeah this should be userland and added to https://github.com/visionmedia/mocha/wiki

@tj tj closed this Aug 15, 2012

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Aug 15, 2012

via something like:

npm install junit-reporter
mocha --reporter junit-reporter
@knewter

This comment has been minimized.

Copy link

knewter commented Aug 15, 2012

surely that should be called junit-reporter-mocha or something? But yeah, I get it. I'll try to extract the reporter from this pull request for my personal use, but won't publish or anything because it's @jon077's to publish :)

@tj

This comment has been minimized.

Copy link
Contributor

tj commented Aug 15, 2012

doesn't have to have -mocha in the name, eventually (we have an issue open for this) we'd like to have all these reporters just consume the JSON stream, so then they're completely decoupled from mocha

@sjonnet19

This comment has been minimized.

Copy link

sjonnet19 commented Aug 15, 2012

@knewter

This comment has been minimized.

Copy link

knewter commented Aug 15, 2012

I honestly don't even know what I was thinking...xunit reporter works just fine for what I'm doing here, don't even see how xunit / junit output differs :-\ Jenkins sees no problem with it apparently.

@jon077

This comment has been minimized.

Copy link
Author

jon077 commented Aug 15, 2012

Userland seems best. I will publish after vacation.

--jon

On Wednesday, August 15, 2012 at 1:36 PM, Josh Adams wrote:

I honestly don't even know what I was thinking...xunit reporter works just fine for what I'm doing here, don't even see how xunit / junit output differs :-\ Jenkins sees no problem with it apparently.


Reply to this email directly or view it on GitHub (#349 (comment)).

@drefined

This comment has been minimized.

Copy link

drefined commented Nov 2, 2012

@jon077 back from vacation yet? :)

@fengmk2 fengmk2 referenced this pull request Feb 4, 2013

Closed

cobertura-xml #4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment