ignore files which are not found by files() #2538

villesau · 2016-10-14T08:51:49Z

broken symlinks caused file not found -errors and the system should not fail on those.

boneskull

Let's discuss this strategy further, or please implement my suggested changes if you're in agreement.

boneskull · 2016-10-15T22:23:22Z

lib/utils.js

-        exports.files(path, ext, ret);
-      } else if (path.match(re)) {
-        ret.push(path);
+      try {


Hi! Thanks for the PR.

I'm not sure I agree with the strategy here. If we want to provide "expected" behavior--and we should--then a broken symlink should still be watched. If/when the symlink is fixed while watching, and the symlink is to a directory, we'd want to watch files in that directory as well (if requested). What happens instead is that broken symlinks silently fail, which causes the same problem for users that this PR was meant to address: "it's not working, and I don't know why".

But that implementation sounds like a pain, and I'm not really interested in significant efforts against the --watch functionality as part of Mocha core short of low-severity bugfixes.

Instead of trying to further enhance --watch, I'd be happy with a solution which leverages fs.lstatSync() and re-throws with a helpful error if a symlink is broken. That'd let the user know something is wrong, and that they'll need to address it if they want Mocha to do what they're telling it to do.

Thanks for the comments @boneskull !

watching broken symlinks and their content would be nice, but right now it wouldn't follow the current convention: --watch seems to watch only files which mocha can resolve on startup. Thus i think it would be too much of effort to fix the behaviour only for symlinks.

I wasn't aware of lstatSync function so i ended up to solution I currently propose in this PR. But indeed lstatSync would be more feasible solution here. I played around with lstatSync a bit and it wouldn't throw at all if the symlink is broken - which is fine in my use case at least. Instead, mocha would fail later on to 'module not found' if it tries to require module from broken symlink, which again sounds like a reasonable behaviour to me. That exception also points out the failing module so it's easy to sort out why it is failing.

So what I propose is that I remove the try-catch and replace statSync with lstatSync here.

Watch option built into mocha is pretty important to us since starting up and setting things up takes pretty long time in larger projects.

jsf-clabot · 2016-11-10T18:32:49Z

All committers have signed the CLA.

villesau · 2016-11-13T20:30:57Z

@boneskull what do you think of current approach?

ScottFreeCode

I have some minor issues that should be pretty easy to clean up. There's also still the central issue of whether this hides problems rather than making them more clear and manageable (see comment on existsSync).

ScottFreeCode · 2016-11-14T23:00:32Z

lib/utils.js

        exports.files(path, ext, ret);
-      } else if (path.match(re)) {
+      } else if (path.match(re) && existsSync(path)) {


Using existsSync here still silently ignores the problem rather than making the error message useful as discussed in the previous review, doesn't it?

Yeah true. I removed existsSync now so it still fails if .js symlinks files are broken, but also gives better error message when trying to require it later on. To me personally both are fine as our major problem was with other files than js.

ScottFreeCode · 2016-11-14T23:06:30Z

test/acceptance/file-utils.spec.js

-describe('lookupFiles', function () {
-  var tmpDir = path.join(os.tmpDir(), 'mocha-lookup-files');
+describe('files utils', function () {
+  var tmpDir = path.join(os.tmpDir(), 'mocha-files');


'mocha-files' isn't any less function-specific since the other function's name is just .files, it's just more vague (outside of the context of the function names anyway) and less resistant to name collision; I'd recommend leaving this directory name as 'mocha-lookup-files' or, if you really want to avoid implying it related only to one of the two functions, changing to 'mocha-file-lookup'.

ScottFreeCode · 2016-11-14T23:08:34Z

test/acceptance/file-utils.spec.js

-      .to
-      .eql([]);
+  describe('.lookupFiles', function () {
+    (symlinkSupported ? it : it.skip)('lookupFiles keke should not choke on symlinks', function () {


Test description should omit "lookupFiles" (which is in the suite name/description, which is generally printed before the test name) and "keke".

ScottFreeCode · 2016-11-14T23:33:26Z

test/acceptance/file-utils.spec.js

-    var res = utils.lookupFiles(tmpFile('mocha-utils*'), ['js'], false)
-      .map(path.normalize.bind(path));
+  describe('.files', function () {
+    (symlinkSupported ? it : it.skip)('should not choke on broken symlinks', function () {


One of the test descriptions specifies "broken" and the other does not; these should be consistent, since it's substantially the same test for the two different functions.

…ymlinks later on

villesau · 2016-11-16T18:49:36Z

@ScottFreeCode @boneskull changed the logic to not to hide the broken symlink.

ScottFreeCode · 2016-11-24T06:30:48Z

Something interesting that may be relevant: I tried this out, and it turns out that broken symlinks to files with this change actually are watched -- as in, if you later create the file to which the symlink should have been pointing, it triggers the test suite to be re-run. So, except for the case of directories (where, I think, villesau has a good point that adding more files after watch start would be inconsistent with current watch behavior in general), this actually seems to fix the behavior altogether, if I'm understanding correctly. (Unless the concern is that the symlink might be broken because it's got the wrong name for a file that does exist but isn't being watched because the symlink to it is broken?) @boneskull What do you think? (GitHub still lists your review as outstanding, by the way.)

boneskull · 2016-11-24T08:23:28Z

@ScottFreeCode thanks for the heads-up.

If a symlink was intended to be watched, is broken, and then is fixed while watching, this is arguably "inconsistent" with the watch behavior elsewhere.

But fixing a broken symlink is not conceptually the same as adding a file.

I don't want to overthink this too much. LGTM

@villesau thanks!

* rename lookup-files.spec.js to file-utils.spec.js * .files should not choke on broken symlinks

villesau mentioned this pull request Oct 14, 2016

--watch crashes mocha on startup #2535

Closed

villesau force-pushed the ignore-failure-when-file-not-found branch from d951fb6 to 9b26253 Compare October 14, 2016 09:01

boneskull suggested changes Oct 15, 2016

View reviewed changes

boneskull added status: waiting for author waiting on response from OP - more information needed pr-needs-work labels Oct 15, 2016

villesau force-pushed the ignore-failure-when-file-not-found branch from b7cac2c to 15654ca Compare November 10, 2016 18:32

rename lookup-files.spec.js to file-utils.spec.js

5bb938c

villesau force-pushed the ignore-failure-when-file-not-found branch from 15654ca to e5ce5c7 Compare November 10, 2016 18:41

ScottFreeCode suggested changes Nov 14, 2016

View reviewed changes

Ville Saukkonen added 2 commits November 15, 2016 13:09

.files should not choke on broken symlinks

20f56ca

remove check for file existence to allow require to choke on broken s…

ab3d1c0

…ymlinks later on

villesau force-pushed the ignore-failure-when-file-not-found branch from e5ce5c7 to ab3d1c0 Compare November 15, 2016 11:24

ScottFreeCode approved these changes Nov 24, 2016

View reviewed changes

boneskull approved these changes Nov 24, 2016

View reviewed changes

boneskull merged commit 4a2e85a into mochajs:master Nov 24, 2016

villesau changed the title ~~ignore files files which are not found by files()~~ ignore files which are not found by files() Nov 24, 2016

boneskull removed status: waiting for author waiting on response from OP - more information needed pr-needs-work labels Dec 12, 2017

sgilroy pushed a commit to TwineHealth/mocha that referenced this pull request Feb 27, 2019

ignore files files which are not found by files() (mochajs#2538)

6ab3afc

* rename lookup-files.spec.js to file-utils.spec.js * .files should not choke on broken symlinks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ignore files which are not found by files() #2538

ignore files which are not found by files() #2538

villesau commented Oct 14, 2016 •

edited

boneskull left a comment

boneskull Oct 15, 2016

villesau Oct 16, 2016 •

edited

jsf-clabot commented Nov 10, 2016 •

edited

villesau commented Nov 13, 2016

ScottFreeCode left a comment

ScottFreeCode Nov 14, 2016

villesau Nov 15, 2016

ScottFreeCode Nov 14, 2016

ScottFreeCode Nov 14, 2016

ScottFreeCode Nov 14, 2016

villesau commented Nov 16, 2016

ScottFreeCode commented Nov 24, 2016 •

edited

boneskull commented Nov 24, 2016

ignore files which are not found by files() #2538

ignore files which are not found by files() #2538

Conversation

villesau commented Oct 14, 2016 • edited

boneskull left a comment

Choose a reason for hiding this comment

boneskull Oct 15, 2016

Choose a reason for hiding this comment

villesau Oct 16, 2016 • edited

Choose a reason for hiding this comment

jsf-clabot commented Nov 10, 2016 • edited

villesau commented Nov 13, 2016

ScottFreeCode left a comment

Choose a reason for hiding this comment

ScottFreeCode Nov 14, 2016

Choose a reason for hiding this comment

villesau Nov 15, 2016

Choose a reason for hiding this comment

ScottFreeCode Nov 14, 2016

Choose a reason for hiding this comment

ScottFreeCode Nov 14, 2016

Choose a reason for hiding this comment

ScottFreeCode Nov 14, 2016

Choose a reason for hiding this comment

villesau commented Nov 16, 2016

ScottFreeCode commented Nov 24, 2016 • edited

boneskull commented Nov 24, 2016

villesau commented Oct 14, 2016 •

edited

villesau Oct 16, 2016 •

edited

jsf-clabot commented Nov 10, 2016 •

edited

ScottFreeCode commented Nov 24, 2016 •

edited