Skip to content

Conversation

@nammn
Copy link

test: string to template literals, to include port

Usage of template literals to include the port if strictEqual fails.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

test: string to template literals, to include port test: string to template literals, to include port
@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 22, 2018
Copy link
Contributor

@apapirovskiapapirovski left a comment

Choose a reason for hiding this comment

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

Thanks for working on this. Some changes are needed as otherwise the linter will complain.

server.listen({
port: 0,
exclusive: true
exclusive: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: please remove the comma here since it's an unrelated change. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

The comma is still here after the update. As it's not related to this PR, could you please remove it?


worker2.on('message',function(port2){
assert.strictEqual(port2,port2|0,'second worker could not listen');
assert.strictEqual(port2,port2|0,`second worker could not listen on port ${port2}`);
Copy link
Contributor

Choose a reason for hiding this comment

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

This will fail the linter, unfortunately. Same above.

You could instead realign like so:

assert.strictEqual(port2,port2|0,`second worker could not listen on port ${port2}`);

Copy link
Contributor

Choose a reason for hiding this comment

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

The alternative would be to just remove the message altogether and instead show the default error message. That would also be fine with me.

Copy link
Member

Choose a reason for hiding this comment

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

I went ahead and removed it.

@Trott
Copy link
Member

@TrottTrott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 23, 2018
@nammn
Copy link
Author

Hey guys, do you have some tips for me to make the checks successful, as i was not changing much?

tniessen pushed a commit that referenced this pull request May 25, 2018
PR-URL: #20889 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@tniessen
Copy link
Member

The failures are unrelated, CI is a bit noisy currently. Landed in 76a1feb, thank you!

targos pushed a commit that referenced this pull request May 25, 2018
PR-URL: #20889 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Trivikram Kamat <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request May 29, 2018
@nammnnammn deleted the show-ports branch October 31, 2019 16:24
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.testIssues and PRs related to the tests.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants

@nammn@Trott@tniessen@apapirovski@jasnell@lpinca@trivikr@nodejs-github-bot