test: refactor common.ddCommand() #23411
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Remove different paths for Windows and POSIX. * Remove fixtures file. Simply run the command immediately/directly. * Since it is never called with more than one value for kilobytes, eliminate that argument. * Update/simplify tests that use this function. (They no longer need to use child_process to run the command.) * Update documentation.
refack
reviewed
Oct 10, 2018
test/common/index.js
Outdated
| } else { | ||
| return `dd if=/dev/zero of="${filename}" bs=1024 count=${kilobytes}`; | ||
| } | ||
| function ddCommand(filename) { |
There was a problem hiding this comment.
Then why not call it createZeroFilledFile?
test/common/README.md
Outdated
|
|
||
| Platform normalizes the `dd` command | ||
| Creates a 10Mb file of all null characters. |
There was a problem hiding this comment.
Mb -> MB to not be confused with megabit?
richardlau
approved these changes
Oct 10, 2018
test/common/index.js
Outdated
| } | ||
| function ddCommand(filename) { | ||
| const fd = fs.openSync(filename, 'w'); | ||
| fs.ftruncateSync(fd, 10240 * 1024); |
There was a problem hiding this comment.
I think this should either be 10 * 1024 * 1024 (my preference) or 10485760.
lpinca
approved these changes
Oct 10, 2018
|
Two failures across the board: test-fs-error-messagesThe removed fixture is also referenced in https://ci.nodejs.org/job/node-test-commit-linuxone/nodes=rhel72-s390x/6012/console https://ci.nodejs.org/job/node-test-commit-osx/nodes=osx1011/21794/console test-module-loadingThis PR makes |
|
Ugh! At least one of those tests writes to the fixtures directory. 😞 |
|
Fixed all nits, and (I think) fixed the test problems. |
Change common.ddCommand() to common.createZeroFilledFile().
|
Whoops, missed a nit. Now I think I got all the nits. |
refack
approved these changes
Oct 10, 2018
|
CI: https://ci.nodejs.org/job/node-test-pull-request/17731/ ✔️ (Thanks, @refack!) |
|
CI is green. Does this look good to you now, @richardlau? |
richardlau
approved these changes
Oct 10, 2018
Trott
added
the
author ready
benjamingr
approved these changes
Oct 11, 2018
thefourtheye
approved these changes
Oct 11, 2018
jasnell
approved these changes
Oct 12, 2018
gdams
approved these changes
Oct 12, 2018
Trott
added a commit
to Trott/io.js
that referenced
this pull request
Oct 12, 2018
* Remove different paths for Windows and POSIX. * Remove fixtures file. Simply run the command immediately/directly. * Since it is never called with more than one value for kilobytes, eliminate that argument. * Update/simplify tests that use this function. (They no longer need to use child_process to run the command.) * Update documentation. PR-URL: nodejs#23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
Trott
added a commit
to Trott/io.js
that referenced
this pull request
Oct 12, 2018
Change common.ddCommand() to common.createZeroFilledFile(). PR-URL: nodejs#23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
|
Landed in bcbb937...3d62d38 |
|
Should this be backported to |
Trott
added a commit
to Trott/io.js
that referenced
this pull request
Oct 13, 2018
* Remove different paths for Windows and POSIX. * Remove fixtures file. Simply run the command immediately/directly. * Since it is never called with more than one value for kilobytes, eliminate that argument. * Update/simplify tests that use this function. (They no longer need to use child_process to run the command.) * Update documentation. PR-URL: nodejs#23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
Trott
added a commit
to Trott/io.js
that referenced
this pull request
Oct 13, 2018
Change common.ddCommand() to common.createZeroFilledFile(). PR-URL: nodejs#23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
targos
pushed a commit
that referenced
this pull request
Oct 13, 2018
* Remove different paths for Windows and POSIX. * Remove fixtures file. Simply run the command immediately/directly. * Since it is never called with more than one value for kilobytes, eliminate that argument. * Update/simplify tests that use this function. (They no longer need to use child_process to run the command.) * Update documentation. Backport-PR-URL: #23630 PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
targos
pushed a commit
that referenced
this pull request
Oct 13, 2018
Change common.ddCommand() to common.createZeroFilledFile(). Backport-PR-URL: #23630 PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
jasnell
pushed a commit
that referenced
this pull request
Oct 17, 2018
* Remove different paths for Windows and POSIX. * Remove fixtures file. Simply run the command immediately/directly. * Since it is never called with more than one value for kilobytes, eliminate that argument. * Update/simplify tests that use this function. (They no longer need to use child_process to run the command.) * Update documentation. PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
jasnell
pushed a commit
that referenced
this pull request
Oct 17, 2018
Change common.ddCommand() to common.createZeroFilledFile(). PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
MylesBorins
pushed a commit
that referenced
this pull request
Oct 30, 2018
* Remove different paths for Windows and POSIX. * Remove fixtures file. Simply run the command immediately/directly. * Since it is never called with more than one value for kilobytes, eliminate that argument. * Update/simplify tests that use this function. (They no longer need to use child_process to run the command.) * Update documentation. Backport-PR-URL: #23630 PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
MylesBorins
pushed a commit
that referenced
this pull request
Oct 30, 2018
Change common.ddCommand() to common.createZeroFilledFile(). Backport-PR-URL: #23630 PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
Closed
rvagg
pushed a commit
that referenced
this pull request
Nov 28, 2018
* Remove different paths for Windows and POSIX. * Remove fixtures file. Simply run the command immediately/directly. * Since it is never called with more than one value for kilobytes, eliminate that argument. * Update/simplify tests that use this function. (They no longer need to use child_process to run the command.) * Update documentation. Backport-PR-URL: #23630 PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
rvagg
pushed a commit
that referenced
this pull request
Nov 28, 2018
Change common.ddCommand() to common.createZeroFilledFile(). Backport-PR-URL: #23630 PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
MylesBorins
pushed a commit
that referenced
this pull request
Nov 29, 2018
* Remove different paths for Windows and POSIX. * Remove fixtures file. Simply run the command immediately/directly. * Since it is never called with more than one value for kilobytes, eliminate that argument. * Update/simplify tests that use this function. (They no longer need to use child_process to run the command.) * Update documentation. Backport-PR-URL: #23630 PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
MylesBorins
pushed a commit
that referenced
this pull request
Nov 29, 2018
Change common.ddCommand() to common.createZeroFilledFile(). Backport-PR-URL: #23630 PR-URL: #23411 Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: Richard Lau <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Benjamin Gruenbaum <[email protected]> Reviewed-By: Sakthipriyan Vairamani <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: George Adams <[email protected]>
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
eliminate that argument.
use child_process to run the command.)
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes