Uh oh!
There was an error while loading. Please reload this page.
test: use consistent timeouts - #42893
Conversation
CC: @nodejs/http |
nodejs-github-bot
commented
Apr 28, 2022
nodejs-github-bot
commented
Apr 28, 2022
| @@ -0,0 +1,133 @@ | |||
| 'use strict'; | |||
There was a problem hiding this comment.
Relying on timers is the most frequent source of flakiness in tests. Apart from doing it concurrently with multiple requests, does this test tests something not already tested in existing tests?
There was a problem hiding this comment.
I know, timers are messy. But unfortunately this test is about testing timers. :(
Yes, it checks that the concurrent checking of requestTimeout and headersTimeout works properly.
There was a problem hiding this comment.
My point is, are requestTimeout and headersTimeout already tested with a single request? If so what makes this test special? Why wouldn't the options work with multiple concurrent request if they work with a single one?
There was a problem hiding this comment.
Well, they should, but I thought adding a more complex "real world like" scenario would have helped for future regression testing.
Uh oh!
There was an error while loading. Please reload this page.
Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
ShogunPanda
commented
May 3, 2022
@lpinca Are we good on this PR? Can we land it? |
lpinca
commented
May 3, 2022
I have no objections. |
ShogunPanda
commented
May 3, 2022
@lpinca Do you mind leaving the second approval so I can proceed? |
nodejs-github-bot
commented
May 3, 2022
PR-URL: #42893 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
ShogunPanda
commented
May 3, 2022
Landed in c3aa86d |
PR-URL: #42893 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Co-authored-by: Luigi Pinca <luigipinca@gmail.com>
juanarbol
commented
May 31, 2022
Sadly, a test seems to be break ( test/parallel/test-http-server-request-timeouts-mixed.js) in V16.x: $ ./node test/parallel/test-http-server-request-timeouts-mixed.js
node:assert:123
throw new AssertionError(obj);
^
AssertionError [ERR_ASSERTION]: Expected values to be strictly equal:
60000 !== 2000
at Object.<anonymous> (/home/juanarbol/GitHub/node/test/parallel/test-http-server-request-timeouts-mixed.js:34:8)
at Module._compile (node:internal/modules/cjs/loader:1105:14)
at Object.Module._extensions..js (node:internal/modules/cjs/loader:1159:10)
at Module.load (node:internal/modules/cjs/loader:981:32)
at Function.Module._load (node:internal/modules/cjs/loader:822:12)
at Function.executeUserEntryPoint [as runMain] (node:internal/modules/run_main:77:12)
at node:internal/main/run_main_module:17:47 {
generatedMessage: true,
code: 'ERR_ASSERTION',
actual: 60000,
expected: 2000,
operator: 'strictEqual'**}** |
This PR enforces consistent timeouts for HTTP tests.