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: cleanup stream tests #8668

Closed
wants to merge 8 commits into from
Closed

test: cleanup stream tests #8668

wants to merge 8 commits into from

Conversation

italoacasas
Copy link
Contributor

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • stream
Description of change
  • const and let instead var
  • assert.strictEqual instead assert.equal

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Sep 20, 2016
@mscdex mscdex added the stream Issues and PRs related to the stream subsystem. label Sep 20, 2016
lpinca
lpinca requested changes Sep 20, 2016
View reviewed changes
Copy link
Member

@lpinca lpinca left a comment

Choose a reason for hiding this comment

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

Just a minor style issue, otherwise LGTM.

test/parallel/test-stream-big-packet.js
@@ -1,10 +1,12 @@
'use strict';

Copy link
Member

Choose a reason for hiding this comment

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

Nit: don't add this line here and in the other files.

jasnell
jasnell approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@jasnell jasnell 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 the nits @lpinca reported fixed.

@imyller imyller changed the title cleanup stream tests test: cleanup stream tests Sep 20, 2016
targos
targos approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

LGTM

lpinca
lpinca reviewed Sep 20, 2016
View reviewed changes
test/parallel/test-stream-ispaused.js
@@ -1,10 +1,11 @@
'use strict';

Copy link
Member

Choose a reason for hiding this comment

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

@italoacasas I think you missed this one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

imyller
imyller approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@imyller imyller left a comment

Choose a reason for hiding this comment

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

LGTM

@imyller
Copy link
Member

imyller commented Sep 20, 2016

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

@lpinca
Copy link
Member

lpinca commented Sep 21, 2016

LGTM

@imyller imyller assigned imyller and unassigned imyller Sep 21, 2016
@imyller
Copy link
Member

imyller commented Sep 22, 2016

I'll start landing this:

  • Four LGTMs
  • No objections
  • All requested changes have been made
  • CI passed (only the usual CI failures)

imyller pushed a commit to imyller/node that referenced this pull request Sep 22, 2016
@italoacasas @imyller
test: cleanup stream tests
4d95872 
const and let instead var
assert.strictEqual instead assert.equal

PR-URL: nodejs#8668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@imyller
Copy link
Member

imyller commented Sep 22, 2016

landed in 4d95872

Thank you for your contribution, @italoacasas

@imyller imyller closed this Sep 22, 2016
@imyller imyller removed their assignment Sep 22, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
@italoacasas @jasnell
test: cleanup stream tests
3f88692 
const and let instead var
assert.strictEqual instead assert.equal

PR-URL: #8668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@italoacasas @Fishrock123
test: cleanup stream tests
9ca8722 
const and let instead var
assert.strictEqual instead assert.equal

PR-URL: #8668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Dec 28, 2016
@italoacasas @targos
test: cleanup stream tests
5dee225 
const and let instead var
assert.strictEqual instead assert.equal

PR-URL: #8668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
targos pushed a commit that referenced this pull request Jan 23, 2017
@italoacasas @targos
test: cleanup stream tests
f5c57c7 
const and let instead var
assert.strictEqual instead assert.equal

PR-URL: #8668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@italoacasas @MylesBorins
test: cleanup stream tests
baf391a 
const and let instead var
assert.strictEqual instead assert.equal

PR-URL: #8668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@italoacasas @MylesBorins
test: cleanup stream tests
735119c 
const and let instead var
assert.strictEqual instead assert.equal

PR-URL: #8668
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ilkka Myller <ilkka.myller@nodefield.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@imyller imyller imyller approved these changes

@targos targos targos 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.
8 participants
@italoacasas @imyller @lpinca @jasnell @targos @mscdex @MylesBorins @nodejs-github-bot