-
Notifications
You must be signed in to change notification settings - Fork 29.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
test: fixed flaky test-http-readable-data-event #19931
Conversation
6e52b1b
to
e1e65cd
Compare
Flaky CI run on FreeBSD: https://ci.nodejs.org/job/node-stress-single-test/1813/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SGTM.
Can you please also fix this
it should be |
I've stopped and restarted the stress test to add the |
e1e65cd
to
4865576
Compare
@lpinca it was still flaky with the Started it again: https://ci.nodejs.org/job/node-stress-single-test/1815/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, even better now.
@@ -27,14 +32,15 @@ const server = http.createServer((req, res) => { | |||
const expectedRead = [helloWorld, null, helloAgainLater, null]; | |||
|
|||
const req = http.request(opts, (res) => { | |||
res.on('error', common.mustNotCall); | |||
res.on('error', common.mustNotCall()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
|
||
res.on('readable', common.mustCall(() => { | ||
let data; | ||
|
||
do { | ||
data = res.read(); | ||
assert.strictEqual(data, expectedRead.shift()); | ||
next(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to remove the next
and use a boolean to verify that res.end(helloAgainLater)
was not yet called. That should be simpler but does the same as far as I read the code.
// Here
if (!streamEnded) {
res.end(helloAgainLater);
streamEnded = true;
}
// At the top of the file
let streamEnded = false;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It’s not what the test is supposed to verify. The intent is that we need to wait for the first chunk to be received before sending the second.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The next
call can be moved after the loop but it's also ok as is.
@nodejs/testing @nodejs/http |
These changes fix the issue on my local machine. 👍 |
res.end(helloAgainLater); | ||
}, common.platformTimeout(10)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bonus points for making an instance of platformTimeout()
unnecessary.
The fast-track label was added, is it ok to land this now? |
Landed in 5a8fcd0 |
Fixes: #19905
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes