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: remove dependency on node-weak #11239

Merged
merged 3 commits into from Feb 11, 2017

Conversation

Projects
None yet
5 participants
@bnoordhuis
Member

bnoordhuis commented Feb 8, 2017

Replace node-weak with a small hand-rolled add-on. We can now drop node-weak and nan, reducing the size of the source tree by about 750 kB (and the size of the tarball by about 150-300 kB.)

The first commit cleans up the tests, the second introduces the add-on, the third one drops nan and node-weak.

@richardlau

This comment has been minimized.

Show comment
Hide comment
@richardlau

richardlau Feb 8, 2017

Member

vcbuild.bat will also require changes.

Member

richardlau commented Feb 8, 2017

vcbuild.bat will also require changes.

@jasnell

jasnell approved these changes Feb 8, 2017

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 8, 2017

Member

@richardlau Right you are, updated.

Member

bnoordhuis commented Feb 8, 2017

@richardlau Right you are, updated.

@mscdex mscdex added the test label Feb 8, 2017

@richardlau

node-weak is also referenced in the LICENSE file via tools/license-builder.sh.

Show outdated Hide outdated test/gc/binding.js
try {
module.exports = require('./build/Release/binding').ongc;
} catch (e) {
module.exports = require('./build/Debug/binding').ongc;

This comment has been minimized.

@richardlau

richardlau Feb 8, 2017

Member

Is it correct to prefer a binding in Release (e.g. if tests are run with node_g)?

For example, build and test Release and then build and test Debug without cleaning in-between.

I don't think it's possible for the makefile to build the binding in debug mode (it always runs node-gyp with the Release node) -- vcbuild.bat runs node-gyp with "%config%\node".

@richardlau

richardlau Feb 8, 2017

Member

Is it correct to prefer a binding in Release (e.g. if tests are run with node_g)?

For example, build and test Release and then build and test Debug without cleaning in-between.

I don't think it's possible for the makefile to build the binding in debug mode (it always runs node-gyp with the Release node) -- vcbuild.bat runs node-gyp with "%config%\node".

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 9, 2017

Member

You're right that the build won't compile a debug version but it seems convenient for manual testing/debugging. What would you do instead?

@bnoordhuis

bnoordhuis Feb 9, 2017

Member

You're right that the build won't compile a debug version but it seems convenient for manual testing/debugging. What would you do instead?

This comment has been minimized.

@richardlau

richardlau Feb 9, 2017

Member

Try to load the version that matched the mode (release/debug) of the node process first and fallback to the other?

@richardlau

richardlau Feb 9, 2017

Member

Try to load the version that matched the mode (release/debug) of the node process first and fallback to the other?

This comment has been minimized.

@bnoordhuis

bnoordhuis Feb 9, 2017

Member

Not a bad idea but I decided to use the same logic we use in test/addons for the sake of consistency. PTAL.

@bnoordhuis

bnoordhuis Feb 9, 2017

Member

Not a bad idea but I decided to use the same logic we use in test/addons for the sake of consistency. PTAL.

@bnoordhuis

This comment has been minimized.

Show comment
Hide comment
@bnoordhuis

bnoordhuis Feb 9, 2017

Member

node-weak is also referenced in the LICENSE file via tools/license-builder.sh.

Funny, I didn't even realize I own the copyright. At any rate, removed!

Member

bnoordhuis commented Feb 9, 2017

node-weak is also referenced in the LICENSE file via tools/license-builder.sh.

Funny, I didn't even realize I own the copyright. At any rate, removed!

@bnoordhuis bnoordhuis referenced this pull request Feb 11, 2017

Closed

update licenses and remove v8_inspector deps license #11310

2 of 2 tasks complete

bnoordhuis added some commits Feb 8, 2017

test: don't call process.exit() in gc tests
Rewrite the tests in test/gc so that they no longer call process.exit().
Instead they exit gracefully now.

PR-URL: #11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
test: remove dependency on node-weak
Replace node-weak with a small hand-rolled add-on.  We can now drop
node-weak and nan, reducing the size of the source tree by about 750 kB
and the size of the tarball by about 150-300 kB.

PR-URL: #11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
test: remove nan + weak
Since the previous commit obsoleted them, remove them.

PR-URL: #11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

@bnoordhuis bnoordhuis closed this Feb 11, 2017

@bnoordhuis bnoordhuis deleted the bnoordhuis:cleanup-gc-tests branch Feb 11, 2017

@bnoordhuis bnoordhuis merged commit 75019df into nodejs:master Feb 11, 2017

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

test: don't call process.exit() in gc tests
Rewrite the tests in test/gc so that they no longer call process.exit().
Instead they exit gracefully now.

PR-URL: #11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

test: remove dependency on node-weak
Replace node-weak with a small hand-rolled add-on.  We can now drop
node-weak and nan, reducing the size of the source tree by about 750 kB
and the size of the tarball by about 150-300 kB.

PR-URL: #11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

test: remove nan + weak
Since the previous commit obsoleted them, remove them.

PR-URL: #11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

test: don't call process.exit() in gc tests
Rewrite the tests in test/gc so that they no longer call process.exit().
Instead they exit gracefully now.

PR-URL: nodejs#11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

test: remove dependency on node-weak
Replace node-weak with a small hand-rolled add-on.  We can now drop
node-weak and nan, reducing the size of the source tree by about 750 kB
and the size of the tarball by about 150-300 kB.

PR-URL: nodejs#11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

test: remove nan + weak
Since the previous commit obsoleted them, remove them.

PR-URL: nodejs#11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

test: don't call process.exit() in gc tests
Rewrite the tests in test/gc so that they no longer call process.exit().
Instead they exit gracefully now.

PR-URL: nodejs#11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

test: remove dependency on node-weak
Replace node-weak with a small hand-rolled add-on.  We can now drop
node-weak and nan, reducing the size of the source tree by about 750 kB
and the size of the tarball by about 150-300 kB.

PR-URL: nodejs#11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>

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

test: remove nan + weak
Since the previous commit obsoleted them, remove them.

PR-URL: nodejs#11239
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
@jasnell

This comment has been minimized.

Show comment
Hide comment
@jasnell

jasnell Mar 7, 2017

Member

needs backport PRs to land in v4 or v6

Member

jasnell commented Mar 7, 2017

needs backport PRs to land in v4 or v6

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment