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

Refactor test-stream-writable-clear-buffer by Trott · Pull Request #30561 · nodejs/node · GitHub

/ node Public

Refactor test-stream-writable-clear-buffer - #30561

Closed
Trott wants to merge 0 commit into
nodejs:masterfrom
Trott:refactor-stream-test
ConversationCommits (0)ChecksFiles changed
Closed

Refactor test-stream-writable-clear-buffer#30561
Trott wants to merge 0 commit into
nodejs:masterfrom
Trott:refactor-stream-test

Conversation

Trott commented Nov 20, 2019

Copy link
Copy Markdown
Member

  • replace setTimeout() with setImmediate()

    Replace setTimeout() with setImmediate() in
    test-stream-writable-clear-buffer. The test still fails in Node.js 8.6.0
    (if you comment out the common module, since it uses BigInts and those
    aren't supported in 8.6.0).

  • use arrow function for anonymous callback

  • remove literal as message in assert.strictEqual()

    This reveals the values that cause the assertion error, should it
    happen.

  • move from sequential to parallel

    I don't believe there's a reason test-stream-writable-clear-buffer needs
    to be in sequential. Move it to parallel.

I tried to make it fail locally in parallel with tools/test.py -j 96 --repeat 192 test/parallel/test-stream-writable-clear-buffer.js but it passed and ran very quickly.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 20, 2019

nodejs-github-bot commented Nov 20, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26792/

lundibundi approved these changes Nov 20, 2019
View reviewed changes
Comment thread test/parallel/test-stream-writable-clear-buffer.js Outdated
addaleax approved these changes Nov 20, 2019
View reviewed changes

addaleax left a comment •
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

This doesn’t fix flakiness, does it? (Just asking because I’d be surprised if the test was flaky in its previous form)

Comment thread test/parallel/test-stream-writable-clear-buffer.js Outdated

Trott commented Nov 20, 2019

Copy link
Copy Markdown
Member Author

This doesn’t fix flakiness, does it? (Just asking because I’d be surprised if the test was flaky in its previous form)

No, it does not fix flakiness to my knowledge. It's just some improvements and getting it moved into parallel.

Trott force-pushed the refactor-stream-test branch from affcba4 to e6d0107 Compare November 20, 2019 21:08

nodejs-github-bot commented Nov 20, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26796/

Trott added the author ready PRs that have at least one approval, no outstanding review comments, and a CI started. label Nov 20, 2019

nodejs-github-bot commented Nov 22, 2019

Copy link
Copy Markdown
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/26843/

Trott added a commit to Trott/io.js that referenced this pull request Nov 22, 2019
test: replace setTimeout with setImmediate in stream test
bce58fd 
Replace setTimeout() with setImmediate() in
test-stream-writable-clear-buffer. The test still fails in Node.js 8.6.0
(if you comment out the `common` module, since it uses BigInts and those
aren't supported in 8.6.0).

PR-URL: nodejs#30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Trott added a commit to Trott/io.js that referenced this pull request Nov 22, 2019
test: use arrow function for callback in stream test
ac43427 
PR-URL: nodejs#30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Trott added a commit to Trott/io.js that referenced this pull request Nov 22, 2019
test: remove string literal as message in strictEqual() in stream test
c13f8fc 
This reveals the values that cause the assertion error, should it
happen.

PR-URL: nodejs#30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Trott added a commit to Trott/io.js that referenced this pull request Nov 22, 2019
test: move stream test to parallel
110c25d 
I don't believe there's a reason test-stream-writable-clear-buffer needs
to be in sequential. Move it to parallel.

PR-URL: nodejs#30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Trott added a commit to Trott/io.js that referenced this pull request Nov 22, 2019
test: move explanatory comment to expected location in file
1a35db5 
Refs: nodejs#30561 (review)

PR-URL: nodejs#30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Trott added a commit to Trott/io.js that referenced this pull request Nov 22, 2019
test: add common.mustCall() to stream test
19e44ef 
Refs: https://github.com/nodejs/node/pull/30561/files#r348667256

PR-URL: nodejs#30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>

Trott commented Nov 22, 2019

Copy link
Copy Markdown
Member Author

Landed in b703218...19e44ef

Trott closed this Nov 22, 2019
Trott force-pushed the refactor-stream-test branch from da7be1d to 19e44ef Compare November 22, 2019 19:44
Trott deleted the refactor-stream-test branch November 22, 2019 19:44
addaleax pushed a commit that referenced this pull request Nov 30, 2019
test: replace setTimeout with setImmediate in stream test
e122d39 
Replace setTimeout() with setImmediate() in
test-stream-writable-clear-buffer. The test still fails in Node.js 8.6.0
(if you comment out the `common` module, since it uses BigInts and those
aren't supported in 8.6.0).

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
test: use arrow function for callback in stream test
ebba322 
PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
test: remove string literal as message in strictEqual() in stream test
103d01e 
This reveals the values that cause the assertion error, should it
happen.

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
test: move stream test to parallel
081b4e2 
I don't believe there's a reason test-stream-writable-clear-buffer needs
to be in sequential. Move it to parallel.

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
test: move explanatory comment to expected location in file
106235f 
Refs: #30561 (review)

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request Nov 30, 2019
test: add common.mustCall() to stream test
36671f9 
Refs: https://github.com/nodejs/node/pull/30561/files#r348667256

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Dec 1, 2019
test: replace setTimeout with setImmediate in stream test
0ce9a9c 
Replace setTimeout() with setImmediate() in
test-stream-writable-clear-buffer. The test still fails in Node.js 8.6.0
(if you comment out the `common` module, since it uses BigInts and those
aren't supported in 8.6.0).

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Dec 1, 2019
test: use arrow function for callback in stream test
dd07ad4 
PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Dec 1, 2019
test: remove string literal as message in strictEqual() in stream test
33de2c6 
This reveals the values that cause the assertion error, should it
happen.

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Dec 1, 2019
test: move stream test to parallel
b38740d 
I don't believe there's a reason test-stream-writable-clear-buffer needs
to be in sequential. Move it to parallel.

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Dec 1, 2019
test: move explanatory comment to expected location in file
4dd0f5a 
Refs: #30561 (review)

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
targos pushed a commit that referenced this pull request Dec 1, 2019
test: add common.mustCall() to stream test
c2a5da1 
Refs: https://github.com/nodejs/node/pull/30561/files#r348667256

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BridgeAR mentioned this pull request Dec 3, 2019
Merged
BethGriggs mentioned this pull request Dec 9, 2019
Closed
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
test: replace setTimeout with setImmediate in stream test
4a5f00c 
Replace setTimeout() with setImmediate() in
test-stream-writable-clear-buffer. The test still fails in Node.js 8.6.0
(if you comment out the `common` module, since it uses BigInts and those
aren't supported in 8.6.0).

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
test: use arrow function for callback in stream test
59c9592 
PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
test: remove string literal as message in strictEqual() in stream test
0903f67 
This reveals the values that cause the assertion error, should it
happen.

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
test: move stream test to parallel
8e36901 
I don't believe there's a reason test-stream-writable-clear-buffer needs
to be in sequential. Move it to parallel.

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
test: move explanatory comment to expected location in file
516bdaf 
Refs: #30561 (review)

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
test: add common.mustCall() to stream test
88bca0f 
Refs: https://github.com/nodejs/node/pull/30561/files#r348667256

PR-URL: #30561
Reviewed-By: Denys Otrishko <shishugi@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
BethGriggs mentioned this pull request Dec 23, 2019
Merged
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

addaleax addaleax approved these changes
+1 more reviewer
lundibundi lundibundi approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs that have at least one approval, no outstanding review comments, and a CI started. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL