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: test sync version of mkdir & rmdir #2588

Closed
wants to merge 1 commit into from

Conversation

thefourtheye
Copy link
Contributor

This patch includes tests for sync versions of mkdir and rmdir.
Also, it moves the test to parallel.

CI Run: https://jenkins-iojs.nodesource.com/job/node-test-pull-request/197/

This patch includes tests for sync versions of mkdir and rmdir.
Also, it moves the test to `parallel`.
@thefourtheye thefourtheye added the test Issues and PRs related to the tests. label Aug 28, 2015
@Fishrock123
Copy link
Contributor

cc @nodejs/build this moves another test to /parallel/

@silverwind
Copy link
Contributor

LGTM code wise. Are sync tests in parallel fine?

@Trott
Copy link
Member

Trott commented Sep 1, 2015

Sync tests in parallel are fine. There's already a whole bunch:

test/parallel/test-child-process-spawnsync-env.js
test/parallel/test-child-process-spawnsync-input.js
test/parallel/test-child-process-spawnsync-timeout.js
test/parallel/test-child-process-spawnsync.js
test/parallel/test-fs-append-file-sync.js
test/parallel/test-fs-fsync.js
test/parallel/test-fs-read-file-sync-hostname.js
test/parallel/test-fs-read-file-sync.js
test/parallel/test-fs-readfilesync-pipe-large.js
test/parallel/test-fs-sync-fd-leak.js
test/parallel/test-fs-write-file-sync.js
test/parallel/test-fs-write-sync.js
test/parallel/test-stdin-pause-resume-sync.js
test/parallel/test-stream2-read-sync-stack.js
test/parallel/test-sync-io-option.js

@silverwind
Copy link
Contributor

Excuse me for asking, but why exactly do we split tests into parallel and sequential then?

@rvagg
Copy link
Member

rvagg commented Sep 2, 2015

Tests in parallel are run in actual parallel processes by the python runner when run with -J according to the number of CPUs on the machine. So, anything that doesn't create a machine-dependency should go in there; examples of problematic tests include ones that bind to specific ports or use specific on-disk resources, those should go in sequential so as to not create inter-test conflicts.

@rvagg
Copy link
Member

rvagg commented Sep 2, 2015

sorry, to answer the actual question @silverwind, "sync" tests in the Node sense are perfectly find and good to go in the parallel directory, they will be run in parallel Node processes

@Trott
Copy link
Member

Trott commented Sep 2, 2015

Parallel tests are run in parallel by four (or whatever) separate node processes. This is handled by the Python wrapper. So synchronous stuff is not a problem. It only becomes a problem if it (for example) binds to a port on localhost. Then it might be binding to a port that another process is trying to bind to. So that needs to go in sequential. (Temp directory stuff is also not a problem because the Python wrapper creates separate tmp directories for the different note processes.)

@rvagg just answered this so now my answer is superfluous but I typed it up, so here you go. Maybe there's a detail in there that is helpful.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Sep 14, 2015
@thefourtheye
Copy link
Contributor Author

@thefourtheye
Copy link
Contributor Author

Bump!

@jbergstroem
Copy link
Member

LGTM

thefourtheye added a commit that referenced this pull request Sep 21, 2015
This patch includes tests for sync versions of mkdir and rmdir.
Also, it moves the test to `parallel`.

PR-URL: #2588
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@thefourtheye
Copy link
Contributor Author

Thanks for the review people, landed in 571a517

@thefourtheye thefourtheye deleted the improve-mkdir-rmdir branch September 21, 2015 07:14
thefourtheye added a commit that referenced this pull request Sep 22, 2015
This patch includes tests for sync versions of mkdir and rmdir.
Also, it moves the test to `parallel`.

PR-URL: #2588
Reviewed-By: Roman Reiss <me@silverwind.io>
Reviewed-By: Johan Bergström <bugs@bergstroem.nu>
@rvagg rvagg mentioned this pull request Sep 22, 2015
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.

7 participants