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

assert.fail() accept a single argument or two arguments #12293

Closed
wants to merge 3 commits into
base: master
from
Jump to file or symbol
Failed to load files and symbols.
+146 −110
Diff settings

Always

Just for now

View
@@ -110,9 +110,6 @@ rules:
}, {
selector: "ThrowStatement > CallExpression[callee.name=/Error$/]",
message: "Use new keyword when throwing an Error."
}, {
selector: "CallExpression[callee.object.name='assert'][callee.property.name='fail'][arguments.length=1]",
message: "assert.fail() message should be third argument"
}]
no-tabs: 2
no-trailing-spaces: 2
View
@@ -256,14 +256,15 @@ If the values are not equal, an `AssertionError` is thrown with a `message`
property set equal to the value of the `message` parameter. If the `message`
parameter is undefined, a default error message is assigned.
## assert.fail(message)
## assert.fail(actual, expected, message, operator)
<!-- YAML
added: v0.1.21
-->
* `actual` {any}
* `expected` {any}
* `message` {any}
* `operator` {string}
* `operator` {string} (default: '!=')
Throws an `AssertionError`. If `message` is falsy, the error message is set as
the values of `actual` and `expected` separated by the provided `operator`.
@@ -277,6 +278,12 @@ assert.fail(1, 2, undefined, '>');
assert.fail(1, 2, 'whoops', '>');
// AssertionError: whoops
assert.fail('boom');
// AssertionError: boom
assert.fail('a', 'b');
// AssertionError: 'a' != 'b'
```
## assert.ifError(value)
View
@@ -79,6 +79,10 @@ function getMessage(self) {
// display purposes.
function fail(actual, expected, message, operator, stackStartFunction) {
if (arguments.length === 1)
message = actual;
if (arguments.length === 2)
operator = '!=';

This comment has been minimized.

@addaleax

addaleax Apr 10, 2017

Member

You could just write message = actual, operator = '!=', stackStartFunction = undefined as part of the function arguments, right?

@addaleax

addaleax Apr 10, 2017

Member

You could just write message = actual, operator = '!=', stackStartFunction = undefined as part of the function arguments, right?

This comment has been minimized.

@Trott

Trott Apr 11, 2017

Member

There are at least three issues with that.

First, that introduces additional behavior changes I was hoping to avoid.

Current behavior preserved in this PR:

> assert.fail()
AssertionError: undefined undefined undefined

With the default parameters change:

> assert.fail()
AssertionError: undefined != undefined

Second, it breaks the two-argument feature added here.

Current PR:

> assert.fail('first', 'second');
AssertionError: 'first' != 'second'

With the suggested change:

> assert.fail('first', 'second')
AssertionError: first

Third, and most importantly, it breaks one of the two current usable argument combinations for the function.

Current behavior also preserved in this PR:

> assert.fail('first', 'second', undefined, 'operator')
AssertionError: 'first' operator 'second'

With the default parameters as suggested replacing the arguments.length checks:

> assert.fail('first', 'second', undefined, 'operator')
AssertionError: first

There's no doubt a way around this, and maybe the problem is my lack of familiarity with default parameters?

@Trott

Trott Apr 11, 2017

Member

There are at least three issues with that.

First, that introduces additional behavior changes I was hoping to avoid.

Current behavior preserved in this PR:

> assert.fail()
AssertionError: undefined undefined undefined

With the default parameters change:

> assert.fail()
AssertionError: undefined != undefined

Second, it breaks the two-argument feature added here.

Current PR:

> assert.fail('first', 'second');
AssertionError: 'first' != 'second'

With the suggested change:

> assert.fail('first', 'second')
AssertionError: first

Third, and most importantly, it breaks one of the two current usable argument combinations for the function.

Current behavior also preserved in this PR:

> assert.fail('first', 'second', undefined, 'operator')
AssertionError: 'first' operator 'second'

With the default parameters as suggested replacing the arguments.length checks:

> assert.fail('first', 'second', undefined, 'operator')
AssertionError: first

There's no doubt a way around this, and maybe the problem is my lack of familiarity with default parameters?

throw new assert.AssertionError({
message: message,
actual: actual,
View
@@ -246,12 +246,6 @@ Checks whether `IPv6` is supported on this platform.
Checks if there are multiple localhosts available.
### fail(msg)
* `msg` [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
* return [&lt;Boolean>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type)
Throws an `AssertionError` with `msg`
### fileExists(pathname)
* pathname [&lt;String>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#String_type)
* return [&lt;Boolean>](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Data_structures#Boolean_type)
View
@@ -404,7 +404,7 @@ process.on('exit', function() {
if (!exports.globalCheck) return;
const leaked = leakedGlobals();
if (leaked.length > 0) {
fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
assert.fail(`Unexpected global(s) found: ${leaked.join(', ')}`);
}
});
@@ -507,14 +507,9 @@ exports.canCreateSymLink = function() {
return true;
};
function fail(msg) {
assert.fail(null, null, msg);
}
exports.fail = fail;
exports.mustNotCall = function(msg) {
return function mustNotCall() {
fail(msg || 'function should not have been called');
assert.fail(msg || 'function should not have been called');
};
};
@@ -627,7 +622,7 @@ exports.WPT = {
},
assert_array_equals: assert.deepStrictEqual,
assert_unreached(desc) {
assert.fail(undefined, undefined, `Reached unreachable code: ${desc}`);
assert.fail(`Reached unreachable code: ${desc}`);
}
};
@@ -217,7 +217,7 @@ TestSession.prototype.sendInspectorCommands = function(commands) {
for (const id in this.messages_) {
s += id + ', ';
}
common.fail('Messages without response: ' +
assert.fail('Messages without response: ' +
s.substring(0, s.length - 2));
}, TIMEOUT);
});
@@ -28,7 +28,7 @@ function callbackOnly(err) {
}
function onEvent(err) {
common.fail('Error should not be emitted if there is callback');
assert.fail('Error should not be emitted if there is callback');
}
function onError(err) {
@@ -453,7 +453,7 @@ TEST(function test_lookup_all_mixed(done) {
else if (isIPv6(ip.address))
assert.strictEqual(ip.family, 6);
else
common.fail('unexpected IP address');
assert.fail('unexpected IP address');
});
done();
@@ -38,7 +38,7 @@ tls.connect(opts, fail).on('error', common.mustCall((err) => {
}));
function fail() {
common.fail('should fail to connect');
assert.fail('should fail to connect');
}
// New secure contexts have the well-known root CAs.
@@ -0,0 +1,33 @@
'use strict';
require('../common');
const assert = require('assert');
// no args
assert.throws(
() => { assert.fail(); },
/^AssertionError: undefined undefined undefined$/
);
// one arg = message
assert.throws(
() => { assert.fail('custom message'); },
/^AssertionError: custom message$/
);
// two args only, operator defaults to '!='
assert.throws(
() => { assert.fail('first', 'second'); },
/^AssertionError: 'first' != 'second'$/
);
// three args
assert.throws(
() => { assert.fail('ignored', 'ignored', 'another custom message'); },
/^AssertionError: another custom message$/
);
// no third arg (but a fourth arg)
assert.throws(
() => { assert.fail('first', 'second', undefined, 'operator'); },
/^AssertionError: 'first' operator 'second'$/
);
@@ -20,10 +20,11 @@
// USE OR OTHER DEALINGS IN THE SOFTWARE.
'use strict';
const common = require('../common');
require('../common');
const assert = require('assert');
process.on('beforeExit', function() {
common.fail('exit should not allow this to occur');
assert.fail('exit should not allow this to occur');
});
process.exit();
@@ -37,7 +37,7 @@ switch (process.argv[2] || '') {
case 'spawn':
break;
default:
common.fail();
assert.fail();
}
function checkExit(statusCode) {
@@ -42,7 +42,7 @@ if (process.argv[2] === 'child') {
child.stderr.setEncoding('utf8');
child.stderr.on('data', function(data) {
common.fail(`Unexpected parent stderr: ${data}`);
assert.fail(`Unexpected parent stderr: ${data}`);
});
// check if we receive both 'hello' at start and 'goodbye' at end
@@ -51,7 +51,7 @@ if (cluster.isMaster) {
setTimeout(function() { client.end(); }, 50);
}).on('error', function(e) {
console.error(e);
common.fail('server.listen failed');
assert.fail('server.listen failed');
cluster.worker.disconnect();
});
}
@@ -40,8 +40,8 @@ assert.throws(function() {
}, /^TypeError: Invalid expected value: \/foo\/$/);
// common.fail() tests
// assert.fail() tests
assert.throws(
() => { common.fail('fhqwhgads'); },
() => { assert.fail('fhqwhgads'); },
/^AssertionError: fhqwhgads$/
);
@@ -41,7 +41,7 @@ const dgram = require('dgram');
socket.on('error', (err) => {
socket.close();
common.fail(`Unexpected error on udp4 socket. ${err.toString()}`);
assert.fail(`Unexpected error on udp4 socket. ${err.toString()}`);
});
socket.bind(0, common.localhostIPv4);
@@ -65,7 +65,7 @@ if (common.hasIPv6) {
socket.on('error', (err) => {
socket.close();
common.fail(`Unexpected error on udp6 socket. ${err.toString()}`);
assert.fail(`Unexpected error on udp6 socket. ${err.toString()}`);
});
socket.bind(0, localhost);
@@ -40,7 +40,7 @@ socket.on('error', (err) => {
return;
}
common.fail(`Unexpected error: ${err}`);
assert.fail(`Unexpected error: ${err}`);
});
// Initiate a few send() operations, which will fail.
@@ -9,6 +9,7 @@
*/
const common = require('../common');
const assert = require('assert');
const domain = require('domain');
const child_process = require('child_process');
@@ -183,14 +184,14 @@ if (process.argv[2] === 'child') {
test.expectedMessages.forEach(function(expectedMessage) {
if (test.messagesReceived === undefined ||
test.messagesReceived.indexOf(expectedMessage) === -1)
common.fail('test ' + test.fn.name + ' should have sent message: ' +
assert.fail('test ' + test.fn.name + ' should have sent message: ' +
expectedMessage + ' but didn\'t');
});
if (test.messagesReceived) {
test.messagesReceived.forEach(function(receivedMessage) {
if (test.expectedMessages.indexOf(receivedMessage) === -1) {
common.fail('test ' + test.fn.name +
assert.fail('test ' + test.fn.name +
' should not have sent message: ' + receivedMessage +
' but did');
}
@@ -53,9 +53,9 @@ const EventEmitter = require('events');
});
ee.on('hello', hello);
ee.once('foo', common.fail);
ee.once('foo', assert.fail);
assert.deepStrictEqual(['hello', 'foo'], events_new_listener_emitted);
assert.deepStrictEqual([hello, common.fail], listeners_new_listener_emitted);
assert.deepStrictEqual([hello, assert.fail], listeners_new_listener_emitted);
ee.emit('hello', 'a', 'b');
}
@@ -21,7 +21,7 @@
'use strict';
const common = require('../common');
require('../common');
const assert = require('assert');
const EventEmitter = require('events').EventEmitter;
@@ -35,12 +35,12 @@ assert.strictEqual(fl.length, 0);
assert(!(e._events instanceof Object));
assert.deepStrictEqual(Object.keys(e._events), []);
e.on('foo', common.fail);
e.on('foo', assert.fail);
fl = e.listeners('foo');
assert.strictEqual(e._events.foo, common.fail);
assert.strictEqual(e._events.foo, assert.fail);
assert(Array.isArray(fl));
assert.strictEqual(fl.length, 1);
assert.strictEqual(fl[0], common.fail);
assert.strictEqual(fl[0], assert.fail);
e.listeners('bar');
@@ -49,12 +49,12 @@ fl = e.listeners('foo');
assert(Array.isArray(e._events.foo));
assert.strictEqual(e._events.foo.length, 2);
assert.strictEqual(e._events.foo[0], common.fail);
assert.strictEqual(e._events.foo[0], assert.fail);
assert.strictEqual(e._events.foo[1], assert.ok);
assert(Array.isArray(fl));
assert.strictEqual(fl.length, 2);
assert.strictEqual(fl[0], common.fail);
assert.strictEqual(fl[0], assert.fail);
assert.strictEqual(fl[1], assert.ok);
console.log('ok');
@@ -34,7 +34,7 @@ e.emit('hello', 'a', 'b');
e.emit('hello', 'a', 'b');
const remove = function() {
common.fail('once->foo should not be emitted');
assert.fail('once->foo should not be emitted');
};
e.once('foo', remove);
@@ -70,11 +70,11 @@ function listener2() {}
const ee = new EventEmitter();
function remove1() {
common.fail('remove1 should not have been called');
assert.fail('remove1 should not have been called');
}
function remove2() {
common.fail('remove2 should not have been called');
assert.fail('remove2 should not have been called');
}
ee.on('removeListener', common.mustCall(function(name, cb) {
@@ -21,6 +21,7 @@
'use strict';
const common = require('../common');
const assert = require('assert');
process.on('uncaughtException', function(err) {
console.log('Caught exception: ' + err);
@@ -32,4 +33,4 @@ setTimeout(common.mustCall(function() {
// Intentionally cause an exception, but don't catch it.
nonexistentFunc(); // eslint-disable-line no-undef
common.fail('This will not run.');
assert.fail('This will not run.');
@@ -95,10 +95,10 @@ check(fs.symlink, fs.symlinkSync, fileUrl, 'foobar');
check(fs.symlink, fs.symlinkSync, 'foobar', fileUrl);
check(fs.truncate, fs.truncateSync, fileUrl);
check(fs.unlink, fs.unlinkSync, fileUrl);
check(null, fs.unwatchFile, fileUrl, common.fail);
check(null, fs.unwatchFile, fileUrl, assert.fail);
check(fs.utimes, fs.utimesSync, fileUrl, 0, 0);
check(null, fs.watch, fileUrl, common.fail);
check(null, fs.watchFile, fileUrl, common.fail);
check(null, fs.watch, fileUrl, assert.fail);
check(null, fs.watchFile, fileUrl, assert.fail);
check(fs.writeFile, fs.writeFileSync, fileUrl, 'abc');
check(fs.access, fs.accessSync, fileUrl2);
@@ -123,10 +123,10 @@ check(fs.symlink, fs.symlinkSync, fileUrl2, 'foobar');
check(fs.symlink, fs.symlinkSync, 'foobar', fileUrl2);
check(fs.truncate, fs.truncateSync, fileUrl2);
check(fs.unlink, fs.unlinkSync, fileUrl2);
check(null, fs.unwatchFile, fileUrl2, common.fail);
check(null, fs.unwatchFile, fileUrl2, assert.fail);
check(fs.utimes, fs.utimesSync, fileUrl2, 0, 0);
check(null, fs.watch, fileUrl2, common.fail);
check(null, fs.watchFile, fileUrl2, common.fail);
check(null, fs.watch, fileUrl2, assert.fail);
check(null, fs.watchFile, fileUrl2, assert.fail);
check(fs.writeFile, fs.writeFileSync, fileUrl2, 'abc');
// an 'error' for exists means that it doesn't exist.
@@ -62,7 +62,7 @@ fs.open('.', 'r', undefined, common.mustCall(function(err, fd) {
try {
stats = fs.fstatSync(fd);
} catch (err) {
common.fail(err);
assert.fail(err);
}
if (stats) {
console.dir(stats);
Oops, something went wrong.
ProTip! Use n and p to navigate between commits in a pull request.