Skip to content

Commit

Permalink
CLI: Reduce startup by using tiny-glob to only scan relevant paths
Browse files Browse the repository at this point in the history
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.
  • Loading branch information
Krinkle committed Aug 21, 2020
1 parent e807d83 commit 5629793
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 74 deletions.
22 changes: 21 additions & 1 deletion package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"commander": "2.12.2",
"js-reporters": "1.2.1",
"node-watch": "0.6.4",
"picomatch": "2.2.2"
"tiny-glob": "0.2.6"
},
"devDependencies": {
"@babel/core": "7.11.1",
Expand Down
16 changes: 5 additions & 11 deletions src/cli/run.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,10 @@ const requireFromCWD = require( "./require-from-cwd" );
const requireQUnit = require( "./require-qunit" );
const utils = require( "./utils" );

const IGNORED_GLOBS = [
".git",
"node_modules"
].concat( utils.getIgnoreList( process.cwd() ) );

const RESTART_DEBOUNCE_LENGTH = 200;

const changedPendingPurge = [];

let QUnit;

function run( args, options ) {
Expand Down Expand Up @@ -108,12 +105,8 @@ run.restart = function( args ) {

this._restartDebounceTimer = setTimeout( () => {

const watchedFiles = utils.findFiles( process.cwd(), {
match: [ "**/*.js" ],
ignore: IGNORED_GLOBS
} );

watchedFiles.forEach( file => delete require.cache[ path.resolve( file ) ] );
changedPendingPurge.forEach( file => delete require.cache[ path.resolve( file ) ] );
changedPendingPurge.length = 0;

if ( QUnit.config.queue.length ) {
console.log( "Finishing current test and restarting..." );
Expand Down Expand Up @@ -158,6 +151,7 @@ run.watch = function watch() {
}
}, ( event, fullpath ) => {
console.log( `File ${event}: ${path.relative( baseDir, fullpath )}` );
changedPendingPurge.push( fullpath );
run.restart( args );
} );

Expand Down
75 changes: 23 additions & 52 deletions src/cli/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

const fs = require( "fs" );
const path = require( "path" );
const picomatch = require( "picomatch" );
const glob = require( "tiny-glob/sync" );

function existsStat() {
try {
Expand All @@ -22,75 +22,47 @@ function getIgnoreList( baseDir ) {
return [];
}

function findFilesInternal( dir, options, result = [], prefix = "" ) {
fs.readdirSync( dir ).forEach( ( name ) => {
const fullName = path.join( dir, name );
const stat = existsStat( fullName );
if ( !stat ) {
return;
}
const prefixedName = prefix + name;
const isIgnore = options.ignores( prefixedName );

if ( isIgnore ) {
return;
}
if ( stat.isDirectory() ) {
findFilesInternal( fullName, options, result, prefixedName + "/" );
} else {
const isMatch = options.matchers( prefixedName );
if ( isMatch ) {
result.push( prefixedName );
}
}
} );
return result;
}

function findFiles( baseDir, options ) {
return findFilesInternal( baseDir, {
matchers: picomatch( options.match || [] ),
ignores: picomatch( options.ignore || [] )
} );
}

function getFilesFromArgs( args ) {
const globs = args.slice();
const patterns = args.slice();

// Default to files in the test directory
if ( !globs.length ) {
globs.push( "test/**/*.js" );
if ( !patterns.length ) {
patterns.push( "test/**/*.js" );
}

const files = [];
const filteredGlobs = [];
const files = new Set();

// For each of the potential globs, we check if it is a directory path and
// update it so that it matches the JS files in that directory.
globs.forEach( glob => {
const stat = existsStat( glob );
patterns.forEach( pattern => {
const stat = existsStat( pattern );

if ( stat && stat.isFile() ) {

// Remember known files to avoid (slow) directory-wide glob scanning.
// Optimisation:
// For non-glob simple files, skip (slow) directory-wide scanning.
// https://github.com/qunitjs/qunit/pull/1385
files.push( glob );
} else if ( stat && stat.isDirectory() ) {
filteredGlobs.push( `${glob}/**/*.js` );
files.add( pattern );
} else {
filteredGlobs.push( glob );
if ( stat && stat.isDirectory() ) {
pattern = `${pattern}/**/*.js`;
}
const results = glob( pattern, {
cwd: process.cwd(),
filesOnly: true,
flush: true
} );
for ( const result of results ) {
files.add( result );
}
}
} );

if ( filteredGlobs.length ) {
files.push.apply( files, findFiles( process.cwd(), { match: filteredGlobs } ) );
}

if ( !files.length ) {
if ( !files.size ) {
error( "No files were found matching: " + args.join( ", " ) );
}

return files;
return Array.from( files ).sort();
}

function error( message ) {
Expand All @@ -103,7 +75,6 @@ function capitalize( string ) {
}

module.exports = {
findFiles,
capitalize,
error,
getFilesFromArgs,
Expand Down
18 changes: 9 additions & 9 deletions test/cli/fixtures/expected/tap-outputs.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ ok 1 Single > has a test

"qunit single.js double.js":
`TAP version 13
ok 1 Single > has a test
ok 2 Double > has a test
ok 3 Double > has another test
ok 1 Double > has a test
ok 2 Double > has another test
ok 3 Single > has a test
1..3
# pass 3
# skip 0
Expand Down Expand Up @@ -71,9 +71,9 @@ not ok 1 Throws match > bad

"qunit test single.js 'glob/**/*-test.js'":
`TAP version 13
ok 1 Single > has a test
ok 2 A-Test > derp
ok 3 Nested-Test > herp
ok 1 A-Test > derp
ok 2 Nested-Test > herp
ok 3 Single > has a test
ok 4 First > 1
ok 5 Second > 1
1..5
Expand All @@ -85,10 +85,10 @@ ok 5 Second > 1
"qunit --seed 's33d' test single.js 'glob/**/*-test.js'": `Running tests with seed: s33d
TAP version 13
ok 1 Second > 1
ok 2 Nested-Test > herp
ok 2 Single > has a test
ok 3 First > 1
ok 4 A-Test > derp
ok 5 Single > has a test
ok 4 Nested-Test > herp
ok 5 A-Test > derp
1..5
# pass 5
# skip 0
Expand Down

0 comments on commit 5629793

Please sign in to comment.