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: cleanup test-fs-watch.js #12595

Closed
wants to merge 1 commit into from
Closed

test: cleanup test-fs-watch.js #12595

wants to merge 1 commit into from

Conversation

RobotMermaid
Copy link
Contributor

Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

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 nodejs-github-bot added the test Issues and PRs related to the tests. label Apr 22, 2017
cjihrig
cjihrig approved these changes Apr 22, 2017
View reviewed changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with a question.

test/sequential/test-fs-watch.js Outdated
oldhandle.close(); // clean up

assert.throws(function() {
const w = fs.watchFile(__filename, {persistent: false}, common.noop);
oldhandle = w._handle;
w._handle = { stop: w._handle.stop };
w.stop();
}, TypeError);
}, /^TypeError: Illegal invocation/);
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 have a $ on the end?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes - oops

Copy link
Contributor

@mscdex mscdex Apr 22, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

@cjihrig I would say so.

@mscdex mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Apr 22, 2017
@bengl bengl added the code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. label Apr 22, 2017
@mscdex
Copy link
Contributor

mscdex commented Apr 22, 2017

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

@jasnell
Copy link
Member

jasnell commented Apr 24, 2017

This will need to be rebased before landing.
@RobotMermaid ... let us know if you're not quite sure how to do the rebasing and one of us will help walk you through it.

@RobotMermaid @Trott
test: cleanup test-fs-watch.js
da6e1e8 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().
@Trott
Copy link
Member

Trott commented Apr 25, 2017

Since the conflict was with a PR of mine that landed, I went ahead and did the rebase. Pushed to the branch here, so it should be all set to go. Another CI probably wouldn't hurt, though.

@Trott
Copy link
Member

Trott commented Apr 25, 2017

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

Trott pushed a commit to Trott/io.js that referenced this pull request Apr 26, 2017
@RobotMermaid @Trott
test: cleanup test-fs-watch.js
b65d5e4 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: nodejs#12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@Trott
Copy link
Member

Trott commented Apr 26, 2017

Landed in b65d5e4.
Thanks for the contribution! 🎉

@Trott Trott closed this Apr 26, 2017
@evanlucas evanlucas mentioned this pull request May 1, 2017
Merged
evanlucas pushed a commit that referenced this pull request May 1, 2017
@RobotMermaid @evanlucas
test: cleanup test-fs-watch.js
7111676 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: #12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@RobotMermaid @evanlucas
test: cleanup test-fs-watch.js
3114349 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: #12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
evanlucas pushed a commit that referenced this pull request May 2, 2017
@RobotMermaid @evanlucas
test: cleanup test-fs-watch.js
e36a256 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: #12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 15, 2017
@RobotMermaid @MylesBorins
test: cleanup test-fs-watch.js
51d9806 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: #12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
@RobotMermaid @MylesBorins
test: cleanup test-fs-watch.js
a714449 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: #12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
MylesBorins pushed a commit that referenced this pull request May 18, 2017
@RobotMermaid @MylesBorins
test: cleanup test-fs-watch.js
b09f738 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: #12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
@MylesBorins MylesBorins mentioned this pull request May 23, 2017
Merged
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@RobotMermaid @andrew749
test: cleanup test-fs-watch.js
9e3ccf5 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: nodejs/node#12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
andrew749 pushed a commit to michielbaird/node that referenced this pull request Jul 19, 2017
@RobotMermaid @andrew749
test: cleanup test-fs-watch.js
03441f8 
Reversed "actual" and "expected" arguments for assert.strictEqual().

Replaced constructor with regular expression for assert.throws().

PR-URL: nodejs/node#12595
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Bryan English <[email protected]>
Reviewed-By: Santiago Gimeno <[email protected]>
Reviewed-By: Gibson Fahnestock <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mscdex mscdex mscdex left review comments

@gibfahn gibfahn gibfahn approved these changes

@cjihrig cjihrig cjihrig approved these changes

@santigimeno santigimeno santigimeno approved these changes

@bengl bengl bengl approved these changes

Assignees
No one assigned
Labels
code-and-learn Issues related to the Code-and-Learn events and PRs submitted during the events. 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.
9 participants
@RobotMermaid @mscdex @jasnell @Trott @bengl @santigimeno @cjihrig @gibfahn @nodejs-github-bot