Skip to content

Commit 145344c

Browse files
committed
lookupFiles repeats various operations over and over. We can avoid alot of this overhead and improve the performance of big test suites.
To improve the performance, we normalize the file extensions once at the beginning and dont need to do it again. This means that we can avoid again normalizing the array in `hasMatchingFileExtension` (before: `hasMatchingExtname`). Also we avoid the performance bottleneck of Array.helper functions like `.map` and `.some()`. `fs.statSync` is called with `throwIfNoEntry`, which exists since node 14.17.0, set to `false`. We can avoid "expensive" errors and the try catch blocks. We just check if stat is undefined, and if so, well, the path has an issue. Using `recursive` and `withFileTypes` option of readdirsync, we can avoid using recursive calling of `lookupFiles`. Added `jsdoc` for better type resolution. Also if you would put `// @ts-check` at the beginning of the file, it would pass.
1 parent d643105 commit 145344c

File tree

2 files changed

+120
-67
lines changed

2 files changed

+120
-67
lines changed

lib/cli/lookup-files.js

Lines changed: 94 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,22 @@
11
'use strict';
2+
23
/**
34
* Contains `lookupFiles`, which takes some globs/dirs/options and returns a list of files.
45
* @module
56
* @private
67
*/
78

8-
var fs = require('node:fs');
9-
var path = require('node:path');
10-
var glob = require('glob');
11-
var errors = require('../errors');
12-
var createNoFilesMatchPatternError = errors.createNoFilesMatchPatternError;
13-
var createMissingArgumentError = errors.createMissingArgumentError;
9+
const fs = require('node:fs');
10+
const path = require('node:path');
11+
const glob = require('glob');
12+
const {
13+
createNoFilesMatchPatternError,
14+
createMissingArgumentError
15+
} = require('../errors');
1416
const debug = require('debug')('mocha:cli:lookup-files');
1517

18+
/** @typedef {`.${string}`} HiddenUnixPathname */
19+
1620
/**
1721
* Determines if pathname would be a "hidden" file (or directory) on UN*X.
1822
*
@@ -24,30 +28,54 @@ const debug = require('debug')('mocha:cli:lookup-files');
2428
*
2529
* @private
2630
* @param {string} pathname - Pathname to check for match.
27-
* @return {boolean} whether pathname would be considered a hidden file.
31+
* @returns {value is HiddenUnixPathname} whether pathname would be considered a hidden file.
2832
* @example
2933
* isHiddenOnUnix('.profile'); // => true
3034
*/
31-
const isHiddenOnUnix = pathname => path.basename(pathname).startsWith('.');
35+
const isHiddenOnUnix = pathname => path.basename(pathname)[0] === '.';
36+
37+
/** @typedef {`.${string}`} FileExtension */
3238

3339
/**
34-
* Determines if pathname has a matching file extension.
40+
* Normalize file extensions to ensure they have a leading period character.
3541
*
36-
* Supports multi-part extensions.
42+
* @private
43+
* @param {FileExtension[]|string[]|undefined|null} exts
44+
* @returns {FileExtension[]}
45+
*/
46+
const normalizeFileExtensions = (exts) => {
47+
if (!exts) {
48+
return [];
49+
}
50+
51+
for (var i = 0; i < exts.length; i++) {
52+
if (exts[i][0] !== '.') {
53+
exts[i] = `.${exts[i]}`;
54+
}
55+
}
56+
return /** @type {FileExtension[]} */ (exts);
57+
}
58+
59+
/**
60+
* Determines if pathname has a matching file extension.
3761
*
3862
* @private
3963
* @param {string} pathname - Pathname to check for match.
40-
* @param {string[]} exts - List of file extensions, w/-or-w/o leading period
41-
* @return {boolean} `true` if file extension matches.
64+
* @param {FileExtension[]} fileExtensions - List of file extensions, w/-or-w/o leading period
65+
* @returns {boolean} `true` if file extension matches.
4266
* @example
43-
* hasMatchingExtname('foo.html', ['js', 'css']); // false
44-
* hasMatchingExtname('foo.js', ['.js']); // true
45-
* hasMatchingExtname('foo.js', ['js']); // ture
67+
* hasMatchingFileExtension('foo.html', ['js', 'css']); // false
68+
* hasMatchingFileExtension('foo.js', ['.js']); // true
69+
* hasMatchingFileExtension('foo.js', ['js']); // ture
4670
*/
47-
const hasMatchingExtname = (pathname, exts = []) =>
48-
exts
49-
.map(ext => (ext.startsWith('.') ? ext : `.${ext}`))
50-
.some(ext => pathname.endsWith(ext));
71+
const hasMatchingFileExtension = (pathname, fileExtensions) => {
72+
for (var i = 0; i < fileExtensions.length; i++) {
73+
if (pathname.endsWith(fileExtensions[i])) {
74+
return true;
75+
}
76+
}
77+
return false;
78+
}
5179

5280
/**
5381
* Lookup file names at the given `path`.
@@ -59,31 +87,30 @@ const hasMatchingExtname = (pathname, exts = []) =>
5987
* @public
6088
* @alias module:lib/cli.lookupFiles
6189
* @param {string} filepath - Base path to start searching from.
62-
* @param {string[]} [extensions=[]] - File extensions to look for.
90+
* @param {string[]} [fileExtensions=[]] - File extensions to look for.
6391
* @param {boolean} [recursive=false] - Whether to recurse into subdirectories.
64-
* @return {string[]} An array of paths.
92+
* @returns {string[]} An array of paths.
6593
* @throws {Error} if no files match pattern.
66-
* @throws {TypeError} if `filepath` is directory and `extensions` not provided.
94+
* @throws {TypeError} if `filepath` is directory and `fileExtensions` not
95+
* provided or an empty array.
6796
*/
6897
module.exports = function lookupFiles(
6998
filepath,
70-
extensions = [],
99+
fileExtensions,
71100
recursive = false
72101
) {
73102
const files = [];
74-
let stat;
103+
fileExtensions = normalizeFileExtensions(fileExtensions);
75104

105+
// Detect glob patterns by checking if the path does not exist as-is
76106
if (!fs.existsSync(filepath)) {
77107
let pattern;
78-
if (glob.hasMagic(filepath, {windowsPathsNoEscape: true})) {
108+
if (glob.hasMagic(filepath, { windowsPathsNoEscape: true })) {
79109
// Handle glob as is without extensions
80110
pattern = filepath;
81111
} else {
82112
// glob pattern e.g. 'filepath+(.js|.ts)'
83-
const strExtensions = extensions
84-
.map(ext => (ext.startsWith('.') ? ext : `.${ext}`))
85-
.join('|');
86-
pattern = `${filepath}+(${strExtensions})`;
113+
pattern = `${filepath}+(${fileExtensions.join('|')})`;
87114
debug('looking for files using glob pattern: %s', pattern);
88115
}
89116
files.push(
@@ -106,50 +133,51 @@ module.exports = function lookupFiles(
106133
return files;
107134
}
108135

109-
// Handle file
110-
try {
111-
stat = fs.statSync(filepath);
112-
if (stat.isFile()) {
113-
return filepath;
114-
}
115-
} catch (err) {
116-
// ignore error
117-
return;
118-
}
119-
120-
// Handle directory
121-
fs.readdirSync(filepath).forEach(dirent => {
122-
const pathname = path.join(filepath, dirent);
123-
let stat;
136+
const stat = fs.statSync(filepath, {
137+
throwIfNoEntry: false
138+
});
124139

125-
try {
126-
stat = fs.statSync(pathname);
127-
if (stat.isDirectory()) {
128-
if (recursive) {
129-
files.push(...lookupFiles(pathname, extensions, recursive));
130-
}
131-
return;
132-
}
133-
} catch (ignored) {
134-
return;
135-
}
136-
if (!extensions.length) {
140+
if (stat === undefined) {
141+
// Unreachable because glob check already checks if path exists, but for
142+
// completeness...
143+
} else if (stat.isFile()) {
144+
files.push(filepath);
145+
} else if (stat.isDirectory()) {
146+
if (fileExtensions.length === 0) {
137147
throw createMissingArgumentError(
138-
`Argument '${extensions}' required when argument '${filepath}' is a directory`,
148+
`Argument '${fileExtensions}' required when argument '${filepath}' is a directory`,
139149
'extensions',
140150
'array'
141-
);
151+
)
142152
}
143153

144-
if (
145-
!stat.isFile() ||
146-
!hasMatchingExtname(pathname, extensions) ||
147-
isHiddenOnUnix(pathname)
148-
) {
149-
return;
154+
// Handle directory
155+
const dirEnts = fs.readdirSync(filepath, { recursive, withFileTypes: true });
156+
157+
for (var i = 0; i < dirEnts.length; i++) {
158+
const dirEnt = dirEnts[i];
159+
160+
const pathname = dirEnt.parentPath
161+
? path.join(dirEnt.parentPath, dirEnt.name)
162+
: path.join(filepath, dirEnt.name);
163+
164+
if (dirEnt.isFile() || dirEnt.isSymbolicLink()) {
165+
if (dirEnt.isSymbolicLink()) {
166+
const stat = fs.statSync(pathname, { throwIfNoEntry: false });
167+
if (!stat || !stat.isFile()) {
168+
continue; // Skip broken symlinks or symlinks to directories
169+
}
170+
}
171+
172+
if (
173+
hasMatchingFileExtension(pathname, /** @type {FileExtension[]} */(fileExtensions)) &&
174+
!isHiddenOnUnix(pathname)
175+
) {
176+
files.push(pathname);
177+
}
178+
}
150179
}
151-
files.push(pathname);
152-
});
180+
}
153181

154182
return files;
155183
};

test/integration/file-utils.spec.js

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ describe('file utils', function () {
1717
tmpDir = result.dirpath;
1818
removeTempDir = result.removeTempDir;
1919

20-
tmpFile = filepath => path.join(tmpDir, filepath);
20+
tmpFile = (...args) => path.join(...[tmpDir, ...args]);
2121

2222
touchFile(tmpFile('mocha-utils.js'));
2323
if (SYMLINK_SUPPORT) {
@@ -64,6 +64,22 @@ describe('file utils', function () {
6464
ex.and('to have length', expectedLength);
6565
});
6666

67+
describe('when containing subdirectories', function () {
68+
beforeEach(function () {
69+
touchFile(tmpFile('types','mocha-utils.d.ts'));
70+
});
71+
72+
it('should find files in subdirectories', function () {
73+
expect(
74+
lookupFiles(tmpDir, ['.d.ts'], true).map(foundFilepath =>
75+
path.normalize(foundFilepath)
76+
),
77+
'to contain',
78+
tmpFile('types', 'mocha-utils.d.ts')
79+
).and('to have length', 1);
80+
});
81+
})
82+
6783
describe('when given `extension` option', function () {
6884
describe('when provided a directory for the filepath', function () {
6985
let filepath;
@@ -253,6 +269,15 @@ describe('file utils', function () {
253269
});
254270
});
255271

272+
describe('when provided path to non existing file', function () {
273+
it('should throw an exception', function () {
274+
expect(() => lookupFiles(tmpFile('mocha-utils-faux')), 'to throw', {
275+
name: 'Error',
276+
code: 'ERR_MOCHA_NO_FILES_MATCH_PATTERN'
277+
});
278+
});
279+
});
280+
256281
describe('when no files match', function () {
257282
it('should throw an exception', function () {
258283
expect(() => lookupFiles(tmpFile('mocha-utils')), 'to throw', {

0 commit comments

Comments
 (0)