get the default process.stdout.write back after tests are finished

[stevemao]

I have a test which tests stdout so I need to change the default behaviour of process.stdout.

process.stdout.write = (function(write) {
    return function(string, encoding, fd) {
      var args = _.toArray(arguments);
      callback.call(callback, string);
    };
  }(process.stdout.write));

mocha's report, however, cannot be seen any more if I use this. It would be good if mocha remembers the default process.stdout.write at the beginning and change it back after tests are finished.

[estilles]

@stevemao, I had a similar issue with tests that require the changing the behavior of console.log(). IMO, I don't think the burden of restoring mocks/stubs/spies should fall on either Mocha or its reporters.

My solution was to create a "helper" function that saves the original console.log(), runs the test (which I pass as a function), and restores it upon completion. The following is a short, but functional, version of my solution. I omitted validation and other sanity checks for the sake of brevity.

module.exports = function helper(fn) {
  var originalConsoleLog = console.log;

  function restore() {
    console.log = originalConsoleLog;
  }

  if (fn.length) {
    fn(restore);
  else {
    fn();
    restore();
  }
}

Then ... in my tests I would ...

var helper = require('/path/to/helper');
...
describe('...', function() {
  // sync example
  it('should ...', function() {
    helper(function() {
      var logs[];
      console.log = function() { logs.push(arguments) }; // <-- or whatever you want to do
      assert(...);
      ...
    });
  });

  // async example
  it('should ...', function(done) {
    helper(function(restore) {
      var logs[];
      console.log = function() { logs.push(arguments) }; // <-- or whatever you want to do
      assert(...);

      asyncStuff(function() {
        assert(...);
        restore();
        done();
      });
    });
  });

});

This is just a rudimentary example, but I think you get the picture. You can easily adapt this to fit your particular use case.

[danielstjules]

I do roughly the same thing as @JohnnyEstilles for any CLI-related tools: https://github.com/danielstjules/jsinspect/blob/master/spec/helpers.js https://github.com/danielstjules/jsinspect/blob/master/spec/reporters/jsonSpec.js

Since I use node a lot for dev tools, I definitely have some bias towards saving a reference to process.stdout.write before tests, though @JohnnyEstilles is probably right in that it might be outside the scope of a test runner. However, it sure would be convenient! :)

[danielstjules]

The only reason I'd maybe lean towards having this behavior supported by Mocha is that you have to do cleanup from within a test, and can't rely on any hooks. So if your test errors out, you're not going to be able to restore process.stdout.write before mocha tries to print a line for the test case. It'll simply result in a missing line from the output, assuming your afterEach logic restores it.

[estilles]

@danielstjules, I do see your point. And, yes it would be very convenient (I do a lot of console.log/ process.stdout.write checks in my tests). So, is this something you guys would consider implementing? I'd be more than happy to work on it. :-)

[danielstjules]

That would be super helpful! I should probably ping someone else here, cause like I said, I'm a bit biased :)

cc @boneskull @dasilvacontin

[stevemao]

Each test should be sandboxed in mocha and users should be able to do whatever dodge things inside each test. Users can change process.stdout.write but they should still be able to see the report (although the report writes to process.stdout but this shouldn't concern users).

[danielstjules]

Alright, I'm convinced this is a good idea. :) @JohnnyEstilles If you'd like to submit a PR, feel free! Otherwise I might work on this tomorrow.

[estilles]

@danielstjules ... Already started working on one. Will submit it soon. :-)

[estilles]

@danielstjules, I went ahead and submitted my PR. It includes a module that I wrote a while back and am actively using in other projects, which happens to be a perfect fit for the issue @stevemao brought up. I included tests, but no docs (couldn't really find a place in the docs or the wiki where this could/should be documented). If you require docs for every PR just point me in the right direction and I'll write something up. :-)

[estilles]

@stevemao / @danielstjules, sorry ... I closed my PR. @a8m and @travisjeffery seem to think this is not a good idea.

@stevemao, it seems like your only recourse is to cleanup after yourself within each affected test, the same way I described in my initial comment.

[danielstjules]

This is in reference to #1601 (comment) & #1601 (comment) Figured it'd be better posted in this issue since the PR was closed.

@a8m @travisjeffery While I agree that proper dependency injection would go a long way, I feel like Mocha should at least maintain its own state. A spec shouldn't be able to break reporter output so easily. For example:

var assert = require('assert');

describe('example', function() {
  var output, write;

  write = process.stdout.write;

  beforeEach(function() {
    output = '';
    process.stdout.write = function(str) {
      output += str;
    };
  });

  afterEach(function() {
    process.stdout.write = write;
  });

  it('I feel like this is acceptable', function() {
    console.log('testing');
    assert.equal(output, 'testing\n');
  });
});

$ mocha example.js



  1 passing (4ms)

Note that there's no output because we modified stdout.write. Along the lines of promoting DI, an alternative to the PR @JohnnyEstilles submitted would be to apply it to Mocha's reporters. We could pass process.stdout.write into the reporters, caching the function, rather than invoking it directly (as seen in https://github.com/mochajs/mocha/blob/master/lib/reporters/dot.js)

[danielstjules]

@JohnnyEstilles Sorry about that. I'm hoping that something smaller in scope might work a bit better.

[estilles]

@danielstjules ... no worries. I understand not everyone will agree with my madness. :-)

[danielstjules]

@a8m Trying to keep the discussion in this one issue, I think @stevemao makes a valid argument: #1601 (comment) I think it's fine for us to recommend that user libraries make proper use of DI, but that same logic should apply to us, especially when it means a better user experience. I'd rather not make people jump through hoops to test a CLI tool.

[a8m]

but that same logic should apply to us, especially when it means a better user experience.

absolutely agree. and it's solved this issue as well. @danielstjules

But this PR #1601 still feel me like a "patch" and cover up of things we don't need to.
Don't get me wrong @JohnnyEstilles, I really appreciate your work, but we see things differently.

[estilles]

@a8m ... understood. That's why I withdrew my PR.

[danielstjules]

absolutely agree. and it's solved this issue as well. @danielstjules

I'm a little confused - what do you mean?

[a8m]

Hmm I think we did not understand each other.

[a8m]

I talked about this

We could pass process.stdout.write into the reporters, caching the function, rather than invoking it directly

[danielstjules]

Oh, gotcha! So we're in agreement that mocha should cache process.stdout.write for its own purposes? :) I think that's the easiest solution to this, without having to manage additional globals. Keeps things small in scope, as well.

[estilles]

The way I see it, Mocha already does something similar to #1601, just on a smaller scale. It already saves a handful of functions from the global object to "avoid Sinon interfering". I supposed you could do the same with process.stdout.write (which is the issue now being discussed). While we're at it, we should go ahead and do the same with console.log() and any other function from the global object a reporter (whether internal or third-party) might use.

Short of my "patch and cover" solution, the only REAL solution to this would be to "actually" sandbox the testing environment.

[dasilvacontin]

Yep, I agree with this, sorry for not seeing the issue before, too much work.

[boneskull]

"Saving" various functions that may or may not be modified by the test environment seems like an unscalable dead-end to me; please don't add more crap to the list.

What are the implications of sandboxing the test environment? What kind of effort is required? Is this even feasible? Does it work in the browser?

[boneskull]

Is it feasible to grab the entire global object and save that, then avoid using globals in Mocha altogether?

[stevemao]

I don't think saving and restoring the entire global is a good idea. However, restoring globals that mocha is using is necessary. Currently the problem is the report might not visible if I change stdout

#1601 (comment)

I could restore stdout but tests might fail or user might forget to do so, etc as others have pointed out here.

[stevemao]

I'm not sure what global values that might change the mocha behaviours (report or whatever). So maybe we can just focus on stdout here.

[boneskull]

So we save setTimeout, clearTimeout, probably others, now process.stdout--what else do we need? My point is that we should find a solution which doesn't require more lines of code anytime some 3p lib decides it wants to obliterate a global.

[stevemao]

I've made my point quite clear here and most collaborators seem to be positive with this.

[estilles]

Is it feasible to grab the entire global object and save that, then avoid using globals in Mocha altogether?

This is pretty much what I was trying to do in #1601. (I know the consensus is that it's overkill.)

So we save setTimeout, clearTimeout, probably others, now process.stdout--what else do we need?

I constantly have the same issue with console.log that @stevemao reported with process.stdout, so If you're making a list, please at it as well.

[boneskull]

Let's make sure we're all on the same page here:

It's not the test framework's responsibility to clean up after your tests.

Does anyone disagree with this statement?

If your test messes with globals, a reasonable expectation is that you will clean up after yourself. This is not impossible in the case of process.stdout.write() or console.log(). In fact, it's pretty easy.

var expect = require('chai').expect;

describe('my nice test', function() {
  var write, log, output = '';

  // restore process.stdout.write() and console.log() to their previous glory
  var cleanup = function() {
    process.stdout.write = write;
    console.log = log;
  };

  beforeEach(function() {
    // store these functions to restore later because we are messing with them
    write = process.stdout.write;
    log = console.log;

    // our stub will concatenate any output to a string
    process.stdout.write = console.log = function(s) {
      output += s;
    };
  });

  // restore after each test
  afterEach(cleanup);

  it('should suppress all output if a non-AssertionError was thrown', function() {
    process.stdout.write('foo');
    console.log('bar');
    // uncomment below line to suppress output, which is bad
    // expect(output).to.equal(foobar);
    expect(output).to.equal('foobar');
  });

  it('should not suppress any output', function() {
    try {
      process.stdout.write('foo');
      console.log('bar');
      // uncomment below line to just throw an AssertionError
      // expect(output).to.equal('barfoo');
      expect(output).to.equal(foobar); // ReferenceError
    } catch (e) {
      cleanup();
      throw e;
    }
  });
});

So, I'm opposed to any sort of automatic restoration of console.log() or process.stdout.write() or whatever, because:

1. Problems like this can be avoided within the tests themselves
2. "Saving" more globals does not scale and increases maintenance burden, since there are likely many globals which Mocha uses which a test could potentially stub
3. Other users may not expect Mocha to have magical powers of healing

However, I'm not opposed to at least investigating a sandbox-type solution, as that might be a good idea anyway, if the level of effort is reasonable.

If other collaborators disagree with me, please pipe up--otherwise I'd like to close this issue.

[travisjeffery]

yeah closing, adding this will lead to more pain (like getting support from people asking what magic is doing something) and needing more configuration (from people who want it turned off), etc.

[stevemao]

@danielstjules , @a8m, @dasilvacontin ?

[stevemao]

@boneskull I get your point here and I was/am actually doing this in my tests. But I'm not 100% happy with the syntax, especially the try...catch block. I have to write the block in all my tests and it just doesn't look right. Making things DRY and simpler is definitely the soul of node modules.

[dasilvacontin]

@stevemao Why the try-catch? You can use afterEach, right?

[danielstjules]

@stevemao what? Why the try-catch? You can use afterEach, right?

@dasilvacontin nope, since Mocha's reporter will try to print before the afterEach hook is invoked, and stdout.write/console.log are restored. So if the test failed, mocha wouldn't output anything for that spec.

[dasilvacontin]

True. Wow, that sucks. Is there a way to listen to the mocha instance events? It's quite possible there is some event emitted before trying to log into the console, like fail.

If as a user you could listen to that event, you could avoid cleanup code from polluting the its.

[travisjeffery]

@danielstjules hmm that case does suck

[travisjeffery]

but that is definitely an edge case too

[stevemao]

Guys, if you think this is too much overhead for mocha, we should at least make it possible to build a plugin module to restore process.stdout before printing out the report. So 👍 for emitting an event. (just to extend this, we might want to emit any kind of mocha event just to be future proof)

@travisjeffery if you wanna test output from a program this is pretty common IMO.

[boneskull]

@stevemao there's a ticket for a plugin api. i have some code written but not much

[stevemao]

So there is a way to listen to the mocha instance events and we don't need the try-catch block here?

[stevemao]

Guys, originally I thought this was a bug (probably low priority) and wanted to have it fixed in this main module. I'm now convinced by @boneskull this is not a bug. However I still think this is a problem that needs to be solved in the future by a writing a document, plugin, addon or whatever. I suggest to reopen this but mark it as a lowest priority (I don't think this is too much overhead to solve anyways).

[stevemao]

I added this to the wiki

https://github.com/mochajs/mocha/wiki/Mess-with-globals

[raine]

I didn't read through the whole thread but what I do instead of overriding process.stdout in tests is pass the environment (e.g. process) as an argument to my code. That way in tests it's easy to pass in custom streams or argv lists.

[test-as-other-user]

@boneskull your example fails to provide a solution for async tests. In this case, it is mocha's responsibility to add a callback to the timeout function to allow developers to cleanup after themselves. The following would be required:

it("should ...", function(done){
  this.timeout(400, () => cleanupBeforeMochaDoesItsPrintingAndBeforeAfterEachIsCalled())
  // mess with globals, if your test hangs for any reason (done() isn't called)
  // the next test is guaranteed to be in a clean state
})

Without this fix, any test which does not call done() (i.e. a failing test) and which messes with process.stdout would basically nuke the rest of mocha's output for that test. Using beforeEach or afterEach to run the cleanup job will not fix process.stdout in time for that test's mocha output; only the subsequent tests. So, yes, it would be nice to run my cleanup jobs, but AFAIK mocha doesn't let me do the job. I'd like to take responsibility for it, if I could.

Currently, the only thing that can be done is:

it("should ...", function(done){
  this.timeout(400)
  // mess with globals
  // this is hardly a good solution
  this.setTimeout(() => cleanupAndFailCurrentTest(), 400-epsilon)
})

I don't see a simpler way to solve this problem than to create a timeout callback (which I'm surprised doesn't exist right now). This would have to run before mocha tries to output result for the current test. Mocha is bleeding into my test scope, if I can't reasonably cleanup my test.

[boneskull]

It's not Mocha's responsibility to add and maintain some random API on demand. Mocha does not provide this level of isolation. If you need that, I'd suggest consuming vm or child_process or another testing framework