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

Cleanup test file write stream #8894

Closed
wants to merge 5 commits into from
Closed

Cleanup test file write stream #8894

wants to merge 5 commits into from

Conversation

sudaraka
Copy link
Contributor

@sudaraka sudaraka commented Oct 2, 2016

Checklist
  • make -j8 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
  • test
Description of change

Some trivial changes to improve the test code in test/parallel/test-file-write-stream.js.

Submitting this as my first contribution as per instructions from NodeTodo.

@sudaraka
test: file-write-stream, replace var with const/let
04f673b 
This replace all `var` occurrences in test-file-write-stream.js with
`const` (where they are not being reassigned) and `let` (where they are
being reassigned).
@sudaraka
test: file-write-stream, add strict assert/comparison
522047a 
This add strict comparison to the asserts and if statements:

  - Replace `assert.equal` with `assert.strictEqual` where:
    1. Result of `typeof` being compared to a string literal.
    2. Result of `fs.readFileSync` with UTF-8 encoding being compared to
       a string constant.

  - Replace `==` with `===` where integer values are being compared to
    integer literals.
@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Oct 2, 2016
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Oct 2, 2016
targos
targos approved these changes Oct 2, 2016
View reviewed changes
test/parallel/test-file-write-stream.js
@@ -51,14 +51,14 @@ file
fs.unlinkSync(fn);
});

for (var i = 0; i < 11; i++) {
for (let i = 0; i < 11; i++) {
(function(i) {
file.write('' + i);
})(i);
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be simplified by removing the IIFE

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed the IIFE, Thanks for pointing it out.

lpinca
lpinca reviewed Oct 2, 2016
View reviewed changes
test/parallel/test-file-write-stream.js
}

process.on('exit', function() {
for (var k in callbacks) {
for (const k in callbacks) {
assert.equal(0, callbacks[k], k + ' count off by ' + callbacks[k]);
Copy link
Member

@lpinca lpinca Oct 2, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

Nit: how about using strictEqual and a template literal here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing it out. Updated it.

@sudaraka
test: file-write-stream, use template literals
42cb021
@sudaraka
test: file-write-stream, add strict assert/comparison (2)
592a9bc 
This replace a `assert.equal` with a `assert.strictEqual` which was
missed in previous commit.
lpinca
lpinca approved these changes Oct 2, 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.

LGTM

@lpinca
Copy link
Member

lpinca commented Oct 2, 2016

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

@Trott
Copy link
Member

Trott commented Oct 2, 2016

LGTM if CI doesn't reveal any issues with the test.

Looping in @thealphanerd in case they have an opinion about the introduction of for (let...) in one place in the test. I think for (let..) is fine in tests, but I won't push back if people think we should avoid it here like we often do in the lib directory due to performance issues.

addaleax
addaleax approved these changes Oct 3, 2016
View reviewed changes
Copy link
Member

@addaleax addaleax left a comment

Choose a reason for hiding this comment

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

LGTM, and fwiw I’m perfectly fine with let here.

@Trott
Copy link
Member

Trott commented Oct 5, 2016

CI failures are unrelated known flaky tests. Landing.

Trott pushed a commit to Trott/io.js that referenced this pull request Oct 5, 2016
@sudaraka @Trott
test: refactor test-file-write-stream
1d4ba1b 
Replace all `var` occurrences in test-file-write-stream.js with
`const` (where they are not being reassigned) and `let` (where they are
being reassigned).

Add strict comparison to the asserts and if statements:

  - Replace `assert.equal` with `assert.strictEqual` where:
    1. Result of `typeof` being compared to a string literal.
    2. Result of `fs.readFileSync` with UTF-8 encoding being compared to
       a string constant.

  - Replace `==` with `===` where integer values are being compared to
    integer literals.

Remove unnecessary very IIFE.

Use template literals.

PR-URL: nodejs#8894
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@Trott
Copy link
Member

Trott commented Oct 5, 2016

Landed in 1d4ba1b

@Trott Trott closed this Oct 5, 2016
jasnell pushed a commit that referenced this pull request Oct 6, 2016
@sudaraka @jasnell
test: refactor test-file-write-stream
f99a048 
Replace all `var` occurrences in test-file-write-stream.js with
`const` (where they are not being reassigned) and `let` (where they are
being reassigned).

Add strict comparison to the asserts and if statements:

  - Replace `assert.equal` with `assert.strictEqual` where:
    1. Result of `typeof` being compared to a string literal.
    2. Result of `fs.readFileSync` with UTF-8 encoding being compared to
       a string constant.

  - Replace `==` with `===` where integer values are being compared to
    integer literals.

Remove unnecessary very IIFE.

Use template literals.

PR-URL: #8894
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Oct 11, 2016
@sudaraka @Fishrock123
test: refactor test-file-write-stream
93c4874 
Replace all `var` occurrences in test-file-write-stream.js with
`const` (where they are not being reassigned) and `let` (where they are
being reassigned).

Add strict comparison to the asserts and if statements:

  - Replace `assert.equal` with `assert.strictEqual` where:
    1. Result of `typeof` being compared to a string literal.
    2. Result of `fs.readFileSync` with UTF-8 encoding being compared to
       a string constant.

  - Replace `==` with `===` where integer values are being compared to
    integer literals.

Remove unnecessary very IIFE.

Use template literals.

PR-URL: #8894
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax 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.
9 participants
@sudaraka @lpinca @Trott @addaleax @targos @cjihrig @mscdex @MylesBorins @nodejs-github-bot