FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: simplify the test cases in http timeouts mix by MrJithil · Pull Request #43470 · nodejs/node · GitHub

/ node Public

test: simplify the test cases in http timeouts mix - #43470

Closed
MrJithil wants to merge 2 commits into
nodejs:mainfrom
MrJithil:fix-43465
ConversationCommits (2)ChecksFiles changed
Closed

test: simplify the test cases in http timeouts mix#43470
MrJithil wants to merge 2 commits into
nodejs:mainfrom
MrJithil:fix-43465

Conversation

MrJithil commented Jun 18, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Simplified the test cases which causing flaky #43465

No test cases removed.

Inspired from the video of @mhdawson - https://www.youtube.com/watch?v=5WtkRoGtbx4

nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Jun 18, 2022
F3n67u requested a review from ShogunPanda June 18, 2022 13:39
F3n67u added the flaky-test Issues and PRs related to the tests with unstable failures on the CI. label Jun 18, 2022

F3n67u commented Jun 18, 2022

Copy link
Copy Markdown
Contributor

Nit: could you change the pr title to test: simplify the test cases in http timeouts mix or some more specific title, please?

MrJithil changed the title Fix 43465 test: simplify the test cases in http timeouts mix Jun 18, 2022
aduh95 reviewed Jun 18, 2022
View reviewed changes

aduh95 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

We want to be able to catch never settling promises.

Comment thread test/parallel/test-http-server-request-timeouts-mixed.js Outdated
Comment thread test/parallel/test-http-server-request-timeouts-mixed.js Outdated
lpinca reviewed Jun 18, 2022
View reviewed changes
Comment thread test/parallel/test-http-server-request-timeouts-mixed.js Outdated

lpinca Jun 18, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

From my understanding this changes the original test. request3 and request4 are created after the requestTimeout expired.

MrJithil Jun 18, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Not exactly

lpinca Jun 18, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

request3 and request4 are created at request1 creation time + threadSleepDelay, where threadSleepDelay = requestTimeout + headersTimeout.

It's not like this in the original test. All request are created in a requestTimeout interval in the original one. There are 4 concurrent requests. Now there are 2.

MrJithil Jun 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Yes. But, basically we are testing 5 requests header timeout. In the previous pattern, we are doing it using some setTimeout functions and it was causing the flaky.
So, rewritten that to this way.

lpinca Jun 19, 2022 •
edited
Loading

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Each of these tests already has a standalone version. This one was made to test concurrent requests. If we have to change it like this, it is better to remove it completely as per https://github.com/nodejs/node/pull/42893/files#r861466882.

MrJithil Jun 19, 2022

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Agreed. Bu, as per the conversation, PR owner telling that he added this case to make concurrent requests and testing them together.

Previously, 5 concurrent requests are creating. Now, we are creating 2 concurrent requests at a time. Again, we are creating 3 concurrent requests.

This splitting is to avoid flakiness.

Instead of removing, I prefer to have some test for concurrency at least. I appreciate that you already forecasted the flakiness in #42893 . Good work.

lpinca Jun 19, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Sorry but I'm -1 to change the test like this. Again, this changes the original intentions.

ShogunPanda Jun 20, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Confirmed, this test should have 4 concurrent request to properly test the possible combinations.
We can revisit the timeout schedule (and eventually use async/await for readable, but please do not change the order of creation.

F3n67u commented Jun 27, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

This test is top 1 flaky test in all last week. If we could not make progress to fix this flaky test, so you mind mark this test as flaky? https://github.com/nodejs/reliability/issues

ShogunPanda commented Jun 27, 2022

Copy link
Copy Markdown
Contributor

Yes, that's on my TODO list. I'll do it very soon!

aduh95 approved these changes Jun 27, 2022
View reviewed changes

aduh95 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Note that there's no need to use util.promisify, Node.js ships a promisified version already.

Comment thread test/parallel/test-http-server-request-timeouts-mixed.js
const connectionsCheckingInterval = headersTimeout / 4;
const requestTimeout = headersTimeout * 2;
const threadSleepDelay = requestTimeout + headersTimeout;
const delay = promisify(setTimeout);

aduh95 Jun 27, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
const delay = promisify(setTimeout);

Comment thread test/parallel/test-http-server-request-timeouts-mixed.js
const assert = require('assert');
const { createServer } = require('http');
const { connect } = require('net');
const { promisify } = require('util');

aduh95 Jun 27, 2022

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality
Suggested change
const { promisify } = require('util');
const { setTimeout: delay } = require('timers/promises');

Comment thread test/parallel/test-http-server-request-timeouts-mixed.js Outdated
F3n67u mentioned this pull request Jun 28, 2022
Closed

F3n67u commented Jun 28, 2022

Copy link
Copy Markdown
Contributor

This test is top 1 flaky test in all last week. If we could not make progress to fix this flaky test, so you mind mark this test as flaky? https://github.com/nodejs/reliability/issues

Hi @ShogunPanda. I mark this test as flaky. We could remove this mark when we fixed this flaky problem. #43597

lpinca mentioned this pull request Aug 10, 2022
Closed
MrJithil closed this Oct 22, 2022
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

lpinca lpinca left review comments
aduh95 aduh95 approved these changes
ShogunPanda Awaiting requested review from ShogunPanda

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

Back | FazBrowse Home | New Git URL