Skip to content

test : refactor parallel/test-tls-async-cb-after-socket-end - #18985

Closed
juggernaut451 wants to merge 1 commit into
nodejs:masterfrom
juggernaut451:refactorTestTlsAsyncCbAfterSocketEnd
Closed

test : refactor parallel/test-tls-async-cb-after-socket-end#18985
juggernaut451 wants to merge 1 commit into
nodejs:masterfrom
juggernaut451:refactorTestTlsAsyncCbAfterSocketEnd

Conversation

@juggernaut451

@juggernaut451juggernaut451 commented Feb 25, 2018

Copy link
Copy Markdown
Contributor

this replaces the function with arrow function, callback by common.mustCall
and added the description

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

test

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label Feb 25, 2018
richardlau
richardlau reviewed Feb 25, 2018
View reviewed changes
Comment threadtest/parallel/test-tls-async-cb-after-socket-end.js Outdated

@richardlaurichardlauFeb 25, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Since the callback is empty it can be omitted from common.mustCall().

@juggernaut451juggernaut451Feb 26, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

@richardlaurichardlauFeb 26, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I meant that this could be just common.mustCall().

@juggernaut451juggernaut451Feb 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

sorry wrong interpreted

@juggernaut451juggernaut451Feb 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

change made :)

BridgeAR
BridgeAR reviewed Feb 27, 2018
View reviewed changes
Comment threadtest/parallel/test-tls-async-cb-after-socket-end.js Outdated

@BridgeARBridgeARFeb 27, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Just so it is not overlooked: #18985 (comment)

@juggernaut451juggernaut451Feb 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixing it

@juggernaut451juggernaut451Feb 28, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed :) Thanks for pointing out

BridgeAR
BridgeAR approved these changes Mar 1, 2018
View reviewed changes
Comment threadtest/parallel/test-tls-async-cb-after-socket-end.js Outdated

@BridgeARBridgeARMar 1, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: it would be great to remove this line again.

@juggernaut451juggernaut451Mar 2, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed :)

davisokoth
davisokoth approved these changes Mar 15, 2018
View reviewed changes

@davisokothdavisokoth left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code looks good. Kindly consider squashing this into one commit (https://github.com/nodejs/node/blob/master/doc/guides/contributing/pull-requests.md#commit-squashing)

@fhinkel

fhinkel commented Mar 15, 2018

Copy link
Copy Markdown
Member

LGTM with @davisokoth 's comment about squashing.

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

@juggernaut451
juggernaut451force-pushed the refactorTestTlsAsyncCbAfterSocketEnd branch from c4ce57a to 855826fCompareMarch 17, 2018 19:05
@juggernaut451

juggernaut451 commented Mar 17, 2018

Copy link
Copy Markdown
ContributorAuthor

change made :)

@trivikr

trivikr commented Mar 26, 2018

Copy link
Copy Markdown
Member

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

@trivikr

trivikr commented Mar 26, 2018

Copy link
Copy Markdown
Member

CI is failing for node-test-commit-linux in debian8-x86/debian8-64
Details: https://ci.nodejs.org/job/node-test-commit-linux/17355/

@trivikr

trivikr commented Mar 26, 2018

Copy link
Copy Markdown
Member

node-test-commit-linux was rerun, failed again for debian8-64https://ci.nodejs.org/job/node-test-commit-linux/17356/
It might be a flaky test

@BridgeAR

BridgeAR commented Apr 9, 2018

Copy link
Copy Markdown
Member

Landed in d1156da

@BridgeARBridgeAR closed this Apr 9, 2018
BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Apr 9, 2018
@juggernaut451@BridgeAR
test: refactor parallel/test-tls-async-cb-after-socket-end
d1156da
PR-URL: nodejs#18985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
targos pushed a commit that referenced this pull request Apr 12, 2018
@juggernaut451@targos
test: refactor parallel/test-tls-async-cb-after-socket-end
4eef82e
PR-URL: #18985
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Richard Lau <riclau@uk.ibm.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
@TrottTrott mentioned this pull request Apr 30, 2018
Closed
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes
@richardlaurichardlaurichardlau approved these changes
@BridgeARBridgeARBridgeAR approved these changes
+1 more reviewer
@davisokothdavisokothdavisokoth approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@juggernaut451@fhinkel@trivikr@BridgeAR@jasnell@richardlau@davisokoth@nodejs-github-bot