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

reporters should be immune to interference with STDOUT #3604

Closed
boneskull opened this issue Dec 10, 2018 · 9 comments
Closed

reporters should be immune to interference with STDOUT #3604

boneskull opened this issue Dec 10, 2018 · 9 comments
Labels
status: accepting prs Mocha can use your help with this one! type: bug a defect, confirmed by a maintainer

Comments

@boneskull
Copy link
Member

Potentially related: #2943

If a reporter uses console.log(), it should retain the function like we do with timers.

This will theoretically allow users to stub out console.log() in their own tests without affecting reporter output.

Since console.log() expects this to be console, it likely needs to take a form of:

const log = console.log.bind(console);

process.stdout.write() may need to be consumed in a similar manner.

To reproduce the problem:

// 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();
});

Another approach would be to instantiate our own Console object (docs) which would bypass the global console entirely. We could consider subclassing Console in order to provide a nice abstraction for reporters consuming process.stdout directly (or other streams).

A workaround (for the user) would be to use something like proxyquire, rewiremock, etc. in lieu of replacing the method outright, like one would with Sinon.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer status: accepting prs Mocha can use your help with this one! labels Dec 10, 2018
@boneskull
Copy link
Member Author

If possible, ensure this is enforced by ESLint, as we do with the timers.

@plroebuck
Copy link
Contributor

Or more simply, provide the ability to redirect to file.

@boneskull
Copy link
Member Author

I think you’re conflating two separate issues. Even if any reporter could write to a built-in or user-defined stream, that stream would need to be protected from test harness interference.

@craigtaub
Copy link
Contributor

craigtaub commented Dec 16, 2018

I like the idea of base utility/s to unify reporters and make them more resilient. Happy to have a go when I get time.

@craigtaub
Copy link
Contributor

craigtaub commented Dec 19, 2018

@boneskull with so many of our reporter tests relying on interfering with process.stdout.write we would have to redo most of them if we did this.
What do you think if we just move the reporters using console.log to use process.stdout.write instead? Very quick change and would add value.

Worth noting console.log calls process.stdout.write internally (with a new-line). And that process.stdout.write cant be stored + used as a reference like console.log can (as it relies on a stream) so not sure how we could do it.
If we tried to make everything use console.log (via reference) we have issues with output (as everything gets a new-line). Tricky.

@boneskull
Copy link
Member Author

@boneskull with so many of our reporter tests relying on interfering with process.stdout.write we would have to redo most of them if we did this.

Can you show how it'd be necessary to change them? I think we can just change the helper function createRunReporterFunction() to spy on calls to process.stdout.write() via a combination of Sinon and rewiremock.

process.stdout.write cant be stored + used as a reference like console.log can

Not sure I understand. console.log (in Node.js) is (likely) bound via Function.prototype.bind to the console object, which is why you can pass log around (this means we'd need to use our own Console instance) as a reference. There's no reason we couldn't do let write = process.stdout.write.bind(process.stdout) if needed.

@boneskull
Copy link
Member Author

I think it'd be helpful to see a proof of concept.

@craigtaub
Copy link
Contributor

I'll put something together

@craigtaub
Copy link
Contributor

Closing as PR was merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: accepting prs Mocha can use your help with this one! type: bug a defect, confirmed by a maintainer
Projects
None yet
Development

No branches or pull requests

3 participants