LGTM

LGTM

LGTM

LGTM

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

CI has some unrelated failures, LGTM.

When applying this change, I've observed that sometimes this test fails on OS X if the common.tmpDir already contained a file before running the test. If that's the case, the first event received is the one related to the removal of the file by common.refreshTmpDir(), even though the call to fs.watch is performed afterwards. Here's a simplified test case that shows the issue. It fails in my OS X box but passes in my Debian Jessie 64.

'use strict';

const common = require('../common');
const fs = require('fs');
const path = require('path');
const assert = require('assert');

common.refreshTmpDir();
const x = path.join(common.tmpDir, 'x.txt');

const fd1 = fs.openSync(x, 'w+');
fs.closeSync(fd1);

common.refreshTmpDir();

const a = path.join(common.tmpDir, 'a.txt');

const watcher1 = fs.watch(
  common.tmpDir,
  (event, filename) => {
    if (filename)
      assert.equal(filename, 'a.txt');
    watcher1.close();
  }
);

const fd = fs.openSync(a, 'w+');
fs.closeSync(fd);

process.on('exit', () => {
  fs.unlink(a);
});

I don't know if it's bug or a limitation with FSEvents. WDYT?

@bnoordhuis @saghul ...

@santigimeno I think that is an issue specific to FSEvents. I'm going to create a separate workaround for that in a subsequent PR. Let me know if this isn't OK to land as-is. (I mean, not having the refreshTmpDir() call is a bug either way, so this ought to at least be an improvement.)

I'm ok with landing this as long as it doesn't cause problems with the CI, otherwise maybe waiting for the workaround is the better choice.

Workaround is stranger than I thought. Some unpredictable and/or non-intuitive stuff going on. Bummer.

Landed in cdcfeb7

@Trott this doesn't land cleanly. Totally up for a backport though

@thealphanerd The file this PR modifies was added in a semver-major PR, so it shouldn't land on LTS.