Skip to content

test: cleanup stream tests#8668

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
Closed
italoacasas wants to merge 8 commits into from
Closed

test: cleanup stream tests #8668

italoacasas wants to merge 8 commits into from

Conversation

@italoacasas
Copy link

@italoacasasitaloacasas commented Sep 20, 2016

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-botnodejs-github-bot added the test Issues and PRs related to the tests. label Sep 20, 2016
@mscdexmscdex 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

@lpincalpinca 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 Outdated
@@ -1,10 +1,12 @@
'use strict';

Copy link
Member

@lpincalpincaSep 20, 2016

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

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

@imyllerimyller changed the title cleanup stream teststest: cleanup stream testsSep 20, 2016
targos
targos approved these changes Sep 20, 2016
View reviewed changes
Copy link
Member

@targostargos 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 Outdated
Copy link
Member

@lpincalpincaSep 20, 2016

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
Author

@italoacasasitaloacasasSep 20, 2016

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

@imyllerimyller 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

@imyllerimyller assigned imyller and unassigned imyllerSep 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
@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 <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@imyller
Copy link
Member

imyller commented Sep 22, 2016

landed in 4d95872

Thank you for your contribution, @italoacasas

@imyllerimyller closed this Sep 22, 2016
@imyllerimyller removed their assignment Sep 22, 2016
jasnell pushed a commit that referenced this pull request Sep 29, 2016
@jasnell
test: cleanup stream tests
3f88692
const and let instead var assert.strictEqual instead assert.equal PR-URL: #8668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@Fishrock123
test: cleanup stream tests
9ca8722
const and let instead var assert.strictEqual instead assert.equal PR-URL: #8668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Dec 28, 2016
@targos
test: cleanup stream tests
5dee225
const and let instead var assert.strictEqual instead assert.equal PR-URL: #8668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Jan 23, 2017
@targos
test: cleanup stream tests
f5c57c7
const and let instead var assert.strictEqual instead assert.equal PR-URL: #8668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jan 24, 2017
@MylesBorins
test: cleanup stream tests
baf391a
const and let instead var assert.strictEqual instead assert.equal PR-URL: #8668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Jan 24, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Feb 1, 2017
@MylesBorins
test: cleanup stream tests
735119c
const and let instead var assert.strictEqual instead assert.equal PR-URL: #8668 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Ilkka Myller <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnelljasnelljasnell approved these changes

@lpincalpincalpinca approved these changes

@targostargostargos approved these changes

+1 more reviewer

@imyllerimyllerimyller approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

streamIssues and PRs related to the stream subsystem.testIssues 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