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

Add a feature to optionally direct the test output to a file. #897

Closed
wants to merge 3 commits into
base: master
from

Conversation

Projects
None yet
6 participants
@demmer
Contributor

demmer commented Jun 13, 2013

When using mocha from within a framework like grunt, it can be useful to direct the test output to a file. For example, when using the xunit reporter, then all of the output from various test suites could be collected and parsed after the fact.

The implementation creates a new output library and changes all of the test reporters to call output.log or output.write instead of directly calling console.log or process.stdout.write.

It also adds a command line option "-O " to enable the redirection.

Also added a test rule to the Makefile that lists all of the reporters and verifies that the output is properly generated when the option is enabled.

Michael Demmer added some commits Jun 11, 2013

Michael Demmer
Add a feature to optionally direct the test output to a file.
The implementation creates a new output library and changes all of the
test reporters to call output.log or output.write instead of directly
calling console.log or process.stdout.write.

Added a test rule to the Makefile that lists all of the reporters and
verifies that the output is properly generated when the option is
enabled.
Michael Demmer
Rework implementation to work on older versions of node.js.
The previous implementation used a non-standard console.Console object
that didn't work on older versions of node.js. Rework it to explicitly
check whether to call console.{log,error} directly or to use util.format
before calling into the file stream.
@julien51

This comment has been minimized.

Show comment
Hide comment
@julien51

julien51 Jun 17, 2013

This would be awesome, not just to redirect to files, but to any stream. For example, we're trying to stream the result of our test suite to a browser and now the only way it to spawn a child process, which is kind of a lot of work just to "hijack" the stdout!

julien51 commented Jun 17, 2013

This would be awesome, not just to redirect to files, but to any stream. For example, we're trying to stream the result of our test suite to a browser and now the only way it to spawn a child process, which is kind of a lot of work just to "hijack" the stdout!

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Jun 17, 2013

Contributor

why not just redirect stdio? seems like a lot of hacky-ish changes for little gain. @julien51 I wouldn't consider that bad practice at all personally

Contributor

tj commented Jun 17, 2013

why not just redirect stdio? seems like a lot of hacky-ish changes for little gain. @julien51 I wouldn't consider that bad practice at all personally

@julien51

This comment has been minimized.

Show comment
Hide comment
@julien51

julien51 Jun 17, 2013

But if we redirect stdio that will apply to all other stdout statement in our app, which has a lot more than mocha in it :( Ok, we'll try that then.

julien51 commented Jun 17, 2013

But if we redirect stdio that will apply to all other stdout statement in our app, which has a lot more than mocha in it :( Ok, we'll try that then.

@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer Jun 17, 2013

Contributor

I have that same issue as @julien51... In my case I'm using mocha from within grunt using grunt-simple-mocha.

I want the grunt output from this task (and other tasks) to go to stdout, but I want the test results in separate files so that my CI system can parse the results.

Contributor

demmer commented Jun 17, 2013

I have that same issue as @julien51... In my case I'm using mocha from within grunt using grunt-simple-mocha.

I want the grunt output from this task (and other tasks) to go to stdout, but I want the test results in separate files so that my CI system can parse the results.

@demmer demmer closed this Jun 17, 2013

@demmer demmer reopened this Jun 17, 2013

@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer Jun 17, 2013

Contributor

Oops -- sorry about that - I didn't intend to close the request.

Contributor

demmer commented Jun 17, 2013

Oops -- sorry about that - I didn't intend to close the request.

@jbowes

This comment has been minimized.

Show comment
Hide comment
@jbowes

jbowes Jun 26, 2013

Contributor

There are also cases where you can't control what's written to stdout by third party libraries that you include in your tests. Any random output mixed in with an xunit report will break parsers trying to read it.

This change would be immensely useful for feeding test results into CI systems.

Contributor

jbowes commented Jun 26, 2013

There are also cases where you can't control what's written to stdout by third party libraries that you include in your tests. Any random output mixed in with an xunit report will break parsers trying to read it.

This change would be immensely useful for feeding test results into CI systems.

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Jun 26, 2013

Contributor

in general I don't really think third-party libs should even write to stdio, that's generally bad practice, with some exceptions of course :D as for debugging with console.log()s you could always just write to stderr instead

Contributor

tj commented Jun 26, 2013

in general I don't really think third-party libs should even write to stdio, that's generally bad practice, with some exceptions of course :D as for debugging with console.log()s you could always just write to stderr instead

@julien51

This comment has been minimized.

Show comment
Hide comment
@julien51

julien51 Jun 26, 2013

Well, that's my point! How can I use mocha as a 3rd party libs in another
node project?

On Wed, Jun 26, 2013 at 7:19 PM, TJ Holowaychuk notifications@github.comwrote:

in general I don't really think third-party libs should even write to
stdio, that's generally bad practice, with some exceptions of course :D as
for debugging with console.log()s you could always just write to stderr
instead


Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/mocha/pull/897#issuecomment-20064677
.

julien51 commented Jun 26, 2013

Well, that's my point! How can I use mocha as a 3rd party libs in another
node project?

On Wed, Jun 26, 2013 at 7:19 PM, TJ Holowaychuk notifications@github.comwrote:

in general I don't really think third-party libs should even write to
stdio, that's generally bad practice, with some exceptions of course :D as
for debugging with console.log()s you could always just write to stderr
instead


Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/mocha/pull/897#issuecomment-20064677
.

@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer Jun 26, 2013

Contributor

I don't totally understand what you're saying here… can you elaborate a
little bit?

But moreover, given that I (and some others on this thread) would
definitely find this useful, can you explain why you think the changes are
"hacky-ish" and what the problem would be with merging it (even if you
personally don't find it compelling).

-m

On Wed, Jun 26, 2013 at 10:19 AM, TJ Holowaychuk
notifications@github.comwrote:

in general I don't really think third-party libs should even write to
stdio, that's generally bad practice, with some exceptions of course :D as
for debugging with console.log()s you could always just write to stderr
instead


Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/mocha/pull/897#issuecomment-20064677
.

Michael Demmer
Osprey Labs -- VP Engineering
www.osprey.io

Contributor

demmer commented Jun 26, 2013

I don't totally understand what you're saying here… can you elaborate a
little bit?

But moreover, given that I (and some others on this thread) would
definitely find this useful, can you explain why you think the changes are
"hacky-ish" and what the problem would be with merging it (even if you
personally don't find it compelling).

-m

On Wed, Jun 26, 2013 at 10:19 AM, TJ Holowaychuk
notifications@github.comwrote:

in general I don't really think third-party libs should even write to
stdio, that's generally bad practice, with some exceptions of course :D as
for debugging with console.log()s you could always just write to stderr
instead


Reply to this email directly or view it on GitHubhttps://github.com/visionmedia/mocha/pull/897#issuecomment-20064677
.

Michael Demmer
Osprey Labs -- VP Engineering
www.osprey.io

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Jul 1, 2013

Contributor

if we do something similar I'd definitely prefer to just pass an arbitrary output Stream

Contributor

tj commented Jul 1, 2013

if we do something similar I'd definitely prefer to just pass an arbitrary output Stream

@tj

This comment has been minimized.

Show comment
Hide comment
@tj

tj Jul 1, 2013

Contributor

@julien51 you'd want to run mocha in a sub process for isolation anyway, so reading from stdio there is pretty natural IMO, no reason to dump it to a file (you could just redirect anyway)

Contributor

tj commented Jul 1, 2013

@julien51 you'd want to run mocha in a sub process for isolation anyway, so reading from stdio there is pretty natural IMO, no reason to dump it to a file (you could just redirect anyway)

@jbowes

This comment has been minimized.

Show comment
Hide comment
@jbowes

jbowes Jul 1, 2013

Contributor

if we do something similar I'd definitely prefer to just pass an arbitrary output Stream

+1 to this. with the patch series in this pull request, it was pretty difficult to connect up a reporter that was outside of the source tree: goinstant/mocha-cobertura-reporter@aaf01a2

Using a stream, ideally passed in on reporter instantiation, would make this much easier.

Contributor

jbowes commented Jul 1, 2013

if we do something similar I'd definitely prefer to just pass an arbitrary output Stream

+1 to this. with the patch series in this pull request, it was pretty difficult to connect up a reporter that was outside of the source tree: goinstant/mocha-cobertura-reporter@aaf01a2

Using a stream, ideally passed in on reporter instantiation, would make this much easier.

@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer Jul 1, 2013

Contributor

I originally started down this road where each reporter would get to the output stream through the Base class. The problem is that a number of reporters emit output from callbacks. Obviously we could bind 'self' before calling the output but it seemed that using a module would be simpler than forcing callers to go through the reporter instance.

However, as @jbowes requested it would be trivial to also pass the stream to the reporter. I will work on a patch to change it to do this as well.

Contributor

demmer commented Jul 1, 2013

I originally started down this road where each reporter would get to the output stream through the Base class. The problem is that a number of reporters emit output from callbacks. Obviously we could bind 'self' before calling the output but it seemed that using a module would be simpler than forcing callers to go through the reporter instance.

However, as @jbowes requested it would be trivial to also pass the stream to the reporter. I will work on a patch to change it to do this as well.

Michael Demmer
Tweak file output implementation based on pull request comments.
Pass the output stream to each of the reporters in the constructor to make
integration simpler for reporters defined outside of the mocha source tree and
to simplify the changes.

Also add support to pass an arbitrary stream to output.initialize and rename
the 'stream' variable to 'ostream' so it doesn't clash with the stream module.
@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer Jul 2, 2013

Contributor

Updated the branch as requested.

Of course, by changing the contract of the reporter constructor, any reporters defined outside of the source tree will need to be modified accordingly, but then they will have simpler access to the output stream.

Contributor

demmer commented Jul 2, 2013

Updated the branch as requested.

Of course, by changing the contract of the reporter constructor, any reporters defined outside of the source tree will need to be modified accordingly, but then they will have simpler access to the output stream.

@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer Jul 12, 2013

Contributor

@visionmedia Do you have any reaction to the latest change?

Contributor

demmer commented Jul 12, 2013

@visionmedia Do you have any reaction to the latest change?

@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer Aug 1, 2013

Contributor

@visionmedia Any thoughts? As @jbowes and @julien51 pointed out, it's not that convenient in all cases to guarantee that mocha's output can be isolated from either the place where it's invoked (grunt-mocha for example) or from output from the test spec itself. Of course any kind of integration with a CI system needs a way to isolate the runner output from everything else, hence it seemed most natural to me to make it just a feature of mocha itself.

Are you opposed to this idea on principal or is it just that you're not a fan of the implementation?

If it's the latter, from my standpoint the options are:

  1. My original proposal where the output stream is abstracted in a module. All reporters need to change to s/console.log/output.log/ and s/process.stdout.write/output.write/.
  2. Open a simple output stream and pass it to the reporter constructor. Reporters would then need to change to (a) not use console.log, instead explicitly format output and write to the stream, plus (b) stash the output stream so it can be referenced from callbacks etc. It sounds like this is what you would prefer?
  3. Some hybrid of the two - this is essentially what my last patch does, but I agree it ended up a bit hackish.

If you prefer option 2 (or some other approach), I'm happy to rework the patch to do it that way.

Contributor

demmer commented Aug 1, 2013

@visionmedia Any thoughts? As @jbowes and @julien51 pointed out, it's not that convenient in all cases to guarantee that mocha's output can be isolated from either the place where it's invoked (grunt-mocha for example) or from output from the test spec itself. Of course any kind of integration with a CI system needs a way to isolate the runner output from everything else, hence it seemed most natural to me to make it just a feature of mocha itself.

Are you opposed to this idea on principal or is it just that you're not a fan of the implementation?

If it's the latter, from my standpoint the options are:

  1. My original proposal where the output stream is abstracted in a module. All reporters need to change to s/console.log/output.log/ and s/process.stdout.write/output.write/.
  2. Open a simple output stream and pass it to the reporter constructor. Reporters would then need to change to (a) not use console.log, instead explicitly format output and write to the stream, plus (b) stash the output stream so it can be referenced from callbacks etc. It sounds like this is what you would prefer?
  3. Some hybrid of the two - this is essentially what my last patch does, but I agree it ended up a bit hackish.

If you prefer option 2 (or some other approach), I'm happy to rework the patch to do it that way.

@park9140

This comment has been minimized.

Show comment
Hide comment
@park9140

park9140 Aug 1, 2013

Contributor

Shouldn't we just allow options to be passed for the reporters and then individually support alternative output types on the reporters?

Contributor

park9140 commented Aug 1, 2013

Shouldn't we just allow options to be passed for the reporters and then individually support alternative output types on the reporters?

@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer Aug 2, 2013

Contributor

That would work for me...

The basic reporter options would be something like: jut-io@2c41b74

There also needs to be a new hook to give the reporter a chance to asynchronously flush the stream: jut-io@06be954

Then the change to the XUnit reporter: jut-io@0b2d74e

@visionmedia If you prefer this approach I'll close this request and open a new one.

Contributor

demmer commented Aug 2, 2013

That would work for me...

The basic reporter options would be something like: jut-io@2c41b74

There also needs to be a new hook to give the reporter a chance to asynchronously flush the stream: jut-io@06be954

Then the change to the XUnit reporter: jut-io@0b2d74e

@visionmedia If you prefer this approach I'll close this request and open a new one.

@matope

This comment has been minimized.

Show comment
Hide comment
@matope

matope Oct 31, 2013

I hope that this PR would be merged.

I can fix our a few codes to suppress stdout as you say easily.
But I think that, if Mocha has a "format" option, to output a valid report (regardless of the test-case) would be a responsibility.

matope commented Oct 31, 2013

I hope that this PR would be merged.

I can fix our a few codes to suppress stdout as you say easily.
But I think that, if Mocha has a "format" option, to output a valid report (regardless of the test-case) would be a responsibility.

@demmer

This comment has been minimized.

Show comment
Hide comment
@demmer

demmer May 17, 2014

Contributor

Switched to a simpler approach in #1218 .

Contributor

demmer commented May 17, 2014

Switched to a simpler approach 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