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.

Sign up for GitHub

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

Jump to bottom

Ignore all those folders mentioned in the gitignore. #1384

Merged
merged 3 commits into from
Jun 14, 2019
Merged

Ignore all those folders mentioned in the gitignore. #1384

merged 3 commits into from
Jun 14, 2019

Conversation

SparshithNR
Copy link
Member

  1. ignores node_modules and .git folder irrespective of list in .gitignore file
  2. gitignored files/folders can be conviniently ignored.

@jsf-clabot
Copy link

jsf-clabot commented Mar 14, 2019 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

@SparshithNR
Copy link
Member Author

For context about how much time this saves,
node_modules/.bin/qunit tests/*test.js 10.60s user 16.44s system 101% cpu 26.734 total
node node_modules/qunit/bin/qunit tests/*test.js 0.71s user 0.11s system 109% cpu 0.750 total

@Krinkle
Copy link
Member

Krinkle commented Mar 14, 2019

@SparshithNR Thanks for this patch.

Take a look at src/cli/run.js which has a IGNORED_GLOBS constant being used for a similar purpose. Could you see if it's possible to incorporate this improvement in that file instead?

@SparshithNR
Copy link
Member Author

SparshithNR commented Mar 14, 2019 •
edited
Loading

@Krinkle We can completely remove IGNORED_GLOBS from src/cli/run.js because it calls findFiles which is where we have our IGNORED_GLOBS getting merged with ignored list passed by the caller. We can keep everything in utils.js than in src/cli/run.js as IGNORED_GLOBS is used in utils.js.

@Krinkle
Copy link
Member

Krinkle commented Mar 14, 2019 •
edited
Loading

@SparshithNR I understand, but findFiles() is written and maintained as a generic utility. The use case for this PR is about how it is used in cli.js, so if possible, I think we should move the logic there.

@SparshithNR
Copy link
Member Author

That makes sense. I will update the PR accordingly.

@SparshithNR
Copy link
Member Author

Made new changes. Everything moved to run now.

@SparshithNR
Copy link
Member Author

Please review this @Krinkle

trentmwillis
trentmwillis previously requested changes May 8, 2019
View reviewed changes
Copy link
Member

@trentmwillis trentmwillis left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing this. Some small changes suggested. After that, looks good to me. @Krinkle do you want to take another look?

src/cli/run.js Outdated Show resolved Hide resolved
src/cli/utils.js Outdated Show resolved Hide resolved
@SparshithNR
Ignore all those folders mentioned in the gitignore.
893089a 
1) ignore node_modules and .git folder always
2) Added test for new function
3) added sample .gitignore file in fixtures folder testing
Krinkle
Krinkle reviewed Jun 13, 2019
View reviewed changes
test/cli/fixtures/.gitignore Outdated
@@ -0,0 +1,2 @@
/abcd
/efgh
Copy link
Member

Choose a reason for hiding this comment

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

Include the standard line break at the end of the file :)

@Krinkle Krinkle dismissed trentmwillis’s stale review June 14, 2019 00:01

Both suggestions have been applied.

@Krinkle Krinkle merged commit 837af39 into qunitjs:master Jun 14, 2019
Krinkle added a commit that referenced this pull request Aug 21, 2020
@Krinkle
CLI: Improve startup time by applying ignore list
a5ce447 
This follows-up 837af39, which introduces the ignore list to
the `--watch` mode when `run.restart()` calls `findFiles()`.

However, it did not use it when running `qunit` normally on
a file pattern. This meant that `qunit 'test/*.js'` generally
still ended up scanning much of node_modules.

Ref #1384.
Krinkle added a commit that referenced this pull request Aug 21, 2020
@Krinkle
CLI: Reduce startup by using tiny-glob to only scan relevant paths
0f48bb8 
This follows-up 837af39, which introduced the ignore list to
the `--watch` mode when `run.restart()` calls `findFiles()`.

This was a big improvement for watch mode, but it still left two
major issues behind:

1. It did not apply to when running qunit normally (without --watch).
   This could be easily fixed by passing IGNORED_GLOBS in run()
   down to getFilesFromArgs/findFiles the same way.

2. We are still scanning all other top-level directories that are
   not ignored, even if they do not match any part of the glob pattern.

I investigated numerous matching libraries (picomatch, minimatch, etc.)
but it seems none of of them offer a way to determine whether
a given directory could contain a matching file. E.g. something
that returns false for `src/` when given `test/*.js`, but returns
true for `src/` if given `**/*.js`.

So rather than approaching it from the angle of a matching library,
I went looking for various "proper" glob libraries instead which
handle both the pattern matching and the directory scanning
responsibilities together.

I considered:

* isaacs/node-glob <http://npm.broofa.com/?q=glob@7.1.6>.
  10 dependencies, including 3 for minimatch. Not an option.

* mrmlnc/fast-glob <http://npm.broofa.com/?q=fast-glob@3.2.4>
  This uses picomatch, which is promising as being a popular
  dependency-free alternative to minimatch.
  But, it unfortunately does add up to 16 dependencies in total.

* Crafity/node-glob <http://npm.broofa.com/?q=node-glob@1.2.0>
  A mostly self-contained implementation with 2 dependencies
  ('async' and 'glob-to-regexp'). Unfortunately, it seems to
  not do a limited traversal but rather

* terkelg/tiny-glob <http://npm.broofa.com/?q=tiny-glob@0.2.6>
  Another self-contained implementation, with two local
  dependencies by the same author. Claims to be much faster
  than both fast-glob and node-glob. I think we have a winner.

Ref #1384.
Krinkle added a commit that referenced this pull request Aug 21, 2020
@Krinkle
CLI: Reduce startup by using tiny-glob to only scan relevant paths
5629793 
This follows-up 837af39, which introduced the ignore list to
the `--watch` mode when `run.restart()` calls `findFiles()`.

This was a big improvement for watch mode, but it still left two
major issues behind:

1. It did not apply to when running qunit normally (without --watch).
   This could be easily fixed by passing IGNORED_GLOBS in run()
   down to getFilesFromArgs/findFiles the same way.

2. We are still scanning all other top-level directories that are
   not ignored, even if they do not match any part of the glob pattern.

I investigated numerous matching libraries (picomatch, minimatch, etc.)
but it seems none of of them offer a way to determine whether
a given directory could contain a matching file. E.g. something
that returns false for `src/` when given `test/*.js`, but returns
true for `src/` if given `**/*.js`.

So rather than approaching it from the angle of a matching library,
I went looking for various "proper" glob libraries instead which
handle both the pattern matching and the directory scanning
responsibilities together.

I considered:

* isaacs/node-glob <http://npm.broofa.com/?q=glob@7.1.6>.
  10 dependencies, including 3 for minimatch. Not an option.

* mrmlnc/fast-glob <http://npm.broofa.com/?q=fast-glob@3.2.4>
  This uses picomatch, which is promising as being a popular
  dependency-free alternative to minimatch.
  But, it unfortunately does add up to 16 dependencies in total.

* Crafity/node-glob <http://npm.broofa.com/?q=node-glob@1.2.0>
  A mostly self-contained implementation with 2 dependencies
  ('async' and 'glob-to-regexp'). Unfortunately, it seems to
  not do a limited traversal but rather

* terkelg/tiny-glob <http://npm.broofa.com/?q=tiny-glob@0.2.6>
  Another self-contained implementation, with two local
  dependencies by the same author. Claims to be much faster
  than both fast-glob and node-glob. I think we have a winner.

Ref #1384.
Krinkle added a commit that referenced this pull request Aug 23, 2020
@Krinkle
CLI: Reduce startup by using tiny-glob to only scan relevant paths
fe6c67c 
This follows-up 837af39, which introduced the ignore list to
the `--watch` mode when `run.restart()` calls `findFiles()`.

This was a big improvement for watch mode, but it still left two
major issues behind:

1. It did not apply to when running qunit normally (without --watch).
   This could be easily fixed by passing IGNORED_GLOBS in run()
   down to getFilesFromArgs/findFiles the same way.

2. We are still scanning all other top-level directories that are
   not ignored, even if they do not match any part of the glob pattern.

I investigated numerous matching libraries (picomatch, minimatch, etc.)
but it seems none of of them offer a way to determine whether
a given directory could contain a matching file. E.g. something
that returns false for `src/` when given `test/*.js`, but returns
true for `src/` if given `**/*.js`.

So rather than approaching it from the angle of a matching library,
I went looking for various "proper" glob libraries instead which
handle both the pattern matching and the directory scanning
responsibilities together.

I considered:

* isaacs/node-glob <http://npm.broofa.com/?q=glob@7.1.6>.
  10 dependencies, including 3 for minimatch. Not an option.

* mrmlnc/fast-glob <http://npm.broofa.com/?q=fast-glob@3.2.4>
  This uses picomatch, which is promising as being a popular
  dependency-free alternative to minimatch.
  But, it unfortunately does add up to 16 dependencies in total.

* Crafity/node-glob <http://npm.broofa.com/?q=node-glob@1.2.0>
  A mostly self-contained implementation with 2 dependencies
  ('async' and 'glob-to-regexp'). Unfortunately, it seems to
  not do a limited traversal but rather

* terkelg/tiny-glob <http://npm.broofa.com/?q=tiny-glob@0.2.6>
  Another self-contained implementation, with two local
  dependencies by the same author. Claims to be much faster
  than both fast-glob and node-glob. I think we have a winner.

Ref #1384.
Krinkle added a commit that referenced this pull request Aug 23, 2020
@Krinkle
CLI: Reduce startup by using tiny-glob to only scan relevant paths
b7fb112 
This follows-up 837af39, which introduced the ignore list to
the `--watch` mode when `run.restart()` calls `findFiles()`.

This was a big improvement for watch mode, but it still left two
major issues behind:

1. It did not apply to when running qunit normally (without --watch).
   This could be easily fixed by passing IGNORED_GLOBS in run()
   down to getFilesFromArgs/findFiles the same way.

2. We are still scanning all other top-level directories that are
   not ignored, even if they do not match any part of the glob pattern.

I investigated numerous matching libraries (picomatch, minimatch, etc.)
but it seems none of of them offer a way to determine whether
a given directory could contain a matching file. E.g. something
that returns false for `src/` when given `test/*.js`, but returns
true for `src/` if given `**/*.js`.

So rather than approaching it from the angle of a matching library,
I went looking for various "proper" glob libraries instead which
handle both the pattern matching and the directory scanning
responsibilities together.

I considered:

* isaacs/node-glob <http://npm.broofa.com/?q=glob@7.1.6>.
  10 dependencies, including 3 for minimatch. Not an option.

* mrmlnc/fast-glob <http://npm.broofa.com/?q=fast-glob@3.2.4>
  This uses picomatch, which is promising as being a popular
  dependency-free alternative to minimatch.
  But, it unfortunately does add up to 16 dependencies in total.

* Crafity/node-glob <http://npm.broofa.com/?q=node-glob@1.2.0>
  A mostly self-contained implementation with 2 dependencies
  ('async' and 'glob-to-regexp'). Unfortunately, it seems to
  not do a limited traversal but rather

* terkelg/tiny-glob <http://npm.broofa.com/?q=tiny-glob@0.2.6>
  Another self-contained implementation, with two local
  dependencies by the same author. Claims to be much faster
  than both fast-glob and node-glob. I think we have a winner.

Ref #1384.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trentmwillis trentmwillis trentmwillis left review comments

@Krinkle Krinkle Krinkle approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SparshithNR @jsf-clabot @Krinkle @trentmwillis