Skip to content

test: refactor test-stream2-writable#10353

Closed
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:stream2-writable
Closed

test: refactor test-stream2-writable#10353
Trott wants to merge 1 commit into
nodejs:masterfrom
Trott:stream2-writable

Conversation

@Trott

@Trott Trott commented Dec 20, 2016

Copy link
Copy Markdown
Member
  • replace setTimeout() with setImmediate()
  • assert.equal() -> assert.strictEqual()
  • remove unused function arguments
  • normalize indentation
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test stream

Description of change
* replace setTimeout() with setImmediate()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

@Trott Trott added stream Issues and PRs related to the stream subsystem. test Issues and PRs related to the tests. labels Dec 20, 2016
@julianduque

julianduque commented Dec 20, 2016

Copy link
Copy Markdown
Contributor

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

mcollina
mcollina reviewed Dec 22, 2016
View reviewed changes
Comment thread test/parallel/test-stream2-writable.js Outdated

@mcollina mcollina Dec 22, 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.

there is a slight difference between setTimeout and setImmediate, given where they are executed in the event loop. If it is indifferent for the test, e.g. even process.nextTick is fine, then add it as a comment.

@Trott Trott Dec 22, 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.

Reverting to setTimeout() just to be 100% sure we're not modifying the test behavior in a way that might undermine the purpose of the test.

@Fishrock123 Fishrock123 Dec 22, 2016

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If you do process.nextTick(process.nextTick(setImmediate( you should virtually guarantee adding it to the end of the next processing of immediates, fwiw.

mcollina
mcollina approved these changes Dec 22, 2016
View reviewed changes

@mcollina mcollina 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.

LGTM with a nit

@Trott
test: refactor test-stream2-writable
6805678 
* replace setTimeout() with setImmediate()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation
@Trott

Trott commented Dec 22, 2016

Copy link
Copy Markdown
Member Author

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

Trott added a commit to Trott/io.js that referenced this pull request Dec 23, 2016
@Trott
test: refactor test-stream2-writable
7472521 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: nodejs#10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@Trott

Trott commented Dec 23, 2016

Copy link
Copy Markdown
Member Author

Landed in 7472521.

@Trott Trott closed this Dec 23, 2016
@evanlucas evanlucas mentioned this pull request Jan 3, 2017
Merged
evanlucas pushed a commit that referenced this pull request Jan 3, 2017
@Trott @evanlucas
test: refactor test-stream2-writable
1a21a47 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: #10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
evanlucas pushed a commit that referenced this pull request Jan 4, 2017
@Trott @evanlucas
test: refactor test-stream2-writable
ba63363 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: #10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@Trott @MylesBorins
test: refactor test-stream2-writable
ffcf8e2 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: #10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 23, 2017
@Trott @MylesBorins
test: refactor test-stream2-writable
ce95ba6 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: #10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@Trott @MylesBorins
test: refactor test-stream2-writable
80aeca7 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: #10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@Trott @MylesBorins
test: refactor test-stream2-writable
8b050a3 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: #10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
This was referenced Jan 24, 2017
Merged
Merged
MylesBorins pushed a commit that referenced this pull request Jan 31, 2017
@Trott @MylesBorins
test: refactor test-stream2-writable
1b9c125 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: #10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@Trott @MylesBorins
test: refactor test-stream2-writable
76bb3cb 
* add duration to setTimeout()
* assert.equal() -> assert.strictEqual()
* remove unused function arguments
* normalize indentation

PR-URL: #10353
Reviewed-By: Julian Duque <julianduquej@gmail.com>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
@Trott Trott deleted the stream2-writable branch January 13, 2022 22:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

+2 more reviewers

@Fishrock123 Fishrock123 Fishrock123 left review comments

@julianduque julianduque julianduque 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.

6 participants

@Trott @julianduque @mcollina @Fishrock123 @MylesBorins @nodejs-github-bot