test: validate arguments to mustCall(), fix incorrect tests #10692

nfriedly · 2017-01-08T21:47:02Z

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)

tests

Notes: this is a subset of the changes in #9031 as requested by @mscdex and @targos.

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message.

cjihrig · 2017-01-08T21:51:02Z

test/common.js

@@ -423,8 +423,10 @@ function runCallChecks(exitCode) {
 }


-exports.mustCall = function(fn, expected) {
-  if (typeof expected !== 'number') expected = 1;
+exports.mustCall = function(fn, expected = 1) {


I don't think this syntax is supported on v4. This will need to be changed now or during backport.

Oh, I didn't realize this would be backported. Which is the preferred method - older syntax everywhere, or change during the backport?

I think the simplest thing to do would be to update this PR. If expected === undefined, set it to 1. Otherwise, throw a TypeError if typeof expected !== 'number'. I don't think we need to worry about negative numbers slipping through, because they should blow up during development.

Done. I tried running the test under v4 just to see it, but there are other bits of modern elsewhere in common.js :/

cjihrig · 2017-01-08T22:29:29Z

test/common.js

@@ -424,7 +424,12 @@ function runCallChecks(exitCode) {


 exports.mustCall = function(fn, expected) {
-  if (typeof expected !== 'number') expected = 1;
+  if (expected === undefined) {


Can you drop the curly braces on both if statements.

cjihrig · 2017-01-08T22:30:00Z

test/common.js

+  if (expected === undefined) {
+    expected = 1;
+  }
+  if (typeof expected !== 'number' || expected < 0) {


Make this an else if, and drop the expected < 0 check.

cjihrig · 2017-01-08T22:30:20Z

test/common.js

+    expected = 1;
+  }
+  if (typeof expected !== 'number' || expected < 0) {
+    throw new RangeError(`Invalid expected value: ${expected}`);


Change this to a TypeError.

cjihrig · 2017-01-08T22:31:12Z

test/parallel/test-common.js

+
+
+assert.throws(function() {
+  common.mustCall(function() {}, 'foo')();


The trailing () shouldn't be necessary here.

cjihrig · 2017-01-08T22:31:20Z

test/parallel/test-common.js

+}, /invalid expected value: foo/i);
+
+assert.throws(function() {
+  common.mustCall(function() {}, /foo/)();


cjihrig · 2017-01-08T22:32:59Z

test/parallel/test-common.js

+
+assert.throws(function() {
+  common.mustCall(function() {}, 'foo')();
+}, /invalid expected value: foo/i);


Can you do /^TypeError: Invalid expected value: foo$/. Also, drop the insensitive check.

cjihrig · 2017-01-08T22:33:17Z

test/parallel/test-common.js

+
+assert.throws(function() {
+  common.mustCall(function() {}, /foo/)();
+}, /invalid expected value: \/foo\//i);


Same thing here.

nfriedly · 2017-01-08T22:48:36Z

@cjihrig I made several improvements that I believe address everything you pointed out, and then rebased it back to two commits because the log was getting a bit hairy.

cjihrig · 2017-01-08T22:58:21Z

test/common.js

@@ -424,7 +424,9 @@ function runCallChecks(exitCode) {


 exports.mustCall = function(fn, expected) {
-  if (typeof expected !== 'number') expected = 1;
+  if (expected === undefined) expected = 1;


Move expected = 1; to its own line please.

cjihrig

LGTM. Thanks!

lpinca · 2017-01-09T07:40:37Z

test/parallel/test-common.js

@@ -5,3 +5,12 @@ const assert = require('assert');
 common.globalCheck = false;
 global.gc = 42;  // Not a valid global unless --expose_gc is set.
 assert.deepStrictEqual(common.leakedGlobals(), ['gc']);
+
+


Nit: can you please remove one of these 2 blank lines?

targos · 2017-01-09T09:14:20Z

CI: https://ci.nodejs.org/job/node-test-pull-request/5758/

instead of silently overwriting invalid values with the default

Trott · 2017-01-10T05:28:10Z

LGTM and thanks! 🎉

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message. PR-URL: #10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

instead of silently overwriting invalid values with the default PR-URL: #10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

jasnell · 2017-01-10T17:31:12Z

Landed in 762a303

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message. PR-URL: nodejs#10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

instead of silently overwriting invalid values with the default PR-URL: nodejs#10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message. PR-URL: nodejs#10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

instead of silently overwriting invalid values with the default PR-URL: nodejs#10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message. PR-URL: nodejs#10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

instead of silently overwriting invalid values with the default PR-URL: nodejs#10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message. PR-URL: nodejs#10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

instead of silently overwriting invalid values with the default PR-URL: nodejs#10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

MylesBorins · 2017-03-08T00:32:36Z

~~This is causing failures on v6.x. Can someone dig in?~~

Missed a commit in the backport... nvm

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message. PR-URL: #10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

instead of silently overwriting invalid values with the default PR-URL: #10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message. PR-URL: #10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

instead of silently overwriting invalid values with the default PR-URL: #10692 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michal Zasso <[email protected]> Reviewed-By: Rich Trott <[email protected]>

fix misplaced ) in http response statuscode test

c6ff3d8

This fixes a misplaced parenthesis in each of the tests in test/parallel/test-http-response-statuscodes.js, causing the tests to pass as long as any error was thrown, without validating the error message.

nodejs-github-bot added the test Issues and PRs related to the tests. label Jan 8, 2017

nfriedly mentioned this pull request Jan 8, 2017

http: more strict statusCode validation #9031

Closed

3 tasks

cjihrig reviewed Jan 8, 2017

View reviewed changes

nfriedly changed the title ~~Validate arguments to mustCall(), fix incorrect tests~~ test: alidate arguments to mustCall(), fix incorrect tests Jan 8, 2017

nfriedly changed the title ~~test: alidate arguments to mustCall(), fix incorrect tests~~ test: validate arguments to mustCall(), fix incorrect tests Jan 8, 2017

cjihrig reviewed Jan 8, 2017

View reviewed changes

nfriedly force-pushed the fix-test-http-response-statuscode branch from 3dac8a1 to c5ba0c3 Compare January 8, 2017 22:46

cjihrig reviewed Jan 8, 2017

View reviewed changes

cjihrig approved these changes Jan 8, 2017

View reviewed changes

nfriedly force-pushed the fix-test-http-response-statuscode branch from c5ba0c3 to f3c1855 Compare January 8, 2017 23:03

jasnell approved these changes Jan 9, 2017

View reviewed changes

lpinca reviewed Jan 9, 2017

View reviewed changes

lpinca approved these changes Jan 9, 2017

View reviewed changes

targos approved these changes Jan 9, 2017

View reviewed changes

validate 'expected' argument to mustCall()

cd7ce11

instead of silently overwriting invalid values with the default

nfriedly force-pushed the fix-test-http-response-statuscode branch from f3c1855 to cd7ce11 Compare January 9, 2017 14:39

jasnell closed this Jan 10, 2017

italoacasas mentioned this pull request Jan 25, 2017

Proposal to update v7.x-staging #10989

Closed

italoacasas mentioned this pull request Jan 29, 2017

v7.5.0 proposal #11062

Merged

MylesBorins added dont-land-on-v4.x labels Mar 8, 2017

MylesBorins added land-on-v6.x and removed lts-watch-v6.x labels Mar 8, 2017

MylesBorins mentioned this pull request Mar 9, 2017

v6.10.1 proposal #11759

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

test: validate arguments to mustCall(), fix incorrect tests #10692

test: validate arguments to mustCall(), fix incorrect tests #10692

nfriedly commented Jan 8, 2017

cjihrig Jan 8, 2017

nfriedly Jan 8, 2017

cjihrig Jan 8, 2017

nfriedly Jan 8, 2017

cjihrig Jan 8, 2017

cjihrig Jan 8, 2017

cjihrig Jan 8, 2017

cjihrig Jan 8, 2017

cjihrig Jan 8, 2017

cjihrig Jan 8, 2017

cjihrig Jan 8, 2017

nfriedly commented Jan 8, 2017 •

edited

Loading

cjihrig Jan 8, 2017

cjihrig left a comment

lpinca Jan 9, 2017

targos commented Jan 9, 2017

Trott commented Jan 10, 2017

jasnell commented Jan 10, 2017

MylesBorins commented Mar 8, 2017 •

edited

Loading



		assert.throws(function() {
		common.mustCall(function() {}, 'foo')();

test: validate arguments to mustCall(), fix incorrect tests #10692

test: validate arguments to mustCall(), fix incorrect tests #10692

Conversation

nfriedly commented Jan 8, 2017

Checklist

Affected core subsystem(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfriedly commented Jan 8, 2017 • edited Loading

Choose a reason for hiding this comment

cjihrig left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

targos commented Jan 9, 2017

Trott commented Jan 10, 2017

jasnell commented Jan 10, 2017

MylesBorins commented Mar 8, 2017 • edited Loading

nfriedly commented Jan 8, 2017 •

edited

Loading

MylesBorins commented Mar 8, 2017 •

edited

Loading