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

reporter-specific options and support for the xunit reporter to output to a file #1106

Closed
wants to merge 5 commits into from

Conversation

demmer
Copy link
Contributor

@demmer demmer commented Jan 9, 2014

This is a less pervasive change to accomplish the same goal as #897 -- i.e. to have a way of separating the xunit reporter output from stdout so that it can be used in a CI system.

In this approach I changed the mocha core to pass a set of reporter-specific options to the reporter class, as well as to call the reporter's done() method to give it a chance to clean things up.

Then the xunit reporter supports an output option to specify a filename, and flushes the file in done().

Previously I implemented a no-op done() method in the Base reporter class.
This is a slight simplification to make the caller responsible for checking
whether the done() method exists and only call it if it does.

The main benefit is that if a reporter class is used that doesn't actually
inherit from Base but simply implements the expected API then it will still
work even after merging the change.
@rmg
Copy link

rmg commented Jan 27, 2014

This is a small but incredibly useful change. Are there any remaining concerns preventing it from being merged?

I've been following it through a couple incarnations and have been using it for @strongloop's internal CI for a while.

@demmer
Copy link
Contributor Author

demmer commented Feb 6, 2014

I just noticed #1102 and for obvious reasons the two would conflict as written now. I didn't really consider that anyone would want to use xunit from within the browser, so my patch will obviously only work on node,js.

If this is something mocha needs to support, then it would be trivial to incorporate the meat of #1102 into this PR. But before putting any more time into this, I'd like to see some indication as to what its fate will be.

Are there any concerns that are blocking you from merging it?

@demmer
Copy link
Contributor Author

demmer commented Feb 26, 2014

@travisjeffery or @visionmedia: Any comments on the fate of this or #897? I'd really like to be able to get us off of our private fork. This would also seem to address the gist of #469 .

@butterdave
Copy link

+1 Big Time - would love this feature

@rothshahar
Copy link

+1 for the feature. I understand that ideally nothing other than the reporter will write to stdout. The reality is that as the owner of the test you don't always have control over all the code that gets executed and definitely can't control if it writes to stdout.

@demmer
Copy link
Contributor Author

demmer commented May 14, 2014

@travisjeffery / @visionmedia I'm not trying to be a nag, but I have no idea if your silence on this PR is a silent indication that you don't believe it should be merged or if you're just waiting to get around to it. As you can see above, I'm not the only one who would want this feature...

Since the PR was written there have been other changes that conflict. I'd be happy to go through and resolve them if I get the ok from you that you'll accept the change.

Can you please comment one way or the other. Thanks in advance.

@travisjeffery
Copy link
Contributor

made some comments, i'm not gonna merge anything for xunit that doesn't work in the browser too

@demmer
Copy link
Contributor Author

demmer commented May 17, 2014

Rebased onto master and reworked to address comments in #1218.

@demmer demmer closed this May 17, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants