Base reporter store ref to console.log

[craigtaub]

Description of the Change

Small fix so that users can silence their own code (focused on just console.log's).

// yuck.spec.js
it('should do, but it do not', function() {
  // user wants to hide output generated by console.log calls in fn 'foo'
  console.log = function() {};
  foo();
});

We can still stub its underlying mechanism.
Decided this small fix offers enough for now. Let me know if others feel strongly that its not.

Alternate Designs

Thinking more about how to unify the reporters (and process.stdout.write).

Applicable Issues

#3604

[coveralls]

Coverage increased (+0.005%) to 92.4% when pulling 62569cc on craigtaub/addPrintLine into c903147 on master.

[plroebuck]

Mentioned elsewhere, but this test should fail after process reassignment...

[craigtaub]

Mentioned elsewhere

In this PR? Sorry man dont quite follow this. A change relating to this logging?

[craigtaub]

@plroebuck any chance you could help me with this please.

[plroebuck]

#2698 (comment)

Are you not experiencing failure with this specific test?

[craigtaub]

Ah I see.

node bin/mocha --reporter dot "test/reporters/xunit.spec.js"

  ․․․․․․․․․․․․․․

  14 passing (21ms)

Passes for me, same if isolate this test. I'll have a look if can find why.

[craigtaub]

Is your concern running function on a falsy value? Seems as its assigned back before the test finishes its there for end of root suite execution.

[boneskull]

thanks @craigtaub. insofar as the code, this can go in, but there are a few issues that should probably be addressed (yet nothing that should block it).

I think though, before we merge it, we need to be certain that this will be more beneficial than harmful.

the main benefit is that tests can stub their own use of console.log in the code under test without impacting reporter output. while the use-case is not unheard of, I suspect anyone dealing with this currently just lives with it.

others may want to stub console.log in such a way that the reporter output can be redirected to elsewhere (maybe a file, or even a socket). I imagine most people doing this sort of thing would take advantage of their shell's ability to pipe output. this may not always be available (windows?), so the only recourse would be to monkeypatch either console.log or swap a different stream in for process.stdout. these users would be better served using a custom reporter, regardless.

in summary, I think if we do this with console.log, we shouldn't cause much mayhem. but if we rescinded the ability for a user to monkeypatch process.stdout, then we may have moved too much cheese.

would welcome any further thoughts/discussion

[boneskull]

IMO the "more correct" thing to do is to modify the eslint configuration to handle console.* the same way it handles timers (unless that's not desirable, for some reason?)

don't think we need to block on that, though; we can change it later.

[craigtaub]

Had a quick look at this. The main issue being that outside the reporters we use console (log and error) a fair amount. The solution was always focused on fixing the issue with reporters. We could have a rule just for reporters (seems brittle), or we can overhaul this solution to follow the timers practice, but AFAIK there has been no indication we actually need this.

[boneskull]

the rules that exist for timers are also specific to certain files, so there’s a precedent for this anyway. again, not a blocker.

[craigtaub]

Ah ok i'll have a look at that.

[craigtaub]

Ive added the lint rule now.

[boneskull]

1. a sinon sandbox is typically easier to use, because no matter what you do, it's only one restore() call:

   var sandbox;
   var myStub;
   var myOtherStub;
   
   beforeEach(function() {
     sandbox = sinon.createSandbox();
     myStub = sandbox.stub();
     myOtherStub = sandbox.stub();
   })
   
   afterEach(function() {
     sandbox.restore();
   });

2. if you call restore() in the test itself after making assertions, the call won't happen if the assertions fail; use hooks instead

[plroebuck]

#2 Good point. Forgot about that.

  describe('when reporter output immune to user test changes', function() {
    var sandbox;
    var baseConsoleLog;

    beforeEach(function() {
      sandbox = sinon.createSandbox();
      sandbox.stub(console, 'log');
      baseConsoleLog = sandbox.stub(Base, 'consoleLog');
    });

    it('should let you stub out console.log without effecting reporters output', function() {
      Base.list([]);
      baseConsoleLog.restore();

      expect(Base.consoleLog, 'was called');
      expect(console.log, 'was not called');
    });

    afterEach(function() {
      sandbox.restore();
    });
  });

[craigtaub]

Nice with the sandboxes. But if I don't restore the Base.consoleLog before the test ends it breaks the reporter running the test.

[plroebuck]

Why?

[craigtaub]

The instance of Base used in the test is the same as that used in the reporter (in this instance spec) running the test.
So the beforeEach hook stubs Base.consoleLog, the EVENT_TEST_PASS event fires in the real reporter and fails to print the title with Base.consoleLog, then afterEach fires and restores.
Have found a fix by letting stub call though (obvious really lol).

[plroebuck]

Edited sandbox code above.

[boneskull]

as above, stubbing & un-stubbing should happen in hooks

[craigtaub]

Dont think this is going to be possible. Same reason above except this cant callThrough().

[plroebuck]

See below

[craigtaub]

Going to get this in shortly as don't think anything outstanding.

[plroebuck]

This is what I'm using...
Achieves equivalent test without messing with process object and produces no "noise" output...

    describe('when output directed to console', function() {
      it("should call 'console.log' with line", function() {
        var xunit = new XUnit(runner);

        var fakeThis = {preferConsole: true};
        Object.setPrototypeOf(fakeThis, XUnit.prototype);

        var consoleLog = sinon.stub(console, 'log');
        xunit.writeln.call(fakeThis, expectedLine);
        consoleLog.restore();

        expect(consoleLog.calledWith(expectedLine), 'to be true');
      });
    });

[plroebuck]

Of course the user could have used this same technique rendering this issue unnecessary.

[craigtaub]

But the logic inside xunit reporter requires process to have been tampered with doesnt it?
The assertion is on the logging but the test setup will have to mess with process wont it?

  if (this.fileStream) {
    // ... not this
  } else if (typeof process === 'object' && process.stdout) {
    // ... not this
  } else {
   // ... want to call this
  }

Happy to be corrected here if ive missed something.

[plroebuck]

You're right, of course.

I'm too far ahead of myself here; I adjusted that part of the code to make XUnit more easily configurable (and testable)...

[plroebuck]

But the scheme for stubbing console.log (assuming XUnit instance was thus configured) is demonstrated.

[plroebuck]

I think though, before we merge it, we need to be certain that this will be more beneficial than harmful.

the main benefit is that tests can stub their own use of console.log in the code under test without impacting reporter output. while the use-case is not unheard of, I suspect anyone dealing with this currently just lives with it.

While I won't block it, I don't think we should merge this (nothing to do with Craig's implementation). There are existing schemes for accomplishing the issue's goal, no changes required. Would prefer to simply add wiki page demonstrating how to correctly stub process.stdout.write and console.log...

[craigtaub]

Would prefer to simply add wiki page demonstrating how to correctly stub process.stdout.write and console.log...

That is fine for us to do, but I do feel most users who do this will initially try the technique in the description, so having immediate successful feedback seems like a benefit for us and users.

There are existing schemes for accomplishing the issue's goal, no changes required.

I have seen discussions but nothing concrete or PR'd yet. Let me know if thats not the case.
If there is something close to being complete i am happy to let that take over this. But I had not seen anything and thought a half-measure now is better than waiting indefinitely for a larger fix.

[plroebuck]

Would prefer to simply add wiki page demonstrating how to correctly stub process.stdout.write and console.log...

That is fine for us to do, but I do feel most users who do this will initially try the technique in the description, so having immediate successful feedback seems like a benefit for us and users.

Problem is that you likely gave them a false positive outcome though; very likely they've now screwed up their test setup if they don't reset correctly. Then their unrelated next test fails instead...

There are existing schemes for accomplishing the issue's goal, no changes required.

I have seen discussions but nothing concrete or PR'd yet. Let me know if that's not the case.
If there is something close to being complete i am happy to let that take over this. But I had not seen anything and thought a half-measure now is better than waiting indefinitely for a larger fix.

No. I meant "no changes to Mocha required" existing schemes, like this.

[craigtaub]

Problem is that you likely gave them a false positive outcome though; very likely they've now screwed up their test setup if they don't reset correctly

If they dont reset correctly yeah will mess up their results, but not much we can do there.. If they do reset correctly they will benefit from this, specially if they setup and reset in hooks, which AFAIK is the best practice for handling those.

I imagine (speaking from personal experience only) most users are likely to try short-hand and give up if doesnt work, with a subset looking up in our docs if there is a clean way to do it .

Would prefer to simply add wiki page demonstrating how to correctly stub process.stdout.write and console.log...

Yes we could definitely do with offering more help for handling this.

[craigtaub]

UPDATE: going to leave this here for now while we continue the work to unify the reporters (i.e. #3874)

[craigtaub]

As wider attempts to fix this are taking some time, I feel this small addition is worth adding for now as this issue still exists.

[craigtaub]

Have added a small section to the Wiki https://github.com/mochajs/mocha/wiki/HOW-TO:-Correctly-stub-stdout (feel free to make updates).