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: remove AIX guard in fs-options-immutable #12687

Conversation

@thefourtheye
Copy link
Contributor

commented Apr 27, 2017

The fs watch test was not run on AIX till now because of the known
issue, #5085. Now that it is
completed, this guard can be removed.

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)

fs aix test


@nodejs/platform-aix

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

test/parallel/test-fs-options-immutable.js Outdated
if (!common.isAix) {
// TODO(thefourtheye) Remove this guard once
// https://github.com/nodejs/node/issues/5085 is fixed
{

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 27, 2017

Contributor

Can we just get rid of this start brace and its matching end brace and re-indent the enclosed code?

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 27, 2017

Author Contributor

All the other tests also have this block thing. I thought it would be consistent. If you want to remove it, I can do it.

This comment has been minimized.

Copy link
@mscdex

mscdex Apr 27, 2017

Contributor

Well IMHO it's excessive since it doesn't (directly) provide scoping for any variables (or any code for that matter). I don't know how everyone else feels about it.

This comment has been minimized.

Copy link
@Trott

Trott Apr 27, 2017

Member

Wouldn't removing the outer block still be consistent with the rest of the test because everything inside that block scope is itself block-scoped? So you'd end up with this block-scoped code?:

{
  let watch;
  assert.doesNotThrow(() => {
    watch = fs.watch(__filename, options, common.noop);
  });
  watch.close();
}

{
  assert.doesNotThrow(() => fs.watchFile(__filename, options, common.noop));
  fs.unwatchFile(__filename);
}

This comment has been minimized.

Copy link
@thefourtheye

thefourtheye Apr 27, 2017

Author Contributor

Oh yeah, makes sense. Removed that block now...

test: remove AIX guard in fs-options-immutable
The fs watch test was not run on AIX till now because of the known
issue, #5085. Now that it is
completed, this guard can be removed.

@thefourtheye thefourtheye force-pushed the thefourtheye:make-fs-options-immutable-available-on-aix branch to aafd390 Apr 27, 2017

@thefourtheye

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

CI Run: https://ci.nodejs.org/job/node-test-pull-request/7700/ (after removing the block)

@mscdex

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

LGTM

@gibfahn

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

LGTM but I'd appreciate a review from @gireeshpunathil

@gireeshpunathil
Copy link
Member

left a comment

Changes look good to me, thanks!

@mhdawson
Copy link
Member

left a comment

LGTM

@mhdawson

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

@gibfahn

This comment has been minimized.

Copy link
Member

commented Apr 28, 2017

Landed in b16869c , thanks @thefourtheye !

@gibfahn gibfahn closed this Apr 28, 2017

gibfahn added a commit that referenced this pull request Apr 28, 2017
test: remove AIX guard in fs-options-immutable
The fs watch test was not run on AIX till now because of the known
issue, #5085. Now that it is
completed, this guard can be removed.

PR-URL: #12687
Reviewed-By: Gibson Fahnestock <gibfahn@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Brian White <mscdex@mscdex.net>

@thefourtheye thefourtheye deleted the thefourtheye:make-fs-options-immutable-available-on-aix branch Apr 30, 2017

@jasnell jasnell referenced this pull request May 11, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 18, 2017

Depends on #7831 which is dont-land.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.