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.
Is the double setImmediate() here intentional?
There was a problem hiding this comment.
@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.
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.
Ah, but then the test fails. Hmmm... Not sure what's wrong.
There was a problem hiding this comment.
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.
(Whoops, typo'ed in the code snippet, edited, above is what I meant.)
There was a problem hiding this comment.
@Trott Mh, I’ve switched to two single setImmediates 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. |
|
All CI issues in that last run are infra-related. Running again: https://ci.nodejs.org/job/node-test-pull-request/4751/ |
|
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.