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.

Sign up for GitHub

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

Jump to bottom

Base reporter store ref to console.log #3725

Merged
merged 13 commits into from Jun 19, 2019
Merged

Base reporter store ref to console.log #3725

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5f64bc2
add console.log ref
craigtaub Feb 11, 2019
fff01ff
inherit println
craigtaub Feb 13, 2019
76e220d
unit test change
craigtaub Feb 13, 2019
3778c5c
use static property and fix test
craigtaub Feb 16, 2019
36022ed
fix base test
craigtaub Feb 16, 2019
47e8dc4
rename consoleLog
craigtaub Mar 2, 2019
fb3e8bf
test include Base.consoleLog check
craigtaub Mar 17, 2019
3bb6d62
linter rule so reporters dont console.log
craigtaub Apr 6, 2019
5376b80
use hooks for stubs with base test
craigtaub Apr 10, 2019
19cc3f5
restore stub dont callThrough
craigtaub Apr 13, 2019
97b3a9d
fix test + rebase
craigtaub Jun 17, 2019
bc82689
fix faulty rebase. Removed printLn
craigtaub Jun 17, 2019
62569cc
remove superfluous base
craigtaub Jun 17, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
9 changes: 9 additions & 0 deletions .eslintrc.yml
View file
Open in desktop
Expand Up @@ -79,3 +79,12 @@ overrides:
parserOptions:
ecmaVersion: 6
sourceType: module

- files:
- lib/reporters/*.js
rules:
no-restricted-syntax:
- error
# disallow Reporters using `console.log()`
- selector: 'CallExpression[callee.object.name=console][callee.property.name=log]'
message: &GH-3604 See https://github.com/mochajs/mocha/issues/3604
23 changes: 15 additions & 8 deletions lib/reporters/base.js
View file
Open in desktop
Expand Up @@ -27,6 +27,11 @@ exports = module.exports = Base;

var isatty = tty.isatty(1) && tty.isatty(2);

/**
* Save log references to avoid tests interfering (see GH-3604).
*/
var consoleLog = console.log;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@craigtaub craigtaub Apr 3, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah ok i'll have a look at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ive added the lint rule now.


/**
* Enable coloring by default, except in the browser interface.
*/
Expand Down Expand Up @@ -192,7 +197,7 @@ var generateDiff = (exports.generateDiff = function(actual, expected) {
* Error property
*/
exports.list = function(failures) {
console.log();
Base.consoleLog();
failures.forEach(function(test, i) {
// format
var fmt =
Expand Down Expand Up @@ -253,7 +258,7 @@ exports.list = function(failures) {
testTitle += str;
});

console.log(fmt, i + 1, testTitle, msg, stack);
Base.consoleLog(fmt, i + 1, testTitle, msg, stack);
});
};

Expand Down Expand Up @@ -308,34 +313,34 @@ Base.prototype.epilogue = function() {
var stats = this.stats;
var fmt;

console.log();
Base.consoleLog();

// passes
fmt =
color('bright pass', ' ') +
color('green', ' %d passing') +
color('light', ' (%s)');

console.log(fmt, stats.passes || 0, milliseconds(stats.duration));
Base.consoleLog(fmt, stats.passes || 0, milliseconds(stats.duration));

// pending
if (stats.pending) {
fmt = color('pending', ' ') + color('pending', ' %d pending');

console.log(fmt, stats.pending);
Base.consoleLog(fmt, stats.pending);
}

// failures
if (stats.failures) {
fmt = color('fail', ' %d failing');

console.log(fmt, stats.failures);
Base.consoleLog(fmt, stats.failures);

Base.list(this.failures);
console.log();
Base.consoleLog();
}

console.log();
Base.consoleLog();
};

/**
Expand Down Expand Up @@ -488,4 +493,6 @@ function sameType(a, b) {
return objToString.call(a) === objToString.call(b);
}

Base.consoleLog = consoleLog;

Base.abstract = true;
24 changes: 14 additions & 10 deletions lib/reporters/doc.js
View file
Open in desktop
Expand Up @@ -44,41 +44,45 @@ function Doc(runner, options) {
return;
}
++indents;
console.log('%s<section class="suite">', indent());
Base.consoleLog('%s<section class="suite">', indent());
++indents;
console.log('%s<h1>%s</h1>', indent(), utils.escape(suite.title));
console.log('%s<dl>', indent());
Base.consoleLog('%s<h1>%s</h1>', indent(), utils.escape(suite.title));
Base.consoleLog('%s<dl>', indent());
});

runner.on(EVENT_SUITE_END, function(suite) {
if (suite.root) {
return;
}
console.log('%s</dl>', indent());
Base.consoleLog('%s</dl>', indent());
--indents;
console.log('%s</section>', indent());
Base.consoleLog('%s</section>', indent());
--indents;
});

runner.on(EVENT_TEST_PASS, function(test) {
console.log('%s <dt>%s</dt>', indent(), utils.escape(test.title));
Base.consoleLog('%s <dt>%s</dt>', indent(), utils.escape(test.title));
var code = utils.escape(utils.clean(test.body));
console.log('%s <dd><pre><code>%s</code></pre></dd>', indent(), code);
Base.consoleLog('%s <dd><pre><code>%s</code></pre></dd>', indent(), code);
});

runner.on(EVENT_TEST_FAIL, function(test, err) {
console.log(
Base.consoleLog(
'%s <dt class="error">%s</dt>',
indent(),
utils.escape(test.title)
);
var code = utils.escape(utils.clean(test.body));
console.log(
Base.consoleLog(
'%s <dd class="error"><pre><code>%s</code></pre></dd>',
indent(),
code
);
console.log('%s <dd class="error">%s</dd>', indent(), utils.escape(err));
Base.consoleLog(
'%s <dd class="error">%s</dd>',
indent(),
utils.escape(err)
);
});
}

Expand Down
2 changes: 1 addition & 1 deletion lib/reporters/dot.js
View file
Open in desktop
Expand Up @@ -68,7 +68,7 @@ function Dot(runner, options) {
});

runner.once(EVENT_RUN_END, function() {
console.log();
process.stdout.write('\n');
boneskull marked this conversation as resolved.
Show resolved Hide resolved
self.epilogue();
});
}
Expand Down
2 changes: 1 addition & 1 deletion lib/reporters/landing.js
View file
Open in desktop
Expand Up @@ -95,7 +95,7 @@ function Landing(runner, options) {

runner.once(EVENT_RUN_END, function() {
cursor.show();
console.log();
process.stdout.write('\n');
self.epilogue();
});
}
Expand Down
8 changes: 4 additions & 4 deletions lib/reporters/list.js
View file
Open in desktop
Expand Up @@ -41,7 +41,7 @@ function List(runner, options) {
var n = 0;

runner.on(EVENT_RUN_BEGIN, function() {
console.log();
Base.consoleLog();
});

runner.on(EVENT_TEST_BEGIN, function(test) {
Expand All @@ -50,7 +50,7 @@ function List(runner, options) {

runner.on(EVENT_TEST_PENDING, function(test) {
var fmt = color('checkmark', ' -') + color('pending', ' %s');
console.log(fmt, test.fullTitle());
Base.consoleLog(fmt, test.fullTitle());
});

runner.on(EVENT_TEST_PASS, function(test) {
Expand All @@ -59,12 +59,12 @@ function List(runner, options) {
color('pass', ' %s: ') +
color(test.speed, '%dms');
cursor.CR();
console.log(fmt, test.fullTitle(), test.duration);
Base.consoleLog(fmt, test.fullTitle(), test.duration);
});

runner.on(EVENT_TEST_FAIL, function(test) {
cursor.CR();
console.log(color('fail', ' %d) %s'), ++n, test.fullTitle());
Base.consoleLog(color('fail', ' %d) %s'), ++n, test.fullTitle());
});

runner.once(EVENT_RUN_END, self.epilogue.bind(self));
Expand Down
4 changes: 2 additions & 2 deletions lib/reporters/progress.js
View file
Open in desktop
Expand Up @@ -58,7 +58,7 @@ function Progress(runner, options) {

// tests started
runner.on(EVENT_RUN_BEGIN, function() {
console.log();
process.stdout.write('\n');
cursor.hide();
});

Expand Down Expand Up @@ -91,7 +91,7 @@ function Progress(runner, options) {
// and the failures if any
runner.once(EVENT_RUN_END, function() {
cursor.show();
console.log();
process.stdout.write('\n');
self.epilogue();
});
}
Expand Down
14 changes: 7 additions & 7 deletions lib/reporters/spec.js
View file
Open in desktop
Expand Up @@ -46,24 +46,24 @@ function Spec(runner, options) {
}

runner.on(EVENT_RUN_BEGIN, function() {
console.log();
Base.consoleLog();
});

runner.on(EVENT_SUITE_BEGIN, function(suite) {
++indents;
console.log(color('suite', '%s%s'), indent(), suite.title);
Base.consoleLog(color('suite', '%s%s'), indent(), suite.title);
});

runner.on(EVENT_SUITE_END, function() {
--indents;
if (indents === 1) {
console.log();
Base.consoleLog();
}
});

runner.on(EVENT_TEST_PENDING, function(test) {
var fmt = indent() + color('pending', ' - %s');
console.log(fmt, test.title);
Base.consoleLog(fmt, test.title);
});

runner.on(EVENT_TEST_PASS, function(test) {
Expand All @@ -73,19 +73,19 @@ function Spec(runner, options) {
indent() +
color('checkmark', ' ' + Base.symbols.ok) +
color('pass', ' %s');
console.log(fmt, test.title);
Base.consoleLog(fmt, test.title);
} else {
fmt =
indent() +
color('checkmark', ' ' + Base.symbols.ok) +
color('pass', ' %s') +
color(test.speed, ' (%dms)');
console.log(fmt, test.title, test.duration);
Base.consoleLog(fmt, test.title, test.duration);
}
});

runner.on(EVENT_TEST_FAIL, function(test) {
console.log(indent() + color('fail', ' %d) %s'), ++n, test.title);
Base.consoleLog(indent() + color('fail', ' %d) %s'), ++n, test.title);
});

runner.once(EVENT_RUN_END, self.epilogue.bind(self));
Expand Down
2 changes: 1 addition & 1 deletion lib/reporters/xunit.js
View file
Open in desktop
Expand Up @@ -142,7 +142,7 @@ XUnit.prototype.write = function(line) {
} else if (typeof process === 'object' && process.stdout) {
process.stdout.write(line + '\n');
} else {
console.log(line);
Base.consoleLog(line);
}
};

Expand Down
24 changes: 23 additions & 1 deletion test/reporters/base.spec.js
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@ var chai = require('chai');
var sinon = require('sinon');
var helpers = require('./helpers');
var reporters = require('../../').reporters;

var AssertionError = assert.AssertionError;
var Base = reporters.Base;
var chaiExpect = chai.expect;
Expand Down Expand Up @@ -417,4 +416,27 @@ describe('Base reporter', function() {
var errOut = stdout.join('\n').trim();
expect(errOut, 'to be', '1) test title:\n Error\n foo\n bar');
});

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(baseConsoleLog, 'was called');
expect(console.log, 'was not called');
});

afterEach(function() {
sandbox.restore();
});
});
plroebuck marked this conversation as resolved.
Show resolved Hide resolved
});
4 changes: 2 additions & 2 deletions test/reporters/xunit.spec.js
View file
Open in desktop
Expand Up @@ -277,14 +277,14 @@ describe('XUnit reporter', function() {
});

describe('when output directed to console', function() {
it("should call 'console.log' with line", function() {
it("should call 'Base.consoleLog' with line", function() {
// :TODO: XUnit needs a trivially testable means to force console.log()
var realProcess = process;
process = false; // eslint-disable-line no-native-reassign, no-global-assign
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@craigtaub craigtaub Mar 11, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mentioned elsewhere

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@plroebuck plroebuck Mar 24, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#2698 (comment)

Are you not experiencing failure with this specific test?

Copy link
Contributor Author

@craigtaub craigtaub Mar 24, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@craigtaub craigtaub Mar 24, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


var xunit = new XUnit(runner);
var fakeThis = {fileStream: false};
var consoleLogStub = sinon.stub(console, 'log');
var consoleLogStub = sinon.stub(Base, 'consoleLog');
xunit.write.call(fakeThis, expectedLine);
consoleLogStub.restore();

Expand Down