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

test: add common.mustNotCall() #11152

Merged
merged 1 commit into from Feb 6, 2017

Conversation

@cjihrig
Contributor

cjihrig commented Feb 3, 2017

This commit adds a mustNotCall() helper for testing. This provides an alternative to using common.fail() as a callback, or creating a callback function for the sole purpose of calling common.fail().

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)
@Trott

Trott approved these changes Feb 3, 2017

Somewhat rubber-stamp-y LGTM if CI is green.

Show outdated Hide outdated test/common.js
fail(msg || 'function should not have been called');
};
};

This comment has been minimized.

@jasnell

jasnell Feb 3, 2017

Member

hmm.. the same basic thing can be accomplished using:

common.mustCall(fn, 0);
@jasnell

jasnell Feb 3, 2017

Member

hmm.. the same basic thing can be accomplished using:

common.mustCall(fn, 0);

This comment has been minimized.

@cjihrig

cjihrig Feb 3, 2017

Contributor

It could. The whole point is to get beautiful assertion messages though. Using common.mustCall() would yield:

Mismatched <anonymous> function calls. Expected 0, actual 1.

@cjihrig

cjihrig Feb 3, 2017

Contributor

It could. The whole point is to get beautiful assertion messages though. Using common.mustCall() would yield:

Mismatched <anonymous> function calls. Expected 0, actual 1.

This comment has been minimized.

@Trott

Trott Feb 3, 2017

Member

Additionally, common.mustCall(fn, 0) will only fail when the exit event is emitted on process. The version in this PR fails immediately. That could be very helpful for a test that otherwise will mysteriously time out because something is holding the event loop open indefinitely.

@Trott

Trott Feb 3, 2017

Member

Additionally, common.mustCall(fn, 0) will only fail when the exit event is emitted on process. The version in this PR fails immediately. That could be very helpful for a test that otherwise will mysteriously time out because something is holding the event loop open indefinitely.

This comment has been minimized.

@jasnell

jasnell Feb 3, 2017

Member

Ok, works for me

@jasnell

jasnell Feb 3, 2017

Member

Ok, works for me

This comment has been minimized.

@mhdawson

mhdawson Feb 3, 2017

Member

Just to add my 2 cents I also think common.mustNotCall() makes it immediately obvious what you want to happen without needing the context to know that 0 will imply it should not be called.

@mhdawson

mhdawson Feb 3, 2017

Member

Just to add my 2 cents I also think common.mustNotCall() makes it immediately obvious what you want to happen without needing the context to know that 0 will imply it should not be called.

@jasnell

jasnell approved these changes Feb 3, 2017

@mhdawson

LGTM

Show outdated Hide outdated test/common.js
fail(msg || 'function should not have been called');
};
};

This comment has been minimized.

@mhdawson

mhdawson Feb 3, 2017

Member

Just to add my 2 cents I also think common.mustNotCall() makes it immediately obvious what you want to happen without needing the context to know that 0 will imply it should not be called.

@mhdawson

mhdawson Feb 3, 2017

Member

Just to add my 2 cents I also think common.mustNotCall() makes it immediately obvious what you want to happen without needing the context to know that 0 will imply it should not be called.

Show outdated Hide outdated test/common.js
@@ -497,6 +497,12 @@ function fail(msg) {
}
exports.fail = fail;
exports.mustNotCall = function(msg) {
return function dontCall() {

This comment has been minimized.

@thefourtheye

thefourtheye Feb 4, 2017

Contributor

Would it help to keep this function's name also as mustNotCall? Stacktrace would be better I guess.

@thefourtheye

thefourtheye Feb 4, 2017

Contributor

Would it help to keep this function's name also as mustNotCall? Stacktrace would be better I guess.

Show outdated Hide outdated test/common.js
@@ -497,6 +497,12 @@ function fail(msg) {
}
exports.fail = fail;
exports.mustNotCall = function(msg) {
return function dontCall() {
fail(msg || 'function should not have been called');

This comment has been minimized.

@lpinca

lpinca Feb 4, 2017

Member

Can we do something to prevent this from printing stuff like

AssertionError: [object Object]

when msg is not a string?

@lpinca

lpinca Feb 4, 2017

Member

Can we do something to prevent this from printing stuff like

AssertionError: [object Object]

when msg is not a string?

This comment has been minimized.

@targos

targos Feb 4, 2017

Member

util.format ?

@targos

targos Feb 4, 2017

Member

util.format ?

This comment has been minimized.

@lpinca

lpinca Feb 4, 2017

Member

On second thought I think this is not needed as the function (common.mustNotCall()) must be invoked explicitly and using a non-string argument probably doesn't make sense.

@lpinca

lpinca Feb 4, 2017

Member

On second thought I think this is not needed as the function (common.mustNotCall()) must be invoked explicitly and using a non-string argument probably doesn't make sense.

@cjihrig

This comment has been minimized.

Show comment
Hide comment
@thefourtheye

LGTM if the CI is happy

@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Feb 6, 2017

Member

@nodejs/build @jbergstroem ... something is happening with the OSX build bot. Failing to start on every CI run

Member

jasnell commented Feb 6, 2017

@nodejs/build @jbergstroem ... something is happening with the OSX build bot. Failing to start on every CI run

@hiroppy

hiroppy approved these changes Feb 6, 2017

test: add common.mustNotCall()
This commit adds a mustNotCall() helper for testing. This provides
an alternative to using common.fail() as a callback, or creating
a callback function for the sole purpose of calling common.fail().

PR-URL: #11152
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@cjihrig

This comment has been minimized.

Show comment
Hide comment
@cjihrig

cjihrig Feb 6, 2017

Contributor

Landing this since the only problem is the macOS CI, which I can verify locally.

Contributor

cjihrig commented Feb 6, 2017

Landing this since the only problem is the macOS CI, which I can verify locally.

@cjihrig cjihrig merged commit 7dd82dd into nodejs:master Feb 6, 2017

@cjihrig cjihrig deleted the cjihrig:must-not-call branch Feb 6, 2017

@italoacasas

This comment has been minimized.

Show comment
Hide comment
@italoacasas

italoacasas Feb 9, 2017

Member

@cjihrig do you have time to backport this to v7.x ?

Member

italoacasas commented Feb 9, 2017

@cjihrig do you have time to backport this to v7.x ?

italoacasas added a commit that referenced this pull request Feb 13, 2017

test: add common.mustNotCall()
This commit adds a mustNotCall() helper for testing. This provides
an alternative to using common.fail() as a callback, or creating
a callback function for the sole purpose of calling common.fail().

PR-URL: #11152
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

italoacasas added a commit to italoacasas/node that referenced this pull request Feb 14, 2017

test: add common.mustNotCall()
This commit adds a mustNotCall() helper for testing. This provides
an alternative to using common.fail() as a callback, or creating
a callback function for the sole purpose of calling common.fail().

PR-URL: nodejs#11152
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017

test: add common.mustNotCall()
This commit adds a mustNotCall() helper for testing. This provides
an alternative to using common.fail() as a callback, or creating
a callback function for the sole purpose of calling common.fail().

PR-URL: nodejs#11152
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

This would need backport PRs to land in v6 and v4 ... there are several recent test changes that depend on this so a backport would be preferrable.

Member

jasnell commented Mar 7, 2017

This would need backport PRs to land in v6 and v4 ... there are several recent test changes that depend on this so a backport would be preferrable.

@gibfahn gibfahn referenced this pull request May 15, 2017

Closed

Backporting to v6.x-staging #216

128 of 128 tasks complete

MylesBorins added a commit that referenced this pull request May 15, 2017

test: add common.mustNotCall()
This commit adds a mustNotCall() helper for testing. This provides
an alternative to using common.fail() as a callback, or creating
a callback function for the sole purpose of calling common.fail().

PR-URL: #11152
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins May 15, 2017

Member

backported to v6.x in bea0a6e

Member

MylesBorins commented May 15, 2017

backported to v6.x in bea0a6e

@MylesBorins MylesBorins referenced this pull request May 23, 2017

Merged

v6.11.0 proposal #13059

andrew749 added a commit to michielbaird/node that referenced this pull request Jul 19, 2017

test: add common.mustNotCall()
This commit adds a mustNotCall() helper for testing. This provides
an alternative to using common.fail() as a callback, or creating
a callback function for the sole purpose of calling common.fail().

PR-URL: nodejs/node#11152
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment