lib, test: migrate _http_outgoing.js's remaining errors to internal/errors.js #17837

b0yfriend · 2017-12-23T11:16:56Z

A couple of lib/_http_outgoing.js's errors were still in the "old style":
throw new Error(<some message here>).

This PR migrates those 2 old style errors to the "new style": internal/errors.js's error-system.

In the future, changes to these errors' messages won't break semver-major status. With the old style, changes to these errors' messages broke semver-major status. It was inconvenient.

This PR also adapts _http_outgoing.js's tests accordingly.

Refs: #17709

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)

lib, test

joyeecheung · 2017-12-23T17:51:14Z

test/parallel/test-http-res-write-after-end.js

@@ -26,7 +26,10 @@ const http = require('http');

 const server = http.Server(common.mustCall(function(req, res) {
  res.on('error', common.mustCall(function onResError(err) {


Since common.expectsError returns a function wrapped in common.mustCall that can be used as callbacks, can you simplify this to:

res.on('error', common.expectsError({ code: 'ERR_STREAM_WRITE_AFTER_END', type: Error }));

?

joyeecheung · 2017-12-23T17:54:21Z

test/parallel/test-outgoing-message-pipe.js

-  (err) => {
-    return ((err instanceof Error) && /Cannot pipe, not readable/.test(err));
-  },
+  common.expectsError({


common.expectsError can take a function and wraps it in assert.throws, can you simplify this to

common.expectsError( () => { outgoingMessage.pipe(outgoingMessage); }, { code: 'ERR_STREAM_CANNOT_PIPE', type: Error } );

?

joyeecheung · 2017-12-23T17:55:03Z

test/parallel/test-pipe-outgoing-message-data-emitted-after-ended.js

@@ -18,7 +17,10 @@ const server = http.createServer(common.mustCall(function(req, res) {
    res.end();
    myStream.emit('data', 'some data');
    res.on('error', common.mustCall(function(err) {
-      assert.strictEqual(err.message, 'write after end');
+      common.expectsError({


Can you simplify this to

res.on('error', common.expectsError({ code: 'ERR_STREAM_WRITE_AFTER_END', type: Error }));

?

A couple of lib/_http_outgoing.js's errors were still in the "old style": `throw new Error(<some message here>)`. This commit migrates those 2 old style errors to the "new style": internal/errors.js's error-system. In the future, changes to these errors' messages won't break semver-major status. With the old style, changes to these errors' messages broke semver-major status. It was inconvenient. Refs: #17709

Two of _http_outgoing.js's errors were migrated to the internal/errors.js system. This commit adapts _http_outgoing.js's tests accordingly. Refs: #17709

_http_outgoing.js's tests perform error-checks. They ensure that appropriate errors are thrown in the appropriate circumstances. A few error-checks were bloated. They used unnecessary intermediate functions. E.g. there were unnecessary combinations of `common.mustCall()` & `common.expectsError()` and `common.expectsError()` & `assert.throws()`. This commit removes those unnecessary intermediate functions. This simplifies the given error-checks.

b0yfriend · 2017-12-24T01:02:19Z

test/parallel/test-http-server-write-after-end.js


 // Fix for https://github.com/nodejs/node/issues/14368

 const server = http.createServer(handle);

 function handle(req, res) {
  res.on('error', common.mustCall((err) => {
-    assert.strictEqual(err.message, 'write after end');


Oh, I missed this. I'll fix it soon!

^^ Addressed this remark in the comment below.

b0yfriend · 2017-12-24T11:10:53Z

test/parallel/test-http-server-write-after-end.js

+    common.expectsError({
+      code: 'ERR_STREAM_WRITE_AFTER_END',
+      type: Error
+    })(err);
    server.close();


Never mind. I thought I could remove the common.mustCall() on line 11, and simplify it to just common.expectsError().

That's what I did in test-pipe-outgoing-message-data-emitted-after-ended.js on lines 19-21.

However, I can't. This block performs server.close() in addition to common.expectsError(). That additional functionality prevents me from simplifying common.mustCall() => common.expectsError().

The way to handle that would be to register two on('error') handlers, one with the expectsError() check and the second with the server.close() call.

b0yfriend · 2017-12-24T11:12:36Z

@joyeecheung Are there any other concerns?

joyeecheung · 2017-12-24T21:26:21Z

These needs two TSC approvals. cc @nodejs/tsc

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

b0yfriend · 2017-12-24T22:50:58Z

Three raspbian builds failed. How do we handle this?

lpinca · 2017-12-25T20:59:23Z

@acparas failures seems to to be unrelated to this change.

joyeecheung · 2017-12-27T19:18:32Z

Landed in d3ac18a, thanks!

A couple of lib/_http_outgoing.js's errors were still in the "old style": `throw new Error(<some message here>)`. This commit migrates those 2 old style errors to the "new style": internal/errors.js's error-system. In the future, changes to these errors' messages won't break semver-major status. With the old style, changes to these errors' messages broke semver-major status. It was inconvenient. Refs: #17709 PR-URL: #17837 Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: James M Snell <[email protected]>

nodejs-github-bot added the http Issues or PRs related to the http subsystem. label Dec 23, 2017

joyeecheung added the errors Issues and PRs related to JavaScript errors originated in Node.js core. label Dec 23, 2017

joyeecheung reviewed Dec 23, 2017

View reviewed changes

joyeecheung mentioned this pull request Dec 23, 2017

Tracking issue: Migrate errors to internal/errors.js #17709

Closed

6 tasks

b0yfriend added 3 commits December 23, 2017 15:55

test: adapt _http_outgoing.js tests to new errors

6ba656c

Two of _http_outgoing.js's errors were migrated to the internal/errors.js system. This commit adapts _http_outgoing.js's tests accordingly. Refs: #17709

b0yfriend commented Dec 24, 2017

View reviewed changes

joyeecheung approved these changes Dec 24, 2017

View reviewed changes

joyeecheung added the semver-major PRs that contain breaking changes and should be released in the next major version. label Dec 24, 2017

lpinca approved these changes Dec 25, 2017

View reviewed changes

jasnell approved these changes Dec 27, 2017

View reviewed changes

joyeecheung added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2017

joyeecheung closed this Dec 27, 2017

joyeecheung removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 27, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

lib, test: migrate _http_outgoing.js's remaining errors to internal/errors.js #17837

lib, test: migrate _http_outgoing.js's remaining errors to internal/errors.js #17837

b0yfriend commented Dec 23, 2017 •

edited

Loading

joyeecheung Dec 23, 2017

b0yfriend Dec 24, 2017

joyeecheung Dec 23, 2017

b0yfriend Dec 24, 2017

joyeecheung Dec 23, 2017

b0yfriend Dec 24, 2017

b0yfriend Dec 24, 2017 •

edited

Loading

b0yfriend Dec 24, 2017

b0yfriend Dec 24, 2017

jasnell Dec 27, 2017

b0yfriend commented Dec 24, 2017

joyeecheung commented Dec 24, 2017

b0yfriend commented Dec 24, 2017

lpinca commented Dec 25, 2017

joyeecheung commented Dec 27, 2017

		@@ -26,7 +26,10 @@ const http = require('http');

		const server = http.Server(common.mustCall(function(req, res) {
		res.on('error', common.mustCall(function onResError(err) {

lib, test: migrate _http_outgoing.js's remaining errors to internal/errors.js #17837

lib, test: migrate _http_outgoing.js's remaining errors to internal/errors.js #17837

Conversation

b0yfriend commented Dec 23, 2017 • edited Loading

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

b0yfriend Dec 24, 2017 • edited Loading

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

b0yfriend commented Dec 24, 2017

joyeecheung commented Dec 24, 2017

b0yfriend commented Dec 24, 2017

lpinca commented Dec 25, 2017

joyeecheung commented Dec 27, 2017

b0yfriend commented Dec 23, 2017 •

edited

Loading

b0yfriend Dec 24, 2017 •

edited

Loading