Merge nodejs/node master #264

kunalspathak · 2017-05-25T18:44:29Z

Merge commit 260cd41 as of 2017-05-24

Interested commit to review:

a969132 - test: Fixed some failing unit test after merge

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

core, test

This reverts commit 571882c. Removing the process.nextTick() call can prevent the consumer from being able to catch error events. PR-URL: nodejs/node#12854 Fixes: nodejs/node#12841 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

This test ensures that a http client request with the default agent that has a socket that is immediately destroyed can still be caught by adding an error event listener to the request object. PR-URL: nodejs/node#12854 Fixes: nodejs/node#12841 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Ben Noordhuis <[email protected]>

I came across this template class but I don't understand why it is there. It is not used in the template specialization following it. I just wanted to bring it up just in case this is something that has been overlooked. PR-URL: nodejs/node#12993 Reviewed-By: Fedor Indutny <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Fixes: nodejs/node#12833 PR-URL: nodejs/node#12957 Reviewed-By: João Reis <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]>

PR-URL: nodejs/node#13012 Reviewed-By: Jeremiah Senkpiel <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

PR-URL: nodejs/node#13113 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]>

PR-URL: nodejs/node#13118 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Use `file` as name of the argument, as the CLI documentation does. PR-URL: nodejs/node#13120 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Anna Henningsen <[email protected]>

This option has been broken for almost a year when used with any of the vm.runIn.. family of functions, except for syntax errors. PR-URL: nodejs/node#13074 Reviewed-By: Anna Henningsen <[email protected]>

This fixes a race condition in the watchdog timer used for vm timeouts. The condition would terminate the main stack's execution instead of the code running under the sandbox. PR-URL: nodejs/node#13074 Reviewed-By: Anna Henningsen <[email protected]>

Fix mismatch in title for napi_get_value_string_utf16 Fixes: nodejs/abi-stable-node#243 PR-URL: nodejs/node#13123 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Add tests to validate that properties marked as static are available through the class as opposed to instances PR-URL: nodejs/node#13124 Reviewed-By: Jason Ginchereau <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Hitesh Kanwathirtha <[email protected]>

When a no-op message event handler is used in a test, make it clear what is expected by using `common.mustCall()` and `common.mustNotCall()`. PR-URL: nodejs/node#13125 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

Use common.mustCall() in test-fs-makeStatsCallback to confirm that the callback is invoked. PR-URL: nodejs/node#13132 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Alexey Orlenko <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>

This commit fixes an incorrect keyboard shortcut in `doc/STYLE_GUIDE.md`: entering em-dashes is done via Alt+Shift+"-" on macOS, not via Ctrl+Alt+"-". Besides that, Option is more canonical name of Alt on Macs. PR-URL: nodejs/node#13134 Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Daijiro Wachi <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

* Wrap text at 80 characters. * Use periods consistently. PR-URL: nodejs/node#13135 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Rich Trott <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]>

PR-URL: nodejs/node#13138 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

PR-URL: nodejs/node#13145 Refs: http://eslint.org/docs/rules/no-useless-constructor Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Daniel Bevenius <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: James M Snell <[email protected]>

Fixed a regression that caused the callback passed to destroy() to not be called if the stream was already destroyed. This caused a regression on the ws module in CITGM introduced by nodejs/node#12925. PR-URL: nodejs/node#13156 Fixes: websockets/ws#1118 Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Calvin Metcalf <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

In test/pummel/test-stream2-basic.js, check that noop functions are called the expected number of times. PR-URL: nodejs/node#13146 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

PR-URL: nodejs/node#13146 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

test-stream2-basic runs in a few seconds. It can be moved to parallel. PR-URL: nodejs/node#13146 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Colin Ihrig <[email protected]>

Covert lib/dgram.js over to using lib/internal/errors.js for generating Errors. See [using-internal-errors.md](https://github.com/nodejs/node/blob/master/doc/guides/using-internal-errors.md) for more details. I have not addressed the cases that use errnoException() and exceptionWithHostPort() helper methods as changing these would require fixing the tests across all of the different files that use them. In addition, these helpers already add a `code` to the Error and we'll have to discuss how that interacts with the `code` used by lib/internal/errors.js. I believe we should convert all users of errnoException and exceptionWithHostPort in a PR dedicated to that conversion. PR-URL: nodejs/node#12926 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Franziska Hinkelmann <[email protected]> Reviewed-By: Ruben Bridgewater <[email protected]>

Adds the ability to for write streams to have an _final method which acts similarly to the _flush method that transform streams have but is called before the finish event is emitted and if asynchronous delays the stream from finishing. The `final` option may also be passed in order to set it. PR-URL: nodejs/node#12828 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Matteo Collina <[email protected]> Reviewed-By: Refael Ackermann <[email protected]>

Really minor but I could not find an open PR for anything n-api where this could be changed, so creating this so that it is not forgotten. PR-URL: nodejs/node#13190 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Colin Ihrig <[email protected]> Reviewed-By: Michael Dawson <[email protected]> Reviewed-By: James M Snell <[email protected]>

jasongin · 2017-05-25T19:05:38Z

In a969132, can you explain "test was failing because of over specifiying a not condition"?

And is this failure from a new test that was added with the merge, or a test that was already existing (and previously passing) in Node-ChakraCore?

kunalspathak · 2017-05-25T19:10:17Z

@jasongin - The failure came when new coverage was added in nodejs/node@47919b3 . The bug was in this line. The condition should be just if ((properties[i].attributes & napi_static) != 0)

jasongin · 2017-05-25T19:12:45Z

OK thanks. That looks fine then. Though for clarity, the component prefix on that commit message should probably be n-api, not test. I think the guidance is that the test prefix is when only test code is changed.

kunalspathak · 2017-05-25T19:14:49Z

Yes, i think i will just separate out that change in a different commit when i merge.

kfarnung · 2017-05-25T20:50:42Z

Should a969132 also have "src" in the tags?

Merge commit 260cd41 as of 2017-05-24 PR-URL: nodejs#264 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>

* Napi test was failing because of over specifiying a not condition. Fixed that and also restructured the code to not iterate over list of properties twice. * Reintroduced a condition for debugger that got removed in previous merge. PR-URL: nodejs#264 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>

* test-vm-timeout-rethrow: This started failing after nodejs/node#13074/ changes upstream. The change is to not do `GetAndClearException` but allocate a string (`JsCreateString`) which throws because engine is in exception state. We need to figure out better way to handle these situation. Marked this as flaky for now. * test-bindings: Marked this as flaky for now. PR-URL: nodejs#264 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>

kunalspathak · 2017-05-25T21:36:03Z

Thanks @kfarnung and @jasongin for the review.

kunalspathak · 2017-05-25T21:37:29Z

CI - ~~https://ci.nodejs.org/job/chakracore-test-pull-request/54/console~~
https://ci.nodejs.org/job/chakracore-test-pull-request/55/

* test-vm-timeout-rethrow: This started failing after nodejs/node#13074/ changes upstream. The change is to not do `GetAndClearException` but allocate a string (`JsCreateString`) which throws because engine is in exception state. We need to figure out better way to handle these situation. Marked below test as flaky. * parallel/test-vm-sigint : Marked this as flaky for now. * parallel/test-vm-sigint-existing-handler : Marked this as flaky for now. * parallel/test-repl-sigint : Marked this as flaky for now. * parallel/test-bindings: Marked this as flaky for now. PR-URL: nodejs#264 Reviewed-By: Kyle Farnung <[email protected]> Reviewed-By: Jason Ginchereau <[email protected]>