FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: remove timers from streams test by addaleax · Pull Request #9360 · nodejs/node · GitHub

/ node Public

test: remove timers from streams test - #9360

Closed
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:test-stream2-readable-empty-buffer-no-eof-no-timers
ConversationCommits (4)ChecksFiles changed
Closed

test: remove timers from streams test#9360
addaleax wants to merge 4 commits into
nodejs:masterfrom
addaleax:test-stream2-readable-empty-buffer-no-eof-no-timers

Conversation

addaleax commented Oct 29, 2016

Copy link
Copy Markdown
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: remove timers from streams test
b8ffd20 
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 added the test Issues and PRs related to the tests. label Oct 29, 2016
addaleax mentioned this pull request Oct 29, 2016
Closed
2 tasks
addaleax added the stream Issues and PRs related to the stream subsystem. label Oct 29, 2016

Trott commented Oct 29, 2016

Copy link
Copy Markdown
Member

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 commented Oct 29, 2016

Copy link
Copy Markdown
Member Author

@Trott done! :)

Trott reviewed Oct 29, 2016
View reviewed changes
Comment thread test/parallel/test-stream2-readable-empty-buffer-no-eof.js Outdated
case 4:
setTimeout(r.read.bind(r, 0), timeout);
return setTimeout(function() {
setImmediate(setImmediate, r.read.bind(r, 0));

Trott Oct 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Is the double setImmediate() here intentional?

addaleax Oct 29, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Trott Oct 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

They're guaranteed to execute in the order they are called so I don't think that inner setImmediate is needed.

Trott Oct 29, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Ah, but then the test fails. Hmmm... Not sure what's wrong.

Trott Oct 30, 2016 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Trott Oct 30, 2016

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

(Whoops, typo'ed in the code snippet, edited, above is what I meant.)

addaleax Oct 30, 2016

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@Trott Mh, I’ve switched to two single setImmediates with reverse order, that should be okay 👍

Trott commented Oct 29, 2016

Copy link
Copy Markdown
Member

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 commented Oct 29, 2016

Copy link
Copy Markdown
Member

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

addaleax commented Oct 30, 2016

Copy link
Copy Markdown
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 commented Oct 30, 2016

Copy link
Copy Markdown
Member Author

CI: https://ci.nodejs.org/job/node-test-commit/5869/

Trott approved these changes Oct 31, 2016
View reviewed changes

Trott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Trott commented Oct 31, 2016

Copy link
Copy Markdown
Member

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

imyller approved these changes Oct 31, 2016
View reviewed changes

imyller left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

Trott commented Nov 2, 2016

Copy link
Copy Markdown
Member

Landed in 45a716c

Trott closed this Nov 2, 2016
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

jasnell jasnell approved these changes
Trott Trott approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
imyller imyller approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL