Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Jump to bottom

test: fix flaky test-https-server-close- tests #43216

Merged
merged 1 commit into from
Jun 19, 2022
Merged

test: fix flaky test-https-server-close- tests #43216

merged 1 commit into from
Jun 19, 2022

Conversation

santigimeno
Copy link
Member

Don't initiate the second connection until the first has been
established.

Refs: nodejs/reliability#289

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels May 26, 2022
@santigimeno santigimeno force-pushed the santi/fix_test_https_close_idle branch from f3f6774 to 9f2d594 Compare May 26, 2022 18:31
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label May 28, 2022
@nodejs-github-bot
Copy link
Collaborator

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

This was referenced May 29, 2022
Open
Open
Open
Open
Open
Open
Open
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 10, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

lpinca
lpinca reviewed Jun 12, 2022
View reviewed changes
test/parallel/test-https-server-close-idle.js Outdated
client1Closed = true;
}));
client1.on('connect', common.mustCall(() => {
assert.strictEqual(connections, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
assert.strictEqual(connections, 1);

It seems that the 'connect' event can be emitted before the 'connection' event on the server. I think we can remove this as the second connection is initiated from the listener of this event anyway.

Copy link
Member

@lpinca lpinca Jun 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@santigimeno are you ok with this? I would like to see if CI is happy after this small change.

Copy link
Member Author

Choose a reason for hiding this comment

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

@lpinca sure thing. Just rebased and pushed with that change. Sorry for the delay. Thanks!

This was referenced Jun 13, 2022
Open
Open
@santigimeno
test: fix flaky test-https-server-close- tests
c0c4661 
Don't initiate the second connection until the first has been
established.

Refs: nodejs/reliability#289
@santigimeno santigimeno force-pushed the santi/fix_test_https_close_idle branch from 9f2d594 to c0c4661 Compare June 17, 2022 13:11
@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@santigimeno santigimeno added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 17, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 18, 2022
Open
34 tasks
@F3n67u F3n67u mentioned this pull request Jun 18, 2022
Open
@F3n67u
Copy link
Member

F3n67u commented Jun 18, 2022 •
edited
Loading

@santigimeno test-http-server-close-idle is also flaky. I filed a issue at #43466. Could this pr fix test-http-server-close-idle flaky and close #43466?

@F3n67u F3n67u mentioned this pull request Jun 18, 2022
Closed
@santigimeno santigimeno changed the title test: fix flaky test-https-server-close-idle test: fix flaky test-https-server-close- tests Jun 18, 2022
@santigimeno
Copy link
Member Author

@santigimeno test-http-server-close-idle is also flaky. I filed a issue at #43466. Could this pr fix test-http-server-close-idle flaky and close #43466?

Yes, this PR fixes 4 very similar flaky tests included the one you mention. The PR title is confusing, I'll change it.

@F3n67u
Copy link
Member

F3n67u commented Jun 18, 2022

@santigimeno test-http-server-close-idle is also flaky. I filed a issue at #43466. Could this pr fix test-http-server-close-idle flaky and close #43466?

Yes, this PR fixes 4 very similar flaky tests included the one you mention. The PR title is confusing, I'll change it.

Great work!

@F3n67u F3n67u added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 18, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@F3n67u F3n67u added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Jun 18, 2022
@github-actions github-actions bot mentioned this pull request Jun 19, 2022
Open
30 tasks
@F3n67u
Copy link
Member

F3n67u commented Jun 19, 2022

Could we land this pr now? Those flaky tests often fail on my pr.

@F3n67u F3n67u added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2022
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 19, 2022
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 19, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 19, 2022
@nodejs-github-bot nodejs-github-bot merged commit f8f2772 into nodejs:main Jun 19, 2022
@nodejs-github-bot
Copy link
Collaborator

Landed in f8f2772

This was referenced Jun 20, 2022
Open
Open
Open
Open
targos pushed a commit that referenced this pull request Jul 12, 2022
@santigimeno @targos
test: fix flaky test-https-server-close- tests
1022c0d 
Don't initiate the second connection until the first has been
established.

Refs: nodejs/reliability#289

PR-URL: #43216
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@targos targos mentioned this pull request Jul 12, 2022
Merged
# for free to join this conversation on GitHub. Already have an account? # to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
flaky-test Issues and PRs related to the tests with unstable failures on the CI. needs-ci PRs that need a full CI run. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@santigimeno @nodejs-github-bot @F3n67u @jasnell @lpinca @targos