-
Notifications
You must be signed in to change notification settings - Fork 29.7k
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: remove timers from streams test #9360
test: remove timers from streams test #9360
Conversation
test-stream2-readable-empty-buffer-no-eof fails on resource-constrained machines due to its use of timers. Removing timers makes it more reliable and doesn’t affect the validity of the test, as it only uses relative timing relations. Failures were noticed on freebsd10-64 in CI. I am able to replicate the failure with `tools/test.py --repeat=100 -j 100`. When run alone, it passes reliably.
Tiniest of nits: The comment near the top of the test refers to "set the timeouts to call r.read(0)". Might change it to something like "use |
@Trott done! :) |
return process.nextTick(function() { | ||
return r.push(Buffer.alloc(0)); | ||
}); | ||
case 4: | ||
setTimeout(r.read.bind(r, 0), timeout); | ||
return setTimeout(function() { | ||
setImmediate(setImmediate, r.read.bind(r, 0)); |
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.
Is the double setImmediate()
here intentional?
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.
@Trott yes, it’s so that this one still gets executed after the one below (I think that appropriately maps the timeout-vs-no-timeout relation the timers had before?)
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.
They're guaranteed to execute in the order they are called so I don't think that inner setImmediate
is needed.
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.
Ah, but then the test fails. Hmmm... Not sure what's wrong.
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.
Oh, I misread your comment. Urp. Yeah, I wonder if the thing to do is just swap them like this:
const rv = setImmediate(function() {
return r.push(Buffer.alloc(0));
});
setImmediate(r.read.bind(r, 0));
return rv;
Easier to read/understand and same effect?
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.
(Whoops, typo'ed in the code snippet, edited, above is what I meant.)
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.
@Trott Mh, I’ve switched to two single setImmediate
s with reverse order, that should be okay 👍
Another super tiny nit, totally ignore if it's just tedious and low value in your opinion: I think this test would be easier to understand if the cases were reversed, since |
The assignment of |
No no, you’re right. I’ve reversed them. |
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
All CI issues in that last run are infra-related. Running again: https://ci.nodejs.org/job/node-test-pull-request/4751/ |
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
Landed in 45a716c |
Checklist
make -j8 test
(UNIX), orvcbuild test nosign
(Windows) passesAffected core subsystem(s)
test stream
Description of change
This is a proposed alternative to #9359, and I’ve taken the liberty to basically use @Trott’s commit message from there:
test-stream2-readable-empty-buffer-no-eof fails on resource-constrained
machines due to its use of timers. Removing timers makes it more
reliable and doesn’t affect the validity of the test, as it only uses
relative timing relations.
Failures were noticed on freebsd10-64 in CI. I am able to replicate the
failure with
tools/test.py --repeat=100 -j 100
. When run alone, itpasses reliably.