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

test: extended test to makeCallback cb type check by lucamaraschi · Pull Request #12140 · nodejs/node · GitHub

/ node Public

test: extended test to makeCallback cb type check - #12140

Closed
lucamaraschi wants to merge 1 commit into
nodejs:masterfrom
lucamaraschi:test-fs-makeCallback
ConversationCommits (1)ChecksFiles changed
Closed

test: extended test to makeCallback cb type check#12140
lucamaraschi wants to merge 1 commit into
nodejs:masterfrom
lucamaraschi:test-fs-makeCallback

Conversation

lucamaraschi commented Mar 30, 2017

Copy link
Copy Markdown
Contributor

makeCallback and makeStatsCallback are both tested intedependently.

Fixes: #12136

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, fs

nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 30, 2017
mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Mar 30, 2017
lpinca requested changes Mar 31, 2017
View reviewed changes
Comment thread test/parallel/test-fs-make-callback.js Outdated

lpinca Mar 31, 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

s/Thows/Throws/ ?

Comment thread test/parallel/test-fs-make-callback.js Outdated

lpinca Mar 31, 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

Doesn't this delete the file? How about using another method that calls makeCallback() like, I don't know, fs.chmod()?
The comment should also be updated.

lucamaraschi Mar 31, 2017

Copy link
Copy Markdown
Contributor 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

Good point...but any of the operations which are calling makeCallback have an impact on the file. I would then use fs.mkdtemp which only creates a temp dir which we can delete at the end of the test. Does it make sense to you @lpinca?

lpinca Mar 31, 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

Yes works for me.

Comment thread test/parallel/test-fs-makeStatsCallback.js Outdated

lpinca Mar 31, 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

s/Thows/Throws/ ?

lucamaraschi force-pushed the test-fs-makeCallback branch from 83c8316 to 10a9547 Compare March 31, 2017 15:00
jasnell approved these changes Mar 31, 2017
View reviewed changes

jasnell 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. One suggestion tho: the process.once('warning', ...) bit can be replaced by a call to common.expectWarning(...)

cjihrig requested changes Mar 31, 2017
View reviewed changes
Comment thread test/parallel/test-fs-make-callback.js Outdated

cjihrig Mar 31, 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

Can you remove this.

Comment thread test/parallel/test-fs-make-callback.js Outdated

cjihrig Mar 31, 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

Should this be os.tmpdir()? Alternatively, there is common.tmpDir for use in tests.

lucamaraschi force-pushed the test-fs-makeCallback branch 2 times, most recently from 424fcb6 to 21f94d7 Compare March 31, 2017 22:45
lpinca reviewed Apr 1, 2017
View reviewed changes
Comment thread test/parallel/test-fs-make-callback.js Outdated

lpinca Apr 1, 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

This throws an error if the test is run in isolation node test/parallel/test-fs-make-callback.js and common.tmpDir has not been created.

I think it makes sense to run common.refreshTmpDir() at the beginning of the test.

lucamaraschi force-pushed the test-fs-makeCallback branch from 21f94d7 to c0b4f0d Compare April 1, 2017 14:12
lpinca reviewed Apr 1, 2017
View reviewed changes
Comment thread test/parallel/test-fs-makeStatsCallback.js Outdated

lpinca Apr 1, 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

I'd use common.expectWarning() for consistency.

test: extended test to makeCallback cb type check
bee1920 
makeCallback and makeStatsCallback are both tested intedependently.

Fixes: nodejs#12136
lucamaraschi force-pushed the test-fs-makeCallback branch from c0b4f0d to bee1920 Compare April 1, 2017 16:08
mhdawson approved these changes Apr 3, 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

mhdawson commented Apr 3, 2017

Copy link
Copy Markdown
Member

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

mhdawson commented Apr 3, 2017

Copy link
Copy Markdown
Member

CI is green landing

mhdawson requested review from thefourtheye and removed request for thefourtheye April 3, 2017 17:32

mhdawson commented Apr 3, 2017

Copy link
Copy Markdown
Member

landed as 53828e8

mhdawson closed this Apr 3, 2017
mhdawson pushed a commit that referenced this pull request Apr 3, 2017
test: extended test to makeCallback cb type check
53828e8 
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Fixes: #12136
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
jasnell mentioned this pull request Apr 4, 2017
Closed
gibfahn mentioned this pull request Jun 15, 2017
Closed
3 tasks

gibfahn commented Jun 17, 2017

Copy link
Copy Markdown
Member

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

targos commented Jun 19, 2017

Copy link
Copy Markdown
Member

I'm backporting this with #12270

targos mentioned this pull request Jun 19, 2017
Closed
2 tasks
gibfahn pushed a commit that referenced this pull request Jun 19, 2017
test: extended test to makeCallback cb type check
b690537 
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
test: extended test to makeCallback cb type check
f255541 
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
test: extended test to makeCallback cb type check
e6d6a41 
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
MylesBorins mentioned this pull request Jul 18, 2017
Merged
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
test: extended test to makeCallback cb type check
0d27e35 
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: nodejs/node#12140
Fixes: nodejs/node#12136
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
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

jasnell jasnell approved these changes
lpinca lpinca approved these changes
cjihrig cjihrig approved these changes
+1 more reviewer
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.

10 participants

Back | FazBrowse Home | New Git URL