Skip to content

test: fix global.PORT in dgram-cluster-test-1#12491

Closed
zivkaziv wants to merge 2 commits into
nodejs:masterfrom
zivkaziv:fix-test-cluster-dgram
Closed

test: fix global.PORT in dgram-cluster-test-1#12491
zivkaziv wants to merge 2 commits into
nodejs:masterfrom
zivkaziv:fix-test-cluster-dgram

Conversation

@zivkaziv
Copy link
Copy Markdown

@zivkaziv zivkaziv commented Apr 18, 2017 •
edited
Loading

Hi,

This is my first attempt at a pull request here. Attempted to fix the issue referenced below.

Note that we pass the port from the cluster master to the workers via process.send.

Got some (administrative) help from @benjamingr #goodnessSquad

Refs: #12376

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)

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

benjamingr commented Apr 18, 2017

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

benjamingr
benjamingr approved these changes Apr 18, 2017
View reviewed changes
Copy link
Copy Markdown
Member

@benjamingr benjamingr left a comment

Choose a reason for hiding this comment

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

Changes LGTM.

@mscdex mscdex added cluster Issues and PRs related to the cluster subsystem. dgram Issues and PRs related to the dgram subsystem / UDP. labels Apr 18, 2017
@zivkaziv
Copy link
Copy Markdown
Author

zivkaziv commented Apr 18, 2017 •
edited
Loading

Hey Benji,
I saw that the linux test failed.
I go over the logs and didn't find the error of this test.

Ok, I found the error log
https://ci.nodejs.org/job/node-test-commit-linux/nodes=fedora22/9212/console
But this error doesn't seems to be related to my commit,
Any idea what it is?

Trott
Trott reviewed Apr 19, 2017
View reviewed changes
Comment thread test/parallel/test-cluster-dgram-1.js Outdated
if (common.isWindows) {
common.skip('dgram clustering is currently not supported ' +
'on windows.');
'on windows.');
Copy link
Copy Markdown
Member

@Trott Trott Apr 19, 2017

Choose a reason for hiding this comment

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

unrelated whitespace change?

Copy link
Copy Markdown
Author

@zivkaziv zivkaziv Apr 19, 2017

Choose a reason for hiding this comment

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

I'm apologize, It's not related.

Copy link
Copy Markdown
Member

@Trott Trott Apr 19, 2017

Choose a reason for hiding this comment

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

No worries. Thanks for taking the time to do this PR, and I hope the duplicate PR thing won't discourage you from doing more!

Copy link
Copy Markdown
Author

@zivkaziv zivkaziv Apr 19, 2017

Choose a reason for hiding this comment

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

Thanks again, I'm definitely planning to do more..

Copy link
Copy Markdown
Contributor

@refack refack Apr 19, 2017

Choose a reason for hiding this comment

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

ื”ื•ื ืžืชื›ื•ื•ืŸ ืฉื›ื“ืื™ ืœืชืงืŸ ืืช ื–ื” ๐Ÿ˜‰
ืืชื” ื™ื›ื•ืœ ืœืฉื ื•ืช ืืช ื”ืงื•ืžื™ื˜ ื”ืื—ืจื•ืŸ ื•ืขืฉื•ืช ืคื•ืจืก-ืคื•ืฉ, ืื• ืœื”ื•ืกื™ืฃ ืขื•ื“ ืงื•ืžื™ื˜.

Copy link
Copy Markdown
Member

@benjamingr benjamingr Apr 19, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@refack please stick to English in comments, thanks :) (so people are not excluded from the discussion). We have the Hebrew locale repo if you'd like to participate in translation and localization efforts.

Copy link
Copy Markdown
Author

@zivkaziv zivkaziv Apr 19, 2017

Choose a reason for hiding this comment

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

I'm fixing it now..
Thanks

Copy link
Copy Markdown
Contributor

@refack refack Apr 19, 2017

Choose a reason for hiding this comment

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

Hebrew re: #12491 (comment)

@Trott
Copy link
Copy Markdown
Member

Trott commented Apr 19, 2017

It looks like there's another PR that fixes the same issue that came in about 3 hours earlier. #12487

I'd leave this one open until in case that one doesn't land or this one is deemed superior for some reason.

As for the CI failure, it looks like a build issue unrelated to this change. I wouldn't worry about it.

@refack
Copy link
Copy Markdown
Contributor

refack commented Apr 19, 2017

ื‘ืจื•ืš ื”ื‘ื

@refack
Copy link
Copy Markdown
Contributor

refack commented Apr 19, 2017

[No secrets were shared in Hebrew ๐Ÿ˜‰(all Googleable), just me screaching an itch to communicate in the old tongue]

@zivkaziv
Copy link
Copy Markdown
Author

zivkaziv commented Apr 19, 2017

I've uploaded a fix for the whitespaces change..
Thanks @Trott for the review and @refack with the clarification:)

#goodnessSquad

thefourtheye
thefourtheye reviewed Apr 19, 2017
View reviewed changes
Comment thread test/parallel/test-cluster-dgram-1.js
s.bind(0, () => {
const port = s.address().port;
s.close();
cb(null, port);