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: use common.crashOnUnhandledRejection #17215

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@sj82516
Contributor

sj82516 commented Nov 22, 2017

Add common.crashOnUnhandledRejection to test/parallel/test-microtask-queue-integration-domain.js

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)
@MylesBorins

LGTM

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@danbev

danbev approved these changes Nov 22, 2017

@@ -30,6 +30,9 @@ const assert = require('assert');
// removed.
require('domain');
// add crash handler for unhandled rejection
common.crashOnUnhandledRejection();

This comment has been minimized.

@danbev

danbev Nov 22, 2017

Member

I think common needs to be declared first:

const common = require('../common');
@danbev

danbev Nov 22, 2017

Member

I think common needs to be declared first:

const common = require('../common');
@gireeshpunathil

This comment has been minimized.

Show comment
Hide comment
@gireeshpunathil

gireeshpunathil Nov 22, 2017

Member

@sj82516 - your second commit brought about 94 files - accidental?

Member

gireeshpunathil commented Nov 22, 2017

@sj82516 - your second commit brought about 94 files - accidental?

@sj82516

This comment has been minimized.

Show comment
Hide comment
@sj82516

sj82516 Nov 22, 2017

Contributor

I run make -j4 test for test and then it installed the node modules.
I will remove the unnecessary part.
Sorry about that

Contributor

sj82516 commented Nov 22, 2017

I run make -j4 test for test and then it installed the node modules.
I will remove the unnecessary part.
Sorry about that

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Nov 22, 2017

Contributor

It seems a bunch of pre-existing node modules are now being removed?

Contributor

mscdex commented Nov 22, 2017

It seems a bunch of pre-existing node modules are now being removed?

@sj82516

This comment has been minimized.

Show comment
Hide comment
@sj82516

sj82516 Nov 22, 2017

Contributor

yes, I removed them.
Sorry for the accident

Contributor

sj82516 commented Nov 22, 2017

yes, I removed them.
Sorry for the accident

Removing my sign off until the extraneous commits can be cleaned up

@jasnell

This PR will need to be reworked to remove the extraneous edits

@sj82516

This comment has been minimized.

Show comment
Hide comment
@sj82516

sj82516 Nov 23, 2017

Contributor

I rebase the commit and push force again.
Is that correct now ?
Sorry for the first time PR to the nodejs project.

Contributor

sj82516 commented Nov 23, 2017

I rebase the commit and push force again.
Is that correct now ?
Sorry for the first time PR to the nodejs project.

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Nov 23, 2017

Contributor

Still the same problem. I think you will need to edit your commit since it seems the file removals are a part of the same commit as your actual (test) changes.

Contributor

mscdex commented Nov 23, 2017

Still the same problem. I think you will need to edit your commit since it seems the file removals are a part of the same commit as your actual (test) changes.

remove extra instructions.
reset the commit to remvoe extra instructions.
@sj82516

This comment has been minimized.

Show comment
Hide comment
@sj82516

sj82516 Nov 23, 2017

Contributor

I see.
So I would reset and rewrite the commit.
Should I push force to this PR or cancel this PR and create new one ?

Contributor

sj82516 commented Nov 23, 2017

I see.
So I would reset and rewrite the commit.
Should I push force to this PR or cancel this PR and create new one ?

@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 23, 2017

Member

@sj82516 you can always force push over the branch you made the PR with

Member

MylesBorins commented Nov 23, 2017

@sj82516 you can always force push over the branch you made the PR with

@mscdex

This comment has been minimized.

Show comment
Hide comment
@mscdex

mscdex Nov 23, 2017

Contributor

Looks better now.

Contributor

mscdex commented Nov 23, 2017

Looks better now.

@targos

targos approved these changes Nov 23, 2017

@targos

This comment has been minimized.

Show comment
Hide comment

MylesBorins added a commit that referenced this pull request Nov 26, 2017

test: use common.crashOnUnhandledRejection
PR-URL: #17215
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@MylesBorins

This comment has been minimized.

Show comment
Hide comment
@MylesBorins

MylesBorins Nov 26, 2017

Member

landed in 7518617 with some minor nits addressed

Member

MylesBorins commented Nov 26, 2017

landed in 7518617 with some minor nits addressed

MylesBorins added a commit that referenced this pull request Dec 12, 2017

test: use common.crashOnUnhandledRejection
PR-URL: #17215
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

MylesBorins added a commit that referenced this pull request Dec 12, 2017

test: use common.crashOnUnhandledRejection
PR-URL: #17215
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

@MylesBorins MylesBorins referenced this pull request Dec 12, 2017

Merged

v9.3.0 proposal #17631

gibfahn added a commit that referenced this pull request Dec 19, 2017

test: use common.crashOnUnhandledRejection
PR-URL: #17215
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

gibfahn added a commit that referenced this pull request Dec 19, 2017

test: use common.crashOnUnhandledRejection
PR-URL: #17215
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Closed

v8.9.4 proposal #17772

gibfahn added a commit that referenced this pull request Dec 20, 2017

test: use common.crashOnUnhandledRejection
PR-URL: #17215
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

@gibfahn gibfahn referenced this pull request Dec 20, 2017

Merged

v8.9.4 proposal #17774

@MylesBorins MylesBorins referenced this pull request Dec 20, 2017

Merged

v6.12.3 proposal #17776

msoechting added a commit to hpicgs/node that referenced this pull request Feb 5, 2018

test: use common.crashOnUnhandledRejection
PR-URL: nodejs#17215
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>

msoechting added a commit to hpicgs/node that referenced this pull request Feb 7, 2018

test: use common.crashOnUnhandledRejection
PR-URL: nodejs#17215
Reviewed-By: Myles Borins <myles.borins@gmail.com>
Reviewed-By: Daniel Bevenius <daniel.bevenius@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment