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

Ignore possible test files under node_modules folder. Fixes #226. #260

Closed
wants to merge 4 commits into from
Closed

Ignore possible test files under node_modules folder. Fixes #226. #260

wants to merge 4 commits into from

Conversation

sotojuan
Copy link
Contributor

As noted in #226, this pull request does two things:

  • Add !node_modules to the array https://github.com/sindresorhus/ava/blob/master/cli.js#L203, so that it ignores the folder when ava is ran with no arguments.
  • Filter out any matched file that has the string node_modules in it. This works in case the user does something like ava **, which is rare but can happen.

Perhaps there's a more efficient way to do this, but I have tested this in various cases and it works, though I haven't been able to write a proper test for it.

Thanks and let me know if there's anything I can improve on.

// ignore all possible test files in node_modules folder
files = files.filter(function (file) {
return (file.indexOf('node_modules') === -1);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This is code is actually before matched files are found.

At this point, files is not an actually a list of files, but a list of glob strings (like "**/*-test.js).

You may be seeing results on your machine here because your operating system is automatically expanding glob strings for you (I believe that is the case on OSX, but not on Windows).

@jamestalmage
Copy link
Contributor

You are on the right track. You need to expand on this using my comments from above.

You should also try to create a test or two. This will likely involve adding some files to the test\fixture folder. Look at some of the tests in test/cli.js for inspiration, and get creative from there.

A good way to verify you have written an effective test is to get rid of your changes to the production code (keep your tests in place). Your new tests should fail without your changes (otherwise what was the point?!), then add back in your changes and verify your changes to the production code take you from failing tests to passing tests.

Done this way, everybody can understand the need for the new code you propose, and your tests become a way of documenting the importance of your PR.

@@ -201,6 +201,7 @@ function init(files) {
function handlePaths(files) {
if (files.length === 0) {
files = [
'!node_modules',
Copy link
Member

Choose a reason for hiding this comment

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

I think we can use '!**/node_modules', to match node_modules at any level and not having to do the filtering below.

@sotojuan
Copy link
Contributor Author

Wouldn't that only work if the user runs ava with no arguments? When you specify files like in my test or have some sort of custom match (which was the source of this issue), it fails (just tested it).

@sindresorhus
Copy link
Member

@sotojuan With how this is currently implemented, yes, but it would work if you instead just always pushed it as the last item to the files array before passing it to globby.

@sotojuan
Copy link
Contributor Author

Thanks, !**/node_modules/** seems to work best if pushed before passing the files to globby.

@sindresorhus
Copy link
Member

@sotojuan That's what I said ;)

@sotojuan
Copy link
Contributor Author

Ha, I meant adding the extra /** at the end made it work!

@sindresorhus
Copy link
Member

Right, yeah.

@jamestalmage
Copy link
Contributor

👍

@sindresorhus
Copy link
Member

Landed. Very nice work @sotojuan ;)

687474703a2f2f7777772e7265616374696f6e676966732e636f6d2f77702d636f6e74656e742f67616c6c6572792f64616e63652d70617274792f74756d626c725f6d306f73727268714d6b31717a6376376e6f345f3235302e676966

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants