Skip to content
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

Conversation

addaleax
Copy link
Member

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected 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, it
passes reliably.

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.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 29, 2016
@addaleax addaleax added the stream Issues and PRs related to the stream subsystem. label Oct 29, 2016
@Trott
Copy link
Member

Trott commented Oct 29, 2016

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 setImmediate() to call r.read(0)"?

@addaleax
Copy link
Member Author

@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));
Copy link
Member

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?

Copy link
Member Author

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?)

Copy link
Member

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.

Copy link
Member

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.

Copy link
Member

@Trott Trott Oct 30, 2016

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?

Copy link
Member

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.)

Copy link
Member Author

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 setImmediates with reverse order, that should be okay 👍

@Trott
Copy link
Member

Trott commented Oct 29, 2016

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 5 gets called first and 0 last.

@Trott
Copy link
Member

Trott commented Oct 29, 2016

The assignment of common can be removed. (Linter is failing because of it--common doesn't get used.)

@addaleax
Copy link
Member Author

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 5 gets called first and 0 last.

No no, you’re right. I’ve reversed them.

@addaleax
Copy link
Member Author

Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott
Copy link
Member

Trott commented Oct 31, 2016

All CI issues in that last run are infra-related. Running again: https://ci.nodejs.org/job/node-test-pull-request/4751/

Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Trott
Copy link
Member

Trott commented Nov 2, 2016

Landed in 45a716c

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants