Skip to content

test: extended test to makeCallback cb type check#12140

Closed
lucamaraschi wants to merge 1 commit intonodejs:masterfrom
lucamaraschi:test-fs-makeCallback
Closed

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

Conversation

@lucamaraschi
Copy link
Contributor

@lucamaraschi lucamaraschi commented Mar 30, 2017

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

@lpinca lpinca Mar 31, 2017

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

@lpinca lpinca Mar 31, 2017

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

@lucamaraschi lucamaraschi Mar 31, 2017

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

@lpinca lpinca Mar 31, 2017

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

@lpinca lpinca Mar 31, 2017

Choose a reason for hiding this comment

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

s/Thows/Throws/ ?

@lucamaraschi lucamaraschi force-pushed the test-fs-makeCallback branch from 83c8316 to 10a9547 Compare March 31, 2017 15:00
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
Copy link
Contributor

@cjihrig cjihrig Mar 31, 2017

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

@cjihrig cjihrig Mar 31, 2017

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

@lpinca lpinca Apr 1, 2017

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.

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

@lpinca lpinca Apr 1, 2017

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
@lucamaraschi lucamaraschi force-pushed the test-fs-makeCallback branch from c0b4f0d to bee1920 Compare April 1, 2017 16:08
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 <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 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 <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
@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 <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
@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 <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 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 <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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jasnell jasnell jasnell approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

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