FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

test: add tests for rs+, sr+ to test-fs-open-flags.js by hiroppy · Pull Request #10780 · nodejs/node · GitHub

/ node Public

test: add tests for rs+, sr+ to test-fs-open-flags.js - #10780

Closed
hiroppy wants to merge 1 commit into
nodejs:masterfrom
hiroppy:feature/add-tests-to-test-fs-open-flags.js
ConversationCommits (1)ChecksFiles changed
Closed

test: add tests for rs+, sr+ to test-fs-open-flags.js#10780
hiroppy wants to merge 1 commit into
nodejs:masterfrom
hiroppy:feature/add-tests-to-test-fs-open-flags.js

Conversation

hiroppy commented Jan 13, 2017 •
edited
Loading

Copy link
Copy Markdown
Member

Add the test of line 35, 36.
https://github.com/nodejs/node/blob/master/lib/internal/fs.js#L35
https://github.com/nodejs/node/blob/master/lib/internal/fs.js#L36

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)

test

nodejs-github-bot added test Issues and PRs related to the tests. dont-land-on-v7.x labels Jan 13, 2017
mscdex added fs Issues and PRs related to the fs subsystem / file system. and removed dont-land-on-v7.x labels Jan 13, 2017
Trott reviewed Jan 13, 2017
View reviewed changes
Comment thread test/parallel/test-fs-open-flags.js Outdated

Trott Jan 13, 2017

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

typo? Seems like it should be || instead of |?

hiroppy Jan 13, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Oops... It is a typo. Thanks!

hiroppy force-pushed the feature/add-tests-to-test-fs-open-flags.js branch from ea9f7f4 to 211358c Compare January 13, 2017 05:09
sam-github suggested changes Jan 13, 2017
View reviewed changes
Comment thread test/parallel/test-fs-open-flags.js Outdated

sam-github Jan 13, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

please put after r+, so that the asserts are in documentation order, making it much easier to check that all documented flags are tested.

Also, I notice that your test for sr+, but that is not documented, but neither are xw, etc, which are also tested here.

I'll document the combinations, unless you are interested in PRing a doc update as well?

hiroppy Jan 13, 2017

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Updated.

I'll update a doc:) Maybe it is here?

hiroppy force-pushed the feature/add-tests-to-test-fs-open-flags.js branch from 211358c to 103397a Compare January 13, 2017 20:35
hiroppy force-pushed the feature/add-tests-to-test-fs-open-flags.js branch from 103397a to 0d87817 Compare January 13, 2017 20:36
mhdawson approved these changes Jan 13, 2017
View reviewed changes

mhdawson 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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM

jasnell commented Jan 16, 2017

Copy link
Copy Markdown
Member

@sam-github does this LGTY now?

jasnell commented Jan 16, 2017

Copy link
Copy Markdown
Member

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

jasnell pushed a commit that referenced this pull request Jan 18, 2017
test: add tests for rs+, sr+ to test-fs-open-flags.js
622b439 
PR-URL: #10780
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell commented Jan 18, 2017

Copy link
Copy Markdown
Member

Landed in 622b439

jasnell closed this Jan 18, 2017
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
test: add tests for rs+, sr+ to test-fs-open-flags.js
0f5b8d7 
PR-URL: nodejs#10780
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas mentioned this pull request Jan 19, 2017
Closed
hiroppy deleted the feature/add-tests-to-test-fs-open-flags.js branch January 21, 2017 08:10
italoacasas mentioned this pull request Jan 25, 2017
Closed
targos pushed a commit that referenced this pull request Jan 28, 2017
test: add tests for rs+, sr+ to test-fs-open-flags.js
c4f1b39 
PR-URL: #10780
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas mentioned this pull request Jan 29, 2017
Merged
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
test: add tests for rs+, sr+ to test-fs-open-flags.js
3cd9833 
PR-URL: nodejs#10780
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
test: add tests for rs+, sr+ to test-fs-open-flags.js
781a99e 
PR-URL: nodejs#10780
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Sam Roberts <vieuxtech@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell commented Mar 8, 2017

Copy link
Copy Markdown
Member

This will need backport PRs in order to land on v6 or v4

This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Trott Trott left review comments
jasnell jasnell approved these changes
cjihrig cjihrig approved these changes
+2 more reviewers
sam-github sam-github approved these changes
mhdawson mhdawson approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

Back | FazBrowse Home | New Git URL