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.

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

test: var to const/let; equal to strictEqual #9944

Closed
wants to merge 1 commit into from
Closed

test: var to const/let; equal to strictEqual #9944

wants to merge 1 commit into from

Conversation

onlyurei
Copy link

@onlyurei onlyurei commented Dec 1, 2016

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

test/parallel/test-fs-append-files.js

  • var -> const / let
  • assert.equal -> assert.strictEqual

@nodejs-github-bot nodejs-github-bot added the test Issues and PRs related to the tests. label Dec 1, 2016
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Dec 1, 2016
@imyller imyller added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Dec 1, 2016
@mscdex
Copy link
Contributor

mscdex commented Dec 1, 2016

There should be a space after test: in the first line of the commit message.

test/parallel/test-fs-append-file.js
- var -> const / let
- assert.equal -> assert.strictEqual
@onlyurei
Copy link
Author

onlyurei commented Dec 1, 2016

@mscdex fixed.

@onlyurei onlyurei changed the title test:var to const/let; assert.equal to strictEqual test: var to const/let; equal to strictEqual Dec 1, 2016
@Trott
Copy link
Member

Trott commented Dec 16, 2016

CI failure is unrelated.

@Trott
Copy link
Member

Trott commented Dec 16, 2016

Looks like someone else came along and did these exact same changes in #10110 about 3 days later. Sorry about that @onlyurei!

However! If you still want to get some improvements in to this file, you can try again with a new pull request. One thing I notice is that there are a bunch of unnecessary fs.unlinkSync() calls at the end of the file. They are there to clean up temp files, but tests that use the temp directory remove it and recreate it so there's no need for tests to clean up that way.

Just making that change would be enough for a pull request. If you want to do something a little more involved (or if you want a good second commit after that one lands), all the ncallbacks stuff can be replaced with wrapping of callbacks in common.mustCall().

In the meantime, I'm going to close this pull request. Sorry it didn't land! Thanks for doing it, though!

@Trott Trott closed this Dec 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants