Skip to content

Conversation

@Trott
Copy link
Member

@TrottTrott commented May 22, 2019

test: fix test-https-agent-additional-options test-https-agent-additional-options can occasionally fail if a socket closes before the checks near the end of the test. Modify the test to check the freeSockets pool after each request. Fixes: https://github.com/nodejs/node/issues/24449 

test: refactor test-https-agent-additional-options Move callback to location where it is less confusing. 

test: favor arrow functions for anonymous callbacks In test-https-agent-additional-options, use arrow functions for anonymous callbacks. Replace a use of `this` with the relevant constant. 

test: replace flag with option test-https-agent-additional-options is invoked with a command-line flag. However, there is an equivalent option that can be enabled within the code. Do that instead. 
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

Trott added 3 commits May 21, 2019 18:49
test-https-agent-additional-options is invoked with a command-line flag. However, there is an equivalent option that can be enabled within the code. Do that instead.
In test-https-agent-additional-options, use arrow functions for anonymous callbacks. Replace a use of `this` with the relevant constant.
Move callback to location where it is less confusing.
@nodejs-github-bot
Copy link
Collaborator

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 22, 2019
@nodejs-github-bot
Copy link
Collaborator

@Trott

This comment has been minimized.

@TrottTrott added the wip Issues and PRs that are still a work in progress. label May 22, 2019
@Trott

This comment has been minimized.

@Trott

This comment has been minimized.

test-https-agent-additional-options can occasionally fail if a socket closes before the checks near the end of the test. Modify the test to check the freeSockets pool after each request. Fixes: nodejs#24449
@Trott

This comment has been minimized.

@Trott
Copy link
MemberAuthor

Trott commented May 23, 2019

Looks like the stress test rebuild didn't pick up the force-pushed changes. Let's try starting fresh:
https://ci.nodejs.org/job/node-stress-single-test/2217/

(And here's a stress test on master showing that it's flaky there: https://ci.nodejs.org/job/node-stress-single-test/2214/)

@Trott
Copy link
MemberAuthor

Hooray, that fixed it! Removing WIP label. /ping @nodejs/testing

@nodejs-github-bot
Copy link
Collaborator

@TrottTrott removed the wip Issues and PRs that are still a work in progress. label May 23, 2019
@TrottTrott changed the title fix flaky test-https-agent-additional-optionstest: fix flaky test-https-agent-additional-optionsMay 23, 2019
@TrottTrott added the review wanted PRs that need reviews. label May 24, 2019
@Trott
Copy link
MemberAuthor

@nodejs/collaborators This could use a review or two. Thanks!

@TrottTrott added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. and removed review wanted PRs that need reviews. labels May 24, 2019
Trott added a commit to Trott/io.js that referenced this pull request May 25, 2019
test-https-agent-additional-options is invoked with a command-line flag. However, there is an equivalent option that can be enabled within the code. Do that instead. PR-URL: nodejs#27830 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request May 25, 2019
In test-https-agent-additional-options, use arrow functions for anonymous callbacks. Replace a use of `this` with the relevant constant. PR-URL: nodejs#27830 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request May 25, 2019
Move callback to location where it is less confusing. PR-URL: nodejs#27830 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request May 25, 2019
test-https-agent-additional-options can occasionally fail if a socket closes before the checks near the end of the test. Modify the test to check the freeSockets pool after each request. PR-URL: nodejs#27830Fixes: nodejs#24449 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@Trott
Copy link
MemberAuthor

Landed in 5b8df5e...0d28300

@TrottTrott closed this May 25, 2019
targos pushed a commit that referenced this pull request May 28, 2019
test-https-agent-additional-options is invoked with a command-line flag. However, there is an equivalent option that can be enabled within the code. Do that instead. PR-URL: #27830 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request May 28, 2019
In test-https-agent-additional-options, use arrow functions for anonymous callbacks. Replace a use of `this` with the relevant constant. PR-URL: #27830 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request May 28, 2019
Move callback to location where it is less confusing. PR-URL: #27830 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request May 28, 2019
test-https-agent-additional-options can occasionally fail if a socket closes before the checks near the end of the test. Modify the test to check the freeSockets pool after each request. PR-URL: #27830Fixes: #24449 Reviewed-By: Sam Roberts <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@targostargos mentioned this pull request Jun 3, 2019
@TrottTrott deleted the add-opt branch January 13, 2022 22:51
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.

4 participants

@Trott@nodejs-github-bot@sam-github@lpinca