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.

Sign up for GitHub

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

Jump to bottom

test: extended test to makeCallback cb type check #12140

Closed
wants to merge 1 commit into from
Closed

test: extended test to makeCallback cb type check #12140

wants to merge 1 commit into from

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
lpinca
lpinca requested changes Mar 31, 2017
View reviewed changes
test/parallel/test-fs-make-callback.js Outdated
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/ ?

test/parallel/test-fs-make-callback.js Outdated
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.

test/parallel/test-fs-makeStatsCallback.js Outdated
// 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/ ?

jasnell
jasnell approved these changes Mar 31, 2017
View reviewed changes
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(...)

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

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

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

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
lpinca
lpinca reviewed Apr 1, 2017
View reviewed changes
test/parallel/test-fs-make-callback.js
// 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.

lpinca
lpinca reviewed Apr 1, 2017
View reviewed changes
test/parallel/test-fs-makeStatsCallback.js Outdated
// 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.

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

Fixes: nodejs#12136
mhdawson
mhdawson approved these changes Apr 3, 2017
View reviewed changes
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

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

@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
@lucamaraschi @mhdawson
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 <[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
Closed
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
Closed
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

@targos targos mentioned this pull request Jun 19, 2017
Closed
2 tasks
gibfahn pushed a commit that referenced this pull request Jun 19, 2017
@lucamaraschi @gibfahn
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 <[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
@lucamaraschi @gibfahn
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 <[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
@lucamaraschi @MylesBorins
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 <[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
Merged
abhishekumar-tyagi pushed a commit to abhishekumar-tyagi/node that referenced this pull request May 5, 2024
@lucamaraschi @targos
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 <[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
Reviewers

@mhdawson mhdawson mhdawson approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

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
@lucamaraschi @mhdawson @gibfahn @targos @jasnell @lpinca @cjihrig @mscdex @italoacasas @nodejs-github-bot