GeezerVisonThe World Wide Web, restored to 800 pixels of glory. | |||||||||||||||||||||||||||
| Home|About|Gaming cabinet|Archives|How it works|Starter sites | |||||||||||||||||||||||||||
GeezerVison is showing github.com — Migrate _stream_readable errors to internal/errors by benhalverson · Pull Request #15042 · nodejs/node Conversation
|
|
@jasnell I wasn't able to get the tests to pass... my attempt... |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Ping @nodejs/streams @mcollina |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
As stated elsewhere, I'm very 👎 on this change until we have a clear strategy on how to keep the same behavior on |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@mcollina thanks for the feedback. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Hello @benhalverson and welcome. Thank you for the contribution 🥇 I've marked it as |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Can you please put this in alphabetical order :-) thank you!
Also, I believe these need line wrapped at 80 chars. Run make lint to check
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
s/ERR_UNDERSCORE_READ_NOT_IMPLEMENTED/ERR_STREAM_READ_NOT_IMPLEMENTED
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
s/ERR_END_READABLE_CALLED_ON_NONEMPTY_STREAM/ERR_STREAM_READABLE_CALLED_ON_NONEMPTY
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
To explain @refack's comments a bit more. The streams module in core is kept in sync with the standalone |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was an error while loading. Please reload this page.
|
At the latest streams wg we decided to unblock this. We would like this (and the equals for Writable, Duplex and Transform) to ship in Node 9. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@benhalverson I think you can use See https://github.com/nodejs/node/blob/c2e838ee13244dd3dbd98ed86ef1966fe7cd90f1/test/parallel/test-require-invalid-package.js as an example. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Thanks @mcollina I'll take a look at the example |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@benhalverson what is the status of this? Would you mind updating to |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
why is this commented out?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I was testing and forgot to uncomment
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I could use some assistance on this test...
current test
function pushError() {
assert.throws(function() {
r.push(Buffer.allocUnsafe(1));
}, /^ERR_STREAM_PUSH_AFTER_EOF/);
}
What I have tried so far:
function pushError() {
common.expectsError({
code: 'ERR_STREAM_PUSH_AFTER_EOF',
message: 'stream.push() after EOF'
});
}
This gives the following error.
Path: parallel/test-stream-unshift-read-race
ok
events.js:182
throw er; // Unhandled 'error' event
^
Error: stream.push() after EOF
at readableAddChunk (_stream_readable.js:243:30)
at Readable.push (_stream_readable.js:211:10)
at pushError (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:77:9)
at Timeout._onTimeout (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:64:25)
at ontimeout (timers.js:471:11)
at tryOnTimeout (timers.js:305:5)
at Timer.listOnTimeout (timers.js:265:5)
Without r.push(Buffer.allocUnsafe(1)); I get this error.
Path: parallel/test-stream-unshift-read-race
Mismatched <anonymous> function calls. Expected exactly 1, actual 0.
at Object.exports.mustCall (/Users/benhalverson/projects/node/test/common/index.js:482:10)
at Object.expectsError (/Users/benhalverson/projects/node/test/common/index.js:707:27)
at pushError (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:78:14)
at Timeout._onTimeout (/Users/benhalverson/projects/node/test/parallel/test-stream-unshift-read-race.js:64:25)
at ontimeout (timers.js:471:11)
at tryOnTimeout (timers.js:305:5)
at Timer.listOnTimeout (timers.js:265:5)
0: asdfasdfas
1: 1234dfasdf
2: 1234asdfas
3: 1234dfasdf
4: 1234asdfas
5: 1234dfasdf
6: 1234asdfas
7: 1234dfasdf
8: 1234asdfas
9: 1234dfasdf
a: 1234asdfas
b: 1234dfasdf
c: 1234asdfas
d: 1234dfasdf
e: 1234asdfas
f: 1234dfasdf
g: 1234asdfa
h: 1234
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I figured it out @mcollina. Reading the docs helped 👍 https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Something went wrong with your latest git operations. Can you please rebase? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I would recommend that you take the changes you have done, start with a fresh branch, and do a force push here. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I was thinking the same... I did the rebase got a bunch of conflicts and it ended up like this after I fixed the conflicts ¯_(ツ)_/¯ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@benhalverson against what branch did you rebase? I am not sure how you set up your clone but I guess you should rebase like |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@BridgeAR I used my local version of node and rebased against upstream/master. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
can you use common.expectsError here as well?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I've updated this test
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM with a nit
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
This should be ERR_STREAM_ENDREADABLE_CALLED_ON_NONEMPTY.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
I've updated this line
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
There is the error code to be changed, I'm moving my feedback to "request changes" instead of approved.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Please remove the empty template string.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
Just as a suggestion - it would be nicer to use
common.expectsError(throwingFn, errorObj) // instead of assert.throws(throwingFn, common.expectsError(errorObj))
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
A empty template string?
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Oops 😂 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@mcollina is there anything else I need to change? |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Looks like the CI job had conflicts even though the PR says it can be merge automatically. @benhalverson could you squash the commits down 1 one (I'm thinking maybe something that is added/them removed is the source of the conflicts) and then we can run the CI again. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have went ahead and squashed the commits in this PR. CI: https://ci.nodejs.org/job/node-test-pull-request/11041/ |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Oh, this feature is nice: https://ci.nodejs.org/job/node-test-linter/13033/console @benhalverson Do you have time to document this error code? If not I can do that, just don't want to delay this PR for too long. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@joyeecheung that error code is not present in readable. I think this should land asap before 9. |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
@mcollina Oh, I think I posted in the wrong thread, this error is from the querystring error migration PR. The errors for this one is Also there are some incorrect use of |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
I have fixed the errors and added documentation. New CI: https://ci.nodejs.org/job/node-test-pull-request/11044/ @jasnell @BridgeAR @mcollina @addaleax @gireeshpunathil @mhdawson PTAL |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
The reason will be displayed to describe this comment to others. Learn more.
FWIW
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
(CI failures are unrelated build issues.) |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
|
Landed as 88fb359 |
Sorry, something went wrong.
There was an error while loading. Please reload this page.
PR-URL: #15042 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs/node#15042 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
PR-URL: nodejs/node#15042 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gireesh Punathil <[email protected]> Reviewed-By: Joyee Cheung <[email protected]>
There was an error while loading. Please reload this page.
Successfully merging this pull request may close these issues.
There was an error while loading. Please reload this page.