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: fix params in parallel/test-fs-null-bytes.js #11601

Merged
merged 1 commit into from
Mar 3, 2017
Merged

test: fix params in parallel/test-fs-null-bytes.js #11601

merged 1 commit into from
Mar 3, 2017

Conversation

vsemozhetbyt
Copy link
Contributor

@vsemozhetbyt vsemozhetbyt commented Feb 28, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

fs, test

parallel/test-fs-null-bytes.js tests fs.appendFile() and fs.writeFile() with wrong parameters scheme, i.e. without mandatory data parameter. This may be not very important for the test aim, but it makes it somehow compromised. This PR adds a simple data parameter to the 6 calls of these functions.

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. Can you add a Refs: line with the related issue URL?

@vsemozhetbyt
Copy link
Contributor Author

@targos Added. Sorry, maybe I had to post 2 separate issues for these cases.

Copy link
Contributor

@seishun seishun left a comment

Choose a reason for hiding this comment

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

  1. Please add an explanation to the commit message explaining the change.
  2. Maybe someone has a better idea for the data than '1'. If not, feel free to ignore.

@targos
Copy link
Member

targos commented Feb 28, 2017

@seishun what about 'data'?

@seishun
Copy link
Contributor

seishun commented Feb 28, 2017

@targos I think it's better to follow other tests. For example. test\parallel\test-fs-write-file-sync.js uses abc and 123.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Feb 28, 2017
@vsemozhetbyt
Copy link
Contributor Author

@seishun Done.

Copy link
Contributor

@seishun seishun left a comment

Choose a reason for hiding this comment

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

Functions fs.appendFile() and fs.writeFile() were called without mandatory data parameter.

How about:

The functions fs.appendFile() and fs.writeFile() were called without the required data parameter.

Also is that how functions are usually referred to in commit messages, with empty brackets?

This PR adds a simple data parameter to 6 calls of these functions.

I don't think this line is necessary, but in any case commit messages don't usually refer to themselves as PRs.

@vsemozhetbyt
Copy link
Contributor Author

vsemozhetbyt commented Feb 28, 2017

@seishun Fixed. As for 'how functions are usually referred to in commit messages' — I don't know. I just followed STYLE_GUIDE here:

References to methods should be used with parenthesis: socket.end() instead of socket.end.

Tell me if I should change this here.

@seishun
Copy link
Contributor

seishun commented Feb 28, 2017

Agreed, let's keep the parentheses as-is.

Also, maybe "were being called" instead of "were called". And maybe "argument" instead of "parameter".

@hiroppy
Copy link
Member

hiroppy commented Mar 1, 2017

@vsemozhetbyt
Copy link
Contributor Author

Could this be landed, please?

@seishun
Copy link
Contributor

seishun commented Mar 3, 2017

@vsemozhetbyt One more thing. The first line is too long. How about replace "params" with "args"?

The functions `fs.appendFile()` and `fs.writeFile()`
were being called without the required `data` argument.

Refs: #11595
@vsemozhetbyt
Copy link
Contributor Author

@seishun Done.

@seishun seishun merged commit c500b5a into nodejs:master Mar 3, 2017
@vsemozhetbyt vsemozhetbyt deleted the test-fs-null-bytes branch March 3, 2017 14:14
addaleax pushed a commit that referenced this pull request Mar 5, 2017
The functions `fs.appendFile()` and `fs.writeFile()`
were being called without the required `data` argument.

Refs: #11595
PR-URL: #11601
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@evanlucas evanlucas mentioned this pull request Mar 8, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

@vsemozhetbyt
Copy link
Contributor Author

@MylesBorins #12477

MylesBorins pushed a commit that referenced this pull request Apr 18, 2017
The functions `fs.appendFile()` and `fs.writeFile()`
were being called without the required `data` argument.

Refs: #11595
Backport-PR-URL: #12477
PR-URL: #11601
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
MylesBorins pushed a commit that referenced this pull request Apr 19, 2017
The functions `fs.appendFile()` and `fs.writeFile()`
were being called without the required `data` argument.

Refs: #11595
Backport-PR-URL: #12477
PR-URL: #11601
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
@MylesBorins MylesBorins mentioned this pull request Apr 19, 2017
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
The functions `fs.appendFile()` and `fs.writeFile()`
were being called without the required `data` argument.

Refs: nodejs/node#11595
Backport-PR-URL: nodejs/node#12477
PR-URL: nodejs/node#11601
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Nikolai Vavilov <vvnicholas@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

9 participants