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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 0 commits into from
Closed

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

wants to merge 0 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Nov 20, 2019

  • 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 nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 20, 2019
@nodejs-github-bot
Copy link
Collaborator

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

addaleax
addaleax approved these changes Nov 20, 2019
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment •
edited
Loading

Choose a reason for hiding this comment

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

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

test/parallel/test-stream-writable-clear-buffer.js Outdated Show resolved Hide resolved
@Trott
Copy link
Member Author

Trott commented Nov 20, 2019

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.

@nodejs-github-bot
Copy link
Collaborator

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 20, 2019
@nodejs-github-bot
Copy link
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
@Trott
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
@Trott
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
@Trott
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
@Trott
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
@Trott
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
@Trott
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
Copy link
Member Author

Trott commented Nov 22, 2019

Landed in b703218...19e44ef

@Trott Trott closed this Nov 22, 2019
@Trott Trott deleted the refactor-stream-test branch November 22, 2019 19:44
addaleax pushed a commit that referenced this pull request Nov 30, 2019
@Trott @addaleax
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
@Trott @addaleax
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
@Trott @addaleax
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
@Trott @addaleax
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
@Trott @addaleax
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
@Trott @addaleax
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
@Trott @targos
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
@Trott @targos
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
@Trott @targos
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
@Trott @targos
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
@Trott @targos
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
@Trott @targos
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 BridgeAR mentioned this pull request Dec 3, 2019
Merged
@BethGriggs BethGriggs mentioned this pull request Dec 9, 2019
Closed
MylesBorins pushed a commit that referenced this pull request Dec 17, 2019
@Trott @MylesBorins
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
@Trott @MylesBorins
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
@Trott @MylesBorins
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
@Trott @MylesBorins
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
@Trott @MylesBorins
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
@Trott @MylesBorins
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 BethGriggs mentioned this pull request Dec 23, 2019
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@addaleax addaleax addaleax approved these changes

@lundibundi lundibundi lundibundi approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, 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
@Trott @nodejs-github-bot @addaleax @lundibundi