Skip to content

Conversation

tarunbatra
Copy link
Contributor

@tarunbatra tarunbatra commented Apr 16, 2017

Refs: #12376
PR_URL: #12451

Tests updated:

  • test/parallel/test-cluster-shared-leak.js
  • test/parallel/test-cluster-master-error.js
  • test/parallel/test-cluster-master-kill.js
  • test/parallel/test-cluster-net-send.js
  • test/parallel/test-cluster-rr-domain-listen.js
  • test/parallel/test-cluster-rr-ref.js
  • test/parallel/test-cluster-worker-no-exit.js
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

test

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 16, 2017
Copy link
Member

@santigimeno santigimeno left a comment

Choose a reason for hiding this comment

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

It looks good. Just to be sure the test behaves as originally intended, I would check that it fails in nodejs version 4.2.1 on an OS other than linux as it's pointed out in the description of the test.

Copy link
Member

Choose a reason for hiding this comment

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

I would rename worker to address.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense. Changing.

@santigimeno santigimeno added the cluster Issues and PRs related to the cluster subsystem. label Apr 16, 2017
@tarunbatra
Copy link
Contributor Author

@santigimeno Yes I saw the comment and I'm curious too, but I don't have access to another OS.

Copy link
Member

@santigimeno santigimeno left a comment

Choose a reason for hiding this comment

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

@tarunbatra I can confirm it also fails as expected with the new version of the test using [email protected] on OS.X. LGTM

@tarunbatra
Copy link
Contributor Author

tarunbatra commented Apr 16, 2017

@santigimeno great. Thanx! I'll try to use similar pattern in other test files.

@tarunbatra
Copy link
Contributor Author

@santigimeno What should be preferred, more commits to this PR or a new PR for small changes to other files regarding the same issue #12376 ?

@santigimeno
Copy link
Member

@tarunbatra if the changes are similar I would group them, but as you prefer :).

Files changed:
 * test/parallel/test-cluster-master-error.js
 * test/parallel/test-cluster-master-kill.js
 * test/parallel/test-cluster-net-send.js
 * test/parallel/test-cluster-rr-domain-listen.js
 * test/parallel/test-cluster-rr-ref.js
 * test/parallel/test-cluster-worker-no-exit.js

Refs: #12376
PR_URL: #12451
@tarunbatra tarunbatra changed the title tests: remove common.PORT from shared leak test [WIP] tests: remove common.PORT from shared,master,rr tests Apr 16, 2017
@benjamingr
Copy link
Member

jasnell pushed a commit that referenced this pull request Apr 18, 2017
PR-URL: #12451
Ref: #12376
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Apr 18, 2017

Landed in 2e5188d

@jasnell jasnell closed this Apr 18, 2017
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
PR-URL: #12451
Ref: #12376
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
PR-URL: #12451
Ref: #12376
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
PR-URL: #12451
Ref: #12376
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants