Skip to content
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: refactor test-debug-signal-cluster #8289

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 26, 2016

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test debugger

Description of change

Notable changes include removing one (but not all) hard-coded ports, using common.fail(), and tidying conditionals and assertions.

@Trott Trott added debugger test Issues and PRs related to the tests. labels Aug 26, 2016

const http = require('http');
const cluster = require('cluster');
const common = require('../../common');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make common the first require().

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a fixture file, common should probably be removed altogether!

@cjihrig
Copy link
Contributor

cjihrig commented Aug 27, 2016

LGTM with a couple comments.

@Trott
Copy link
Member Author

Trott commented Aug 27, 2016

Notable changes include removing one (but not all) hard-coded ports,
using `common.fail()`, and tidying conditionals and assertions.
@Trott Trott force-pushed the refactor-debug-signal-cluster branch from 6977be3 to c5fae55 Compare August 27, 2016 05:30
@Trott
Copy link
Member Author

Trott commented Aug 27, 2016

Unfortunate build failures in CI, but nothing related to the changes here. Using the opportunity to rebase. CI again: https://ci.nodejs.org/job/node-test-pull-request/3852/

@Trott
Copy link
Member Author

Trott commented Aug 27, 2016

@Trott
Copy link
Member Author

Trott commented Aug 28, 2016

(CI is green.)

@santigimeno
Copy link
Member

LGTM

1 similar comment
@jasnell
Copy link
Member

jasnell commented Aug 29, 2016

LGTM

Trott added a commit to Trott/io.js that referenced this pull request Aug 30, 2016
Notable changes include removing one (but not all) hard-coded ports,
using `common.fail()`, and tidying conditionals and assertions.

PR-URL: nodejs#8289
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@Trott
Copy link
Member Author

Trott commented Aug 30, 2016

Landed in e192825

@Trott Trott closed this Aug 30, 2016
@Fishrock123 Fishrock123 mentioned this pull request Sep 6, 2016
Fishrock123 pushed a commit to Fishrock123/node that referenced this pull request Sep 8, 2016
Notable changes include removing one (but not all) hard-coded ports,
using `common.fail()`, and tidying conditionals and assertions.

PR-URL: nodejs#8289
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

 Conflicts:
	test/parallel/test-debug-signal-cluster.js
Fishrock123 pushed a commit that referenced this pull request Sep 9, 2016
Notable changes include removing one (but not all) hard-coded ports,
using `common.fail()`, and tidying conditionals and assertions.

PR-URL: #8289
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

 Conflicts:
	test/parallel/test-debug-signal-cluster.js
MylesBorins pushed a commit that referenced this pull request Sep 30, 2016
Notable changes include removing one (but not all) hard-coded ports,
using `common.fail()`, and tidying conditionals and assertions.

PR-URL: #8289
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 10, 2016
Notable changes include removing one (but not all) hard-coded ports,
using `common.fail()`, and tidying conditionals and assertions.

PR-URL: #8289
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
rvagg pushed a commit that referenced this pull request Oct 18, 2016
Notable changes include removing one (but not all) hard-coded ports,
using `common.fail()`, and tidying conditionals and assertions.

PR-URL: #8289
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Oct 26, 2016
Notable changes include removing one (but not all) hard-coded ports,
using `common.fail()`, and tidying conditionals and assertions.

PR-URL: #8289
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Oct 26, 2016
@Trott Trott deleted the refactor-debug-signal-cluster branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants