Skip to content

test: add fs/promises filehandle sync() and datasync() tests. - #20530

Closed
shisama wants to merge 4 commits into
nodejs:masterfrom
shisama:test-fs-promises-file-handle-sync
Closed

test: add fs/promises filehandle sync() and datasync() tests.#20530
shisama wants to merge 4 commits into
nodejs:masterfrom
shisama:test-fs-promises-file-handle-sync

Conversation

@shisama

@shisamashisama commented May 5, 2018

Copy link
Copy Markdown
Contributor

To increase test coverage for fs/promises,
added tests for filehandle.sync and filehandle.datasync.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-botnodejs-github-bot added the test Issues and PRs related to the tests. label May 5, 2018
ChALkeR
ChALkeR reviewed May 6, 2018
View reviewed changes
Comment threadtest/parallel/test-fs-promises-file-handle-sync.js Outdated

@ChALkeRChALkeRMay 6, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

handle.write(buf)?
Same results and coverage, also see #20559.

ChALkeR
ChALkeR reviewed May 6, 2018
View reviewed changes
Comment threadtest/parallel/test-fs-promises-file-handle-sync.js Outdated

@ChALkeRChALkeRMay 6, 2018•
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

read(handle → handle.read(? See the comment above.

@shisamashisamaMay 7, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@ChALkeR Fixed them. Thanks.

@ChALkeRChALkeR added experimental Issues and PRs related to experimental features. fs Issues and PRs related to the fs subsystem / file system. labels May 8, 2018
@shisama
shisamaforce-pushed the test-fs-promises-file-handle-sync branch from 1ff5b56 to e8f8408CompareMay 9, 2018 01:50
@ChALkeR

ChALkeR commented May 20, 2018

Copy link
Copy Markdown
Member

/cc @Trott

@BridgeAR

BridgeAR commented May 29, 2018

Copy link
Copy Markdown
Member

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

@shisama
shisamaforce-pushed the test-fs-promises-file-handle-sync branch from 5784c2d to 2b54d4aCompareJune 5, 2018 11:27
@shisama

shisama commented Jun 5, 2018

Copy link
Copy Markdown
ContributorAuthor

This is rebased on 0300f7c. make -j4 test is passed.

@apapirovski

apapirovski commented Jun 10, 2018

Copy link
Copy Markdown
Contributor

@nodejs/fs please give this a look and a review, if possible. It's been over a month.

joyeecheung
joyeecheung approved these changes Jun 10, 2018
View reviewed changes

@joyeecheungjoyeecheung left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This does not seem to actually test that fsync and fdatasync calls are effective, but I am not quite sure how to write tests for them with our APIs either, so LGTM as long as the code paths are covered.

Comment threadtest/parallel/test-fs-promises-file-handle-sync.js Outdated

@joyeecheungjoyeecheungJun 10, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

common.mustCall() here seems unnecessary. If anything, common.crashOnUnhandledRejection() above gives a better hint about why it fails.

@shisamashisamaJun 10, 2018

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@joyeecheung Thanks. Removed common.mustCall().

Masashi Hirano added 4 commits June 10, 2018 16:37
Masashi Hirano
test: add fs/promises filehandle sync() and datasync() tests.
05d5a1b
To increase test coverage for fs/promises,
added tests for filehandle.sync and filehandle.datasync.
shisama
test: replace fs/promises methods with fs/promises filehandle methods
4c72ba1
Replacing fs/promises methods read and write with fs/promises
filehandle methods.
Masashi Hirano
test: fix fs/promises to fs.promises
c431fc5
Fix module loading because fs/promises was moved to fs.promises
Masashi Hirano
test: remove calling common.mustCall() that is unnecessary
d462d2c
@shisama
shisamaforce-pushed the test-fs-promises-file-handle-sync branch from 2b54d4a to d462d2cCompareJune 10, 2018 08:50
@apapirovski

apapirovski commented Jun 25, 2018

Copy link
Copy Markdown
Contributor

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

@apapirovskiapapirovski added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jun 25, 2018
@shisama

shisama commented Jun 30, 2018

Copy link
Copy Markdown
ContributorAuthor

CI failure is not related to this. It happens in parallel/test-timers-same-timeout-wrong-list-deleted.

@hiroppy

hiroppy commented Jun 30, 2018

Copy link
Copy Markdown
Member

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

@shisama

shisama commented Jul 7, 2018

Copy link
Copy Markdown
ContributorAuthor

I think CI failure node-test-commit-windows-fanned is not related to this.

@targos

targos commented Jul 15, 2018

Copy link
Copy Markdown
Member

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

targos pushed a commit to targos/node that referenced this pull request Jul 15, 2018
Masashi Hirano@targos
test: add filehandle sync() and datasync() tests
71ca076
To increase test coverage for fs.promises,
added tests for filehandle.sync and filehandle.datasync.
PR-URL: nodejs#20530
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targos

targos commented Jul 15, 2018

Copy link
Copy Markdown
Member

Landed in 71ca076. Thanks for the contribution @shisama.

@targostargos closed this Jul 15, 2018
@targostargos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 15, 2018
targos pushed a commit that referenced this pull request Jul 15, 2018
Masashi Hirano@targos
test: add filehandle sync() and datasync() tests
678313d
To increase test coverage for fs.promises,
added tests for filehandle.sync and filehandle.datasync.
PR-URL: #20530
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@targostargos mentioned this pull request Jul 17, 2018
Merged
This was referenced Jul 18, 2018
Closed
Closed
Closed
Closed
Closed
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ChALkeRChALkeRChALkeR left review comments
@jasnelljasnelljasnell approved these changes
@cjihrigcjihrigcjihrig approved these changes
@joyeecheungjoyeecheungjoyeecheung approved these changes

Assignees

No one assigned

Labels

experimentalIssues and PRs related to experimental features.fsIssues and PRs related to the fs subsystem / file system.testIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@shisama@ChALkeR@BridgeAR@apapirovski@hiroppy@targos@jasnell@cjihrig@joyeecheung@nodejs-github-bot