test: simplify the test cases in http timeouts mix#43470
test: simplify the test cases in http timeouts mix#43470MrJithil wants to merge 2 commits intonodejs:mainfrom
Conversation
nodejs-github-bot
added
needs-ci
F3n67u
added
the
flaky-test
|
Nit: could you change the pr title to |
MrJithil
changed the title
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
We want to be able to catch never settling promises.
|
|
||
| // Finish the first request now and wait more than the request timeout value | ||
| request1.client.write(requestBodyPart2 + requestBodyPart3); | ||
| await delay(threadSleepDelay); |
There was a problem hiding this comment.
From my understanding this changes the original test. request3 and request4 are created after the requestTimeout expired.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Sorry but I'm -1 to change the test like this. Again, this changes the original intentions.
There was a problem hiding this comment.
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.
|
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 |
|
Yes, that's on my TODO list. I'll do it very soon! |
aduh95
left a comment
aduh95
left a comment
There was a problem hiding this comment.
Note that there's no need to use util.promisify, Node.js ships a promisified version already.
| const connectionsCheckingInterval = headersTimeout / 4; | ||
| const requestTimeout = headersTimeout * 2; | ||
| const threadSleepDelay = requestTimeout + headersTimeout; | ||
| const delay = promisify(setTimeout); |
There was a problem hiding this comment.
| const delay = promisify(setTimeout); |
| const assert = require('assert'); | ||
| const { createServer } = require('http'); | ||
| const { connect } = require('net'); | ||
| const { promisify } = require('util'); |
There was a problem hiding this comment.
| const { promisify } = require('util'); | |
| const { setTimeout: delay } = require('timers/promises'); |
Hi @ShogunPanda. I mark this test as flaky. We could remove this mark when we fixed this flaky problem. #43597 |
6 participants
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