-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
ignore files which are not found by files() #2538
ignore files which are not found by files() #2538
Conversation
d951fb6
to
9b26253
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's discuss this strategy further, or please implement my suggested changes if you're in agreement.
exports.files(path, ext, ret); | ||
} else if (path.match(re)) { | ||
ret.push(path); | ||
try { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
b7cac2c
to
15654ca
Compare
15654ca
to
e5ce5c7
Compare
@boneskull what do you think of current approach? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
).
exports.files(path, ext, ret); | ||
} else if (path.match(re)) { | ||
} else if (path.match(re) && existsSync(path)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using existsSync
here still silently ignores the problem rather than making the error message useful as discussed in the previous review, doesn't it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
describe('lookupFiles', function () { | ||
var tmpDir = path.join(os.tmpDir(), 'mocha-lookup-files'); | ||
describe('files utils', function () { | ||
var tmpDir = path.join(os.tmpDir(), 'mocha-files'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'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'
.
.to | ||
.eql([]); | ||
describe('.lookupFiles', function () { | ||
(symlinkSupported ? it : it.skip)('lookupFiles keke should not choke on symlinks', function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test description should omit "lookupFiles" (which is in the suite name/description, which is generally printed before the test name) and "keke".
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 () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
e5ce5c7
to
ab3d1c0
Compare
@ScottFreeCode @boneskull changed the logic to not to hide the broken symlink. |
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.) |
@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
fixes #2535 (comment)
broken symlinks caused file not found -errors and the system should not fail on those.