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.

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

test: fix flaky test-https-agent-additional-options #27830

Closed
wants to merge 4 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott 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-bot nodejs-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.

@Trott Trott 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
Member Author

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
Member Author

Trott commented May 23, 2019

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

@nodejs-github-bot
Copy link
Collaborator

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

Trott commented May 24, 2019

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

@Trott Trott 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 <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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#27830
Fixes: nodejs#24449
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@Trott
Copy link
Member Author

Trott commented May 25, 2019

Landed in 5b8df5e...0d28300

@Trott Trott 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 <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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 <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
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: #27830
Fixes: #24449
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@targos targos mentioned this pull request Jun 3, 2019
@Trott Trott deleted the add-opt branch January 13, 2022 22:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants