Uh oh!
There was an error while loading. Please reload this page.
test: simplify the test cases in http timeouts mix - #43470
Conversation
F3n67u
commented
Jun 18, 2022
Nit: could you change the pr title to |
aduh95
left a comment
There was a problem hiding this comment.
We want to be able to catch never settling promises.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
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 |
ShogunPanda
commented
Jun 27, 2022
Yes, that's on my TODO list. I'll do it very soon! |
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'); |
Uh oh!
There was an error while loading. Please reload this page.
F3n67u
commented
Jun 28, 2022
Hi @ShogunPanda. I mark this test as flaky. We could remove this mark when we fixed this flaky problem. #43597 |
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