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: extended test to makeCallback cb type check #12140

Closed

Conversation

lucamaraschi
Copy link
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 nodejs-github-bot added the test Issues and PRs related to the tests. label Mar 30, 2017
@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Mar 30, 2017
assert.throws(test(/foo/), cbTypeError);
assert.throws(test([]), cbTypeError);
assert.throws(test({}), cbTypeError);
function invalidCallbackThowsTests() {
Copy link
Member

Choose a reason for hiding this comment

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

s/Thows/Throws/ ?

return function() {
// fs.stat() calls makeCallback() on its second argument
fs.stat(__filename, cb);
fs.unlink(__filename, cb);
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

Yes works for me.

// Passing undefined/nothing calls rethrow() internally, which emits a warning
assert.doesNotThrow(testMakeStatsCallback());

function invalidCallbackThowsTests() {
Copy link
Member

Choose a reason for hiding this comment

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

s/Thows/Throws/ ?

Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM. One suggestion tho: the process.once('warning', ...) bit can be replaced by a call to common.expectWarning(...)

}

//invalidCallbackThrowsTests();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you remove this.


function test(cb) {
const tmpDir = '/tmp';
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@lucamaraschi lucamaraschi force-pushed the test-fs-makeCallback branch 2 times, most recently from 424fcb6 to 21f94d7 Compare March 31, 2017 22:45
// fs.stat() calls makeCallback() on its second argument
fs.stat(__filename, cb);
// fs.mkdtemp() calls makeCallback() on its third argument
fs.mkdtemp(`${common.tmpDir}${sep}`, {}, cb);
Copy link
Member

Choose a reason for hiding this comment

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

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.

// Verify the case where a callback function is provided
assert.doesNotThrow(testMakeStatsCallback(common.noop));

process.once('warning', common.mustCall((warning) => {
Copy link
Member

Choose a reason for hiding this comment

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

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

makeCallback and makeStatsCallback are both tested intedependently.

Fixes: nodejs#12136
Copy link
Member

@mhdawson mhdawson left a comment

Choose a reason for hiding this comment

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

LGTM

@mhdawson
Copy link
Member

mhdawson commented Apr 3, 2017

@mhdawson
Copy link
Member

mhdawson commented Apr 3, 2017

CI is green landing

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

mhdawson commented Apr 3, 2017

landed as 53828e8

@mhdawson mhdawson closed this Apr 3, 2017
mhdawson pushed a commit that referenced this pull request Apr 3, 2017
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Fixes: #12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@jasnell jasnell mentioned this pull request Apr 4, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 17, 2017

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
Copy link
Member

targos commented Jun 19, 2017

I'm backporting this with #12270

gibfahn pushed a commit that referenced this pull request Jun 19, 2017
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: #12140
Backport-PR-URL: #13785
Fixes: #12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
makeCallback and makeStatsCallback are both tested intedependently.

PR-URL: nodejs/node#12140
Fixes: nodejs/node#12136
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
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.

10 participants