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.

Sign up for GitHub

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

Jump to bottom

tests: remove common.PORT from shared,master,rr tests #12451

Closed
wants to merge 2 commits into from
Closed

tests: remove common.PORT from shared,master,rr tests #12451

wants to merge 2 commits into from

Conversation

tarunbatra
Copy link
Contributor

@tarunbatra tarunbatra commented Apr 16, 2017 •
edited
Loading

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
santigimeno
santigimeno reviewed Apr 16, 2017
View reviewed changes
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.

test/parallel/test-cluster-shared-leak.js Outdated
worker2 = cluster.fork();
worker2.on('online', function() {
conn = net.connect(common.PORT, common.mustCall(function() {
conn = net.connect(worker.port, common.mustCall(function() {
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.

santigimeno
santigimeno approved these changes Apr 16, 2017
View reviewed changes
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 nodejs@4.2.1 on OS.X. LGTM

@tarunbatra
Copy link
Contributor Author

tarunbatra commented Apr 16, 2017 •
edited
Loading

@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 :).

@tarunbatra
test: remove common.PORT from master,rr tests
9bb25ed 
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
@Trott Trott mentioned this pull request Apr 16, 2017
Closed
64 tasks
@benjamingr
Copy link
Member

CI: https://ci.nodejs.org/job/node-test-pull-request/7438/

jasnell pushed a commit that referenced this pull request Apr 18, 2017
@tarunbatra @jasnell
test: remove common.PORT from multiple tests
2e5188d 
PR-URL: #12451
Ref: #12376
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@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
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks
gibfahn pushed a commit that referenced this pull request Jun 18, 2017
@tarunbatra @gibfahn
test: remove common.PORT from multiple tests
6578884 
PR-URL: #12451
Ref: #12376
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
@tarunbatra @gibfahn
test: remove common.PORT from multiple tests
d5491fc 
PR-URL: #12451
Ref: #12376
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
@tarunbatra @MylesBorins
test: remove common.PORT from multiple tests
f2467ed 
PR-URL: #12451
Ref: #12376
Reviewed-By: Santiago Gimeno <santiago.gimeno@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@santigimeno santigimeno santigimeno approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
cluster Issues and PRs related to the cluster subsystem. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@tarunbatra @santigimeno @benjamingr @jasnell @lpinca @cjihrig @gibfahn @nodejs-github-bot