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: improve test-fs-read-stream.js #9629

Closed
wants to merge 1 commit into from
Closed

test: improve test-fs-read-stream.js #9629

wants to merge 1 commit into from

Conversation

jennabelle
Copy link
Contributor

@jennabelle jennabelle commented Nov 16, 2016 •
edited
Loading

tests: refactor to ES6 to strengthen tests

changed var to const, assert.equal() to assert.strictEqual()

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Nov 16, 2016
@Fishrock123
Copy link
Contributor

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

Trott
Trott approved these changes Nov 16, 2016
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

LGTM

@bnoordhuis
Copy link
Member

@jennabelle Can you make the commit log conform to the house style? CONTRIBUTING.md outlines what it should look like. The first line should at least start with test: and not exceed 50 columns.

@jennabelle jennabelle changed the title changed var to const, assert.equal() to assert.strictEqual() test: changed var to const, assert.equal() to assert.strictEqual() Nov 16, 2016
@jennabelle
Copy link
Contributor Author

@bnoordhuis done, but pls let me know if there is something i missed.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Nov 16, 2016
bnoordhuis
bnoordhuis approved these changes Nov 16, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

Code changes LGTM but you only updated the title of the pull request, not the commit, and it's still > 50 columns.

mhdawson
mhdawson approved these changes Nov 17, 2016
View reviewed changes
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM provided title is updated to meet commit guidelines.

@jennabelle jennabelle changed the title test: changed var to const, assert.equal() to assert.strictEqual() test: change var to const, assert.equal() to assert.strictEqual() Nov 19, 2016
@jennabelle jennabelle changed the title test: change var to const, assert.equal() to assert.strictEqual() test: refactor to ES6 to strengthen tests Nov 19, 2016
@jennabelle
Copy link
Contributor Author

@bnoordhuis @mhdawson sorry, i misunderstood earlier- just changed title to < 50 cols and commit log should conform to CONTRIBUTING.md guidelines now- pls let me know what u think

@fhinkel
Copy link
Member

fhinkel commented Nov 21, 2016

You need to change the commit message. Do that locally with git commit --amend and force push. To see if it worked, click at the top on "Commits(1)".

@gibfahn
Copy link
Member

gibfahn commented Nov 21, 2016 •
edited
Loading

@jennabelle This is the bit you need to change:
image

Commit message could be something like this (second line optional):

test: improve test-fs-read-stream.js

Changed var to const and assert.equal() to assert.strictEqual()

@jennabelle jennabelle force-pushed the make-tests-more-clear branch from 7e421a6 to 22b1ac2 Compare November 27, 2016 08:47
@jennabelle
Copy link
Contributor Author

@gibfahn @fhinkel @mhdawson @bnoordhuis Thanks for the direction, done.

screen shot 2016-11-27 at 12 51 07 am

@fhinkel
Copy link
Member

fhinkel commented Nov 27, 2016

LGTM Thanks!

bnoordhuis
bnoordhuis approved these changes Nov 28, 2016
View reviewed changes
Copy link
Member

@bnoordhuis bnoordhuis left a comment

Choose a reason for hiding this comment

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

LGTM but the commit log title is not the same as the pull request title.

@jennabelle jennabelle changed the title test: refactor to ES6 to strengthen tests test: improve test-fs-read-stream.js Nov 28, 2016
@jennabelle
Copy link
Contributor Author

@bnoordhuis Thx, done.

@mhdawson
Copy link
Member

mhdawson commented Dec 6, 2016

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

gibfahn
gibfahn approved these changes Dec 7, 2016
View reviewed changes
Copy link
Member

@gibfahn gibfahn left a comment

Choose a reason for hiding this comment

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

CI is green, LGTM

Trott pushed a commit to Trott/io.js that referenced this pull request Dec 7, 2016
@jennabelle @Trott
test: improve test-fs-read-stream.js
29b2e88 
var -> const
assert.equal() -> assert.strictEqual()

PR-URL: nodejs#9629
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@Trott
Copy link
Member

Trott commented Dec 7, 2016

Landed in 29b2e88. Thanks for your patience and persistence! 🎉

@Trott Trott closed this Dec 7, 2016
addaleax pushed a commit that referenced this pull request Dec 8, 2016
@jennabelle @addaleax
test: improve test-fs-read-stream.js
e695862 
var -> const
assert.equal() -> assert.strictEqual()

PR-URL: #9629
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
addaleax pushed a commit to addaleax/node that referenced this pull request Dec 8, 2016
@jennabelle @addaleax
test: improve test-fs-read-stream.js
314b04d 
var -> const
assert.equal() -> assert.strictEqual()

PR-URL: nodejs#9629
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@italoacasas italoacasas mentioned this pull request Dec 15, 2016
Merged
MylesBorins pushed a commit that referenced this pull request Dec 21, 2016
@jennabelle @MylesBorins
test: improve test-fs-read-stream.js
70861d6 
var -> const
assert.equal() -> assert.strictEqual()

PR-URL: #9629
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Dec 21, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gibfahn gibfahn gibfahn approved these changes

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

@bnoordhuis bnoordhuis bnoordhuis approved these changes

Assignees
No one assigned
Labels
fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.