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

refactor test/sequential/test-fs-watch.js #14534

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 29, 2017

  • add block scoping
  • rename block-scoped identifiers (e.g., filenameTwo -> filename)
  • use common.mustCall() instead of exit handler
  • order modules per test writing guide
Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

test fs

@Trott Trott added fs Issues and PRs related to the fs subsystem / file system. test Issues and PRs related to the tests. labels Jul 29, 2017
@Trott
Copy link
Member Author

Trott commented Jul 29, 2017

@Trott
Copy link
Member Author

Trott commented Jul 30, 2017

@nodejs/testing

});
{
fs.watch(__filename, { persistent: false }, function() {
assert(0);
Copy link
Contributor

Choose a reason for hiding this comment

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

While you're here, can we have something nicer than assert(0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Switched to common.mustNotCall(). Thanks.

* add block scoping
* rename block-scoped identifiers (e.g., filenameTwo -> filename)
* use common.mustCall() instead of exit handler
* use common.mustNotCall() as appropriate
* order modules per test writing guide
@Trott
Copy link
Member Author

Trott commented Jul 30, 2017

@Trott
Copy link
Member Author

Trott commented Aug 1, 2017

Landed in 8c2cac6

@Trott Trott closed this Aug 1, 2017
Trott added a commit to Trott/io.js that referenced this pull request Aug 1, 2017
* add block scoping
* rename block-scoped identifiers (e.g., filenameTwo -> filename)
* use common.mustCall() instead of exit handler
* use common.mustNotCall() as appropriate
* order modules per test writing guide

PR-URL: nodejs#14534
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax
Copy link
Member

addaleax commented Aug 1, 2017

This doesn’t land cleanly on 8.x; if you can, please follow the guide and raise a backport PR, if you don’t think it’s worth it just add the dont-land-on label, for a single test that should be fine.

Trott added a commit to Trott/io.js that referenced this pull request Aug 1, 2017
* add block scoping
* rename block-scoped identifiers (e.g., filenameTwo -> filename)
* use common.mustCall() instead of exit handler
* use common.mustNotCall() as appropriate
* order modules per test writing guide

PR-URL: nodejs#14534
Reviewed-By: Colin Ihrig <[email protected]>
@Trott
Copy link
Member Author

Trott commented Aug 1, 2017

I think this (and I'd guess a lot of other things) will land cleanly if #14162 is backported first. Still, I want this to land so I'm going to go backport it right now...

@Trott
Copy link
Member Author

Trott commented Aug 1, 2017

Backported in #14575

addaleax pushed a commit that referenced this pull request Aug 1, 2017
* add block scoping
* rename block-scoped identifiers (e.g., filenameTwo -> filename)
* use common.mustCall() instead of exit handler
* use common.mustNotCall() as appropriate
* order modules per test writing guide

Backport-PR-URL: #14575
Backport-Reviewed-By: Anna Henningsen <[email protected]>

PR-URL: #14534
Reviewed-By: Colin Ihrig <[email protected]>
@addaleax addaleax mentioned this pull request Aug 2, 2017
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

@Trott Trott deleted the watch-scope branch January 13, 2022 22:46
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.

4 participants