Skip to content

Commit 918523e

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 can normalize the file extensions once at the beginning and dont need to do it again if we have a recursive call. `normalizeFileExtensions` is a new function, which takes care of it once per run. 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. Also added fourth parameter for to `lookupFiles` which is the `files` parameter, passed through the recursive calls, avoiding the repetive spreads and and array generationsto to join the paths. 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 918523e

File tree

2 files changed

+129
-70
lines changed

2 files changed

+129
-70
lines changed

lib/cli/lookup-files.js

Lines changed: 103 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,17 @@
55
* @private
66
*/
77

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;
8+
const fs = require('node:fs');
9+
const path = require('node:path');
10+
const glob = require('glob');
11+
const {
12+
createNoFilesMatchPatternError,
13+
createMissingArgumentError
14+
} = require('../errors');
1415
const debug = require('debug')('mocha:cli:lookup-files');
1516

17+
/** @typedef {`.${string}`} HiddenUnixPathname */
18+
1619
/**
1720
* Determines if pathname would be a "hidden" file (or directory) on UN*X.
1821
*
@@ -24,30 +27,59 @@ const debug = require('debug')('mocha:cli:lookup-files');
2427
*
2528
* @private
2629
* @param {string} pathname - Pathname to check for match.
27-
* @return {boolean} whether pathname would be considered a hidden file.
30+
* @returns {value is HiddenUnixPathname} whether pathname would be considered a hidden file.
2831
* @example
2932
* isHiddenOnUnix('.profile'); // => true
3033
*/
31-
const isHiddenOnUnix = pathname => path.basename(pathname).startsWith('.');
34+
const isHiddenOnUnix = pathname => path.basename(pathname)[0] === '.';
35+
36+
/** @typedef {`.${string}`} FileExtension */
3237

3338
/**
34-
* Determines if pathname has a matching file extension.
39+
* Normalize file extensions to ensure they have a leading period character.
3540
*
36-
* Supports multi-part extensions.
41+
* @private
42+
* @param {FileExtension[]|string[]|undefined|null} exts
43+
* @param {boolean} normalized
44+
* @returns {FileExtension[]}
45+
*/
46+
const normalizeFileExtensions = (exts, normalized) => {
47+
if (normalized === true) {
48+
return /** @type {FileExtension[]} */ (exts)
49+
}
50+
51+
if (!exts) {
52+
return [];
53+
}
54+
55+
for (var i = 0; i < exts.length; i++) {
56+
if (exts[i][0] !== '.') {
57+
exts[i] = `.${exts[i]}`;
58+
}
59+
}
60+
return /** @type {FileExtension[]} */ (exts);
61+
}
62+
63+
/**
64+
* Determines if pathname has a matching file extension.
3765
*
3866
* @private
3967
* @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.
68+
* @param {FileExtension[]} fileExtensions - List of file extensions, w/-or-w/o leading period
69+
* @returns {boolean} `true` if file extension matches.
4270
* @example
43-
* hasMatchingExtname('foo.html', ['js', 'css']); // false
44-
* hasMatchingExtname('foo.js', ['.js']); // true
45-
* hasMatchingExtname('foo.js', ['js']); // ture
71+
* hasMatchingFileExtension('foo.html', ['js', 'css']); // false
72+
* hasMatchingFileExtension('foo.js', ['.js']); // true
73+
* hasMatchingFileExtension('foo.js', ['js']); // ture
4674
*/
47-
const hasMatchingExtname = (pathname, exts = []) =>
48-
exts
49-
.map(ext => (ext.startsWith('.') ? ext : `.${ext}`))
50-
.some(ext => pathname.endsWith(ext));
75+
const hasMatchingFileExtension = (pathname, fileExtensions) => {
76+
for (var i = 0; i < fileExtensions.length; i++) {
77+
if (pathname.endsWith(fileExtensions[i])) {
78+
return true;
79+
}
80+
}
81+
return false;
82+
}
5183

5284
/**
5385
* Lookup file names at the given `path`.
@@ -59,31 +91,33 @@ const hasMatchingExtname = (pathname, exts = []) =>
5991
* @public
6092
* @alias module:lib/cli.lookupFiles
6193
* @param {string} filepath - Base path to start searching from.
62-
* @param {string[]} [extensions=[]] - File extensions to look for.
94+
* @param {string[]} [fileExtensions=[]] - File extensions to look for.
6395
* @param {boolean} [recursive=false] - Whether to recurse into subdirectories.
64-
* @return {string[]} An array of paths.
96+
* @param {string[]} [files=[]] - The array of files found (used internally).
97+
* @param {boolean} [recursion=false] - Whether function is being called recursively (used internally).
98+
* @returns {string[]} An array of paths.
6599
* @throws {Error} if no files match pattern.
66-
* @throws {TypeError} if `filepath` is directory and `extensions` not provided.
100+
* @throws {TypeError} if `filepath` is directory and `fileExtensions` not
101+
* provided or an empty array.
67102
*/
68103
module.exports = function lookupFiles(
69104
filepath,
70-
extensions = [],
71-
recursive = false
105+
fileExtensions,
106+
recursive = false,
107+
files = [],
108+
recursion = false
72109
) {
73-
const files = [];
74-
let stat;
110+
fileExtensions = normalizeFileExtensions(fileExtensions, recursion);
75111

112+
// Detect glob patterns by checking if the path does not exist as-is
76113
if (!fs.existsSync(filepath)) {
77114
let pattern;
78115
if (glob.hasMagic(filepath, {windowsPathsNoEscape: true})) {
79116
// Handle glob as is without extensions
80117
pattern = filepath;
81118
} else {
82119
// glob pattern e.g. 'filepath+(.js|.ts)'
83-
const strExtensions = extensions
84-
.map(ext => (ext.startsWith('.') ? ext : `.${ext}`))
85-
.join('|');
86-
pattern = `${filepath}+(${strExtensions})`;
120+
pattern = `${filepath}+(${fileExtensions.join('|')})`;
87121
debug('looking for files using glob pattern: %s', pattern);
88122
}
89123
files.push(
@@ -106,50 +140,50 @@ module.exports = function lookupFiles(
106140
return files;
107141
}
108142

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-
}
143+
const stat = fs.statSync(filepath, {
144+
throwIfNoEntry: false
145+
});
146+
147+
if (stat === undefined) {
148+
// Unreachable because glob check already checks if path exists, but for
149+
// completeness...
150+
} else if (stat.isFile()) {
151+
files.push(filepath);
152+
} else if (stat.isDirectory()) {
153+
// Handle directory
154+
const dirEnts = fs.readdirSync(filepath);
119155

120-
// Handle directory
121-
fs.readdirSync(filepath).forEach(dirent => {
122-
const pathname = path.join(filepath, dirent);
123-
let stat;
156+
for (var i = 0; i < dirEnts.length; i++) {
157+
const pathname = path.join(filepath, dirEnts[i]);
158+
const subStat = fs.statSync(pathname, {
159+
throwIfNoEntry: false
160+
});
124161

125-
try {
126-
stat = fs.statSync(pathname);
127-
if (stat.isDirectory()) {
128-
if (recursive) {
129-
files.push(...lookupFiles(pathname, extensions, recursive));
162+
if (subStat === undefined) {
163+
continue;
164+
} else if (subStat.isDirectory()) {
165+
if (recursive === true) {
166+
lookupFiles(pathname, fileExtensions, recursive, files, true);
167+
}
168+
} else if (
169+
subStat.isFile()
170+
) {
171+
if (fileExtensions.length === 0) {
172+
throw createMissingArgumentError(
173+
`Argument '${fileExtensions}' required when argument '${filepath}' is a directory`,
174+
'extensions',
175+
'array'
176+
)
177+
}
178+
if (
179+
hasMatchingFileExtension(pathname, /** @type {FileExtension[]} */ (fileExtensions)) &&
180+
!isHiddenOnUnix(pathname)
181+
) {
182+
files.push(pathname);
130183
}
131-
return;
132184
}
133-
} catch (ignored) {
134-
return;
135-
}
136-
if (!extensions.length) {
137-
throw createMissingArgumentError(
138-
`Argument '${extensions}' required when argument '${filepath}' is a directory`,
139-
'extensions',
140-
'array'
141-
);
142185
}
143-
144-
if (
145-
!stat.isFile() ||
146-
!hasMatchingExtname(pathname, extensions) ||
147-
isHiddenOnUnix(pathname)
148-
) {
149-
return;
150-
}
151-
files.push(pathname);
152-
});
186+
}
153187

154188
return files;
155189
};

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)