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

test: refactor test-stream2-writable #10353

Closed
wants to merge 1 commit into from
Closed

test: refactor test-stream2-writable #10353

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Dec 20, 2016

  • 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
Copy link
Contributor

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

mcollina
mcollina reviewed Dec 22, 2016
View reviewed changes
test/parallel/test-stream2-writable.js
@@ -328,7 +328,7 @@ test('dont end while writing', function(t) {
assert(!this.writing);
wrote = true;
this.writing = true;
setTimeout(function() {
setImmediate(function() {
Copy link
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.

Copy link
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.

Copy link
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
Copy link
Member

@mcollina mcollina left a comment

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
Copy link
Member Author

Trott commented Dec 22, 2016

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
Copy link
Member Author

Trott commented Dec 23, 2016

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

@Fishrock123 Fishrock123 Fishrock123 left review comments

@julianduque julianduque julianduque approved these changes

@mcollina mcollina mcollina approved these changes

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