-
-
Notifications
You must be signed in to change notification settings - Fork 3k
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
chore: fully clear require/import cache in non-parallel watch mode #5155
Changes from 4 commits
2615a18
55d042e
24aaa00
7f26597
9df2bf1
8dfc1b4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,38 @@ | ||
'use strict'; | ||
const fs = require('fs'); | ||
const path = require('path'); | ||
const debug = require('debug')('mocha:cli:handle:requires'); | ||
const {requireOrImport} = require('../nodejs/esm-utils'); | ||
const PluginLoader = require('../plugin-loader'); | ||
|
||
/** | ||
* `require()` the modules as required by `--require <require>`. | ||
* | ||
* Returns array of `mochaHooks` exports, if any. | ||
* @param {string[]} requires - Modules to require | ||
* @returns {Promise<object>} Plugin implementations | ||
* @private | ||
*/ | ||
exports.handleRequires = async (requires = [], {ignoredPlugins = []} = {}) => { | ||
const pluginLoader = PluginLoader.create({ignore: ignoredPlugins}); | ||
for await (const mod of requires) { | ||
let modpath = mod; | ||
// this is relative to cwd | ||
if (fs.existsSync(mod) || fs.existsSync(`${mod}.js`)) { | ||
modpath = path.resolve(mod); | ||
debug('resolved required file %s to %s', mod, modpath); | ||
} | ||
const requiredModule = await requireOrImport(modpath); | ||
if (requiredModule && typeof requiredModule === 'object') { | ||
if (pluginLoader.load(requiredModule)) { | ||
debug('found one or more plugin implementations in %s', modpath); | ||
} | ||
} | ||
debug('loaded required module "%s"', mod); | ||
} | ||
const plugins = await pluginLoader.finalize(); | ||
if (Object.keys(plugins).length) { | ||
debug('finalized plugin implementations: %O', plugins); | ||
} | ||
return plugins; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,9 @@ const path = require('path'); | |
const chokidar = require('chokidar'); | ||
const Context = require('../context'); | ||
const collectFiles = require('./collect-files'); | ||
|
||
const {handleRequires} = require('./handle-requires'); | ||
const {setCacheBusterKey} = require('../nodejs/esm-utils'); | ||
const {uniqueID} = require('../utils'); | ||
/** | ||
* Exports the `watchRun` function that runs mocha in "watch" mode. | ||
* @see module:lib/cli/run-helpers | ||
|
@@ -97,9 +99,14 @@ exports.watchRun = (mocha, {watchFiles, watchIgnore}, fileCollectParams) => { | |
return createWatcher(mocha, { | ||
watchFiles, | ||
watchIgnore, | ||
beforeRun({mocha}) { | ||
async beforeRun({mocha}) { | ||
mocha.unloadFiles(); | ||
|
||
// Reload hooks. If not done, global hooks keep their state between watch runs, but test files always get a new | ||
// instance, making state mutation impossible via global hooks. | ||
const plugins = await handleRequires(mocha.options.require); | ||
mocha.options.rootHooks = plugins.rootHooks; | ||
|
||
// I don't know why we're cloning the root suite. | ||
const rootSuite = mocha.suite.clone(); | ||
|
||
|
@@ -254,16 +261,21 @@ const createRerunner = (mocha, watcher, {beforeRun} = {}) => { | |
// running. | ||
let runner = null; | ||
|
||
const blastFullCache = !mocha.options.parallel; | ||
|
||
// true if a file has changed during a test run | ||
let rerunScheduled = false; | ||
|
||
const run = () => { | ||
const run = async () => { | ||
try { | ||
mocha = beforeRun ? beforeRun({mocha, watcher}) || mocha : mocha; | ||
mocha = beforeRun ? (await beforeRun({mocha, watcher})) || mocha : mocha; | ||
runner = mocha.run(() => { | ||
debug('finished watch run'); | ||
runner = null; | ||
blastCache(watcher); | ||
if (blastFullCache) { | ||
setCacheBusterKey(uniqueID()); | ||
} | ||
blastCache(watcher, blastFullCache); | ||
if (rerunScheduled) { | ||
rerun(); | ||
} else { | ||
|
@@ -348,15 +360,26 @@ const eraseLine = () => { | |
/** | ||
* Blast all of the watched files out of `require.cache` | ||
* @param {FSWatcher} watcher - chokidar FSWatcher | ||
* @param {boolean} blastAll | ||
* @ignore | ||
* @private | ||
*/ | ||
const blastCache = watcher => { | ||
const files = getWatchedFiles(watcher); | ||
files.forEach(file => { | ||
delete require.cache[file]; | ||
}); | ||
debug('deleted %d file(s) from the require cache', files.length); | ||
const blastCache = (watcher, blastAll) => { | ||
if (blastAll) { | ||
Object.keys(require.cache) | ||
// Avoid deleting mocha binary (at minimum, breaks Mocha's watch tests) | ||
.filter(file => !file.includes('/mocha/bin/')) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Bug] What if someone has a structure like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 100% Agree. Narrowed the filter to the binary both when running Mocha's tests ( |
||
.forEach(file => { | ||
delete require.cache[file]; | ||
}); | ||
debug('deleted all files from the require cache'); | ||
} else { | ||
const files = getWatchedFiles(watcher); | ||
files.forEach(file => { | ||
delete require.cache[file]; | ||
}); | ||
debug('deleted %d file(s) from the require cache', files.length); | ||
} | ||
JoshuaKGoldberg marked this conversation as resolved.
Show resolved
Hide resolved
|
||
}; | ||
|
||
/** | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,11 @@ | ||
let flag = false; | ||
|
||
module.exports.getFlag = () => flag; | ||
|
||
module.exports.enableFlag = () => { | ||
flag = true; | ||
}; | ||
|
||
module.exports.disableFlag = () => { | ||
flag = false; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
const dependency = require('./lib/dependency-with-state'); | ||
|
||
module.exports = { | ||
mochaHooks: { | ||
beforeEach: () => { | ||
dependency.enableFlag(); | ||
} | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
const dependency = require('./lib/dependency-with-state'); | ||
|
||
it('checks and mutates dependency', () => { | ||
if (dependency.getFlag()) { | ||
throw new Error('test failed'); | ||
} | ||
// Will pass 1st run, fail on subsequent ones | ||
dependency.enableFlag(); | ||
}); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,17 @@ | ||
const dependency = require('./lib/dependency-with-state'); | ||
|
||
// Will fail 1st run, unless hook runs | ||
before(() => { | ||
dependency.disableFlag(); | ||
}); | ||
|
||
// Will pass 1st run, fail on subsequent ones, unless hook runs | ||
afterEach(() => { | ||
dependency.disableFlag(); | ||
}); | ||
|
||
it('hook should have mutated dependency', () => { | ||
if (!dependency.getFlag()) { | ||
throw new Error('test failed'); | ||
} | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah this is a good question... I'd personally lean towards always blasting the full cache.
...provided there isn't a noticeable performance hit. Have you observed any particular slowdowns trying this out?
We're still a swamped with picking the project up right now and other commitments, so we're not likely to be able to do a full performance dive anytime soon (#5027). If you can set that up to just get a quick baseline of whether this is bad, that'd be very helpful!
Note also that we wouldn't be able to merge this soon - we've got some more work to get through before we can start shipping
semver-major
changes.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About benchmarking: I ran it three times with the conditional blasting, and three times with the latest commit always blasting. It looks negligible, in both scenarios milliseconds go up and down a bit, but are in the same range. Around mid July I can do a more decent benchmark, with potentially a few hundreds of dependencies, if you wish.
Results (only
test/integration/options/watch.spec.js
):There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
About full-blast always: I think it is the best approach, but I wanted to both play it safe and gather your feedback first. Removed the conditional logic and now always doing a full cache blast. Let me know if you see anything to improve
About when to release a new major: No problem, there's no hurry, and worst case scenario could just use my fork's branch.
Thanks for keeping the library alive and maintained!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, sounds good to me! Full cache blasting is a 👍 from me. 🙂