chore: fully clear require/import cache in non-parallel watch mode #5155
Conversation
Kartones
force-pushed
the
clear-require-cache-in-watch-mode
branch
from
1b588e8 to
55d042e
Compare
|
Added 24aaa001911cd78da5dc38898f2443121491cfbf because testing the changes on a real big project, I got the following: [mocha] waiting for changes...
ATest
Error: Mocha instance is already disposed, it cannot be used again. |
This comment was marked as resolved.
This comment was marked as resolved.
|
7f26597 Does not touch ESM imports if the cache busting key is empty, and the busting logic takes into account |
There was a problem hiding this comment.
👏 What a fantastic investigation, thanks for sending this in @Kartones (and sorry for taking so long to get back to you)!
I'm in favor of this general change - I think it's a bug to not aggressively clear the cache between reruns. But (also posting in the issue):
- This is a major version breaking change and so won't land for a little while
- I'd want to also hear from @mochajs/maintenance-crew on what else we want to do
Requesting changes on the one [Bug].
Cheers!
lib/cli/watch-run.js
Outdated
| 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.
[Bug] What if someone has a structure like ./src/confusing/mocha/bin/hooks.js? There's nothing stopping folks from doing that and then reporting it as a bug. This'll need to be a more specific filter.
There was a problem hiding this comment.
100% Agree. Narrowed the filter to the binary both when running Mocha's tests (../bin/..) and when running as an installed dependency ../lib/..).
lib/cli/watch-run.js
Outdated
| @@ -254,16 +261,21 @@ const createRerunner = (mocha, watcher, {beforeRun} = {}) => { | |||
| // running. | |||
| let runner = null; | |||
|
|
|||
| const blastFullCache = !mocha.options.parallel; | |||
There was a problem hiding this comment.
An alternative is to always do a full blast, and then we can remove some conditional logic and simplify a bit.
Yeah this is a good question... I'd personally lean towards always blasting the full cache.
- It's odd for both contributors and users having different behaviors between modes
- Just in case, it'd be good to clamp down on this kind of issue whenever possible
...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.
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):
Conditionally blasting:
✔ reruns test when watched test file is touched (4017ms)
✔ reruns test when watched test file crashes (4012ms)
✔ reruns test when file matching --watch-files changes (4016ms)
✔ reruns test when file matching --watch-files is added (4017ms)
✔ reruns test when file matching --watch-files is removed (4015ms)
✔ does not rerun test when file not matching --watch-files is changed (4021ms)
✔ picks up new test files when they are added (4018ms)
✔ reruns test when file matching --extension is changed (4017ms)
✔ reruns when "rs\n" typed (4017ms)
✔ reruns test when file starting with . and matching --extension is changed (4021ms)
✔ ignores files in "node_modules" and ".git" by default (4023ms)
✔ ignores files matching --watch-ignore (4023ms)
✔ reloads test files when they change (4021ms)
✔ reloads test dependencies when they change (4024ms)
✔ reloads all required dependencies between runs (4023ms)
✔ reloads all required dependencies between runs when mutated from a hook (4021ms)
✔ respects --fgrep on re-runs (4028ms)
✔ should not leak event listeners (15039ms)
✔ reruns test when watched test file is touched (4019ms)
✔ reruns test when watched test file is crashed (4018ms)
✔ mochaHooks.beforeAll runs as expected (4021ms)
✔ mochaHooks.beforeEach runs as expected (4024ms)
✔ mochaHooks.afterAll runs as expected (4026ms)
✔ mochaHooks.afterEach runs as expected (4022ms)
✔ reruns test when watched test file is touched (4029ms)
✔ reruns test when watched test file crashes (4018ms)
✔ reruns test when file matching --watch-files changes (4029ms)
✔ reruns test when file matching --watch-files is added (4030ms)
✔ reruns test when file matching --watch-files is removed (4034ms)
✔ does not rerun test when file not matching --watch-files is changed (4022ms)
✔ picks up new test files when they are added (4029ms)
✔ reruns test when file matching --extension is changed (4016ms)
✔ reruns when "rs\n" typed (4013ms)
✔ reruns test when file starting with . and matching --extension is changed (4028ms)
✔ ignores files in "node_modules" and ".git" by default (4028ms)
✔ ignores files matching --watch-ignore (4019ms)
✔ reloads test files when they change (4033ms)
✔ reloads test dependencies when they change (4039ms)
✔ reloads all required dependencies between runs (4033ms)
✔ reloads all required dependencies between runs when mutated from a hook (4026ms)
✔ respects --fgrep on re-runs (4037ms)
✔ should not leak event listeners (15053ms)
✔ reruns test when watched test file is touched (4024ms)
✔ reruns test when watched test file is crashed (4030ms)
✔ mochaHooks.beforeAll runs as expected (4028ms)
✔ mochaHooks.beforeEach runs as expected (4032ms)
✔ mochaHooks.afterAll runs as expected (4024ms)
✔ mochaHooks.afterEach runs as expected (4042ms)
✔ reruns test when watched test file is touched (4042ms)
✔ reruns test when watched test file crashes (4012ms)
✔ reruns test when file matching --watch-files changes (4017ms)
✔ reruns test when file matching --watch-files is added (4036ms)
✔ reruns test when file matching --watch-files is removed (4024ms)
✔ does not rerun test when file not matching --watch-files is changed (4024ms)
✔ picks up new test files when they are added (4035ms)
✔ reruns test when file matching --extension is changed (4024ms)
✔ reruns when "rs\n" typed (4032ms)
✔ reruns test when file starting with . and matching --extension is changed (4025ms)
✔ ignores files in "node_modules" and ".git" by default (4025ms)
✔ ignores files matching --watch-ignore (4025ms)
✔ reloads test files when they change (4032ms)
✔ reloads test dependencies when they change (4032ms)
✔ reloads all required dependencies between runs (4036ms)
✔ reloads all required dependencies between runs when mutated from a hook (4032ms)
✔ respects --fgrep on re-runs (4049ms)
✔ should not leak event listeners (15054ms)
when in parallel mode
✔ reruns test when watched test file is touched (4026ms)
✔ reruns test when watched test file is crashed (4018ms)
with required hooks
✔ mochaHooks.beforeAll runs as expected (4024ms)
✔ mochaHooks.beforeEach runs as expected (4026ms)
✔ mochaHooks.afterAll runs as expected (4025ms)
✔ mochaHooks.afterEach runs as expected (4028ms)
Always blasting:
✔ reruns test when watched test file is touched (4026ms)
✔ reruns test when watched test file crashes (4022ms)
✔ reruns test when file matching --watch-files changes (4024ms)
✔ reruns test when file matching --watch-files is added (4026ms)
✔ reruns test when file matching --watch-files is removed (4025ms)
✔ does not rerun test when file not matching --watch-files is changed (4030ms)
✔ picks up new test files when they are added (4031ms)
✔ reruns test when file matching --extension is changed (4026ms)
✔ reruns when "rs\n" typed (4028ms)
✔ reruns test when file starting with . and matching --extension is changed (4024ms)
✔ ignores files in "node_modules" and ".git" by default (4023ms)
✔ ignores files matching --watch-ignore (4025ms)
✔ reloads test files when they change (4028ms)
✔ reloads test dependencies when they change (4025ms)
✔ reloads all required dependencies between runs (4025ms)
✔ reloads all required dependencies between runs when mutated from a hook (4039ms)
✔ respects --fgrep on re-runs (4024ms)
✔ should not leak event listeners (15055ms)
✔ reruns test when watched test file is touched (4012ms)
✔ reruns test when watched test file is crashed (4026ms)
✔ mochaHooks.beforeAll runs as expected (4026ms)
✔ mochaHooks.beforeEach runs as expected (4025ms)
✔ mochaHooks.afterAll runs as expected (4024ms)
✔ mochaHooks.afterEach runs as expected (4026ms)
✔ reruns test when watched test file is touched (4027ms)
✔ reruns test when watched test file crashes (4024ms)
✔ reruns test when file matching --watch-files changes (4025ms)
✔ reruns test when file matching --watch-files is added (4028ms)
✔ reruns test when file matching --watch-files is removed (4025ms)
✔ does not rerun test when file not matching --watch-files is changed (4023ms)
✔ picks up new test files when they are added (4031ms)
✔ reruns test when file matching --extension is changed (4025ms)
✔ reruns when "rs\n" typed (4027ms)
✔ reruns test when file starting with . and matching --extension is changed (4024ms)
✔ ignores files in "node_modules" and ".git" by default (4026ms)
✔ ignores files matching --watch-ignore (4024ms)
✔ reloads test files when they change (4025ms)
✔ reloads test dependencies when they change (4028ms)
✔ reloads all required dependencies between runs (4025ms)
✔ reloads all required dependencies between runs when mutated from a hook (4025ms)
✔ respects --fgrep on re-runs (4031ms)
✔ should not leak event listeners (15038ms)
✔ reruns test when watched test file is touched (4021ms)
✔ reruns test when watched test file is crashed (4026ms)
✔ mochaHooks.beforeAll runs as expected (4023ms)
✔ mochaHooks.beforeEach runs as expected (4026ms)
✔ mochaHooks.afterAll runs as expected (4025ms)
✔ mochaHooks.afterEach runs as expected (4023ms)
✔ reruns test when watched test file is touched (4018ms)
✔ reruns test when watched test file crashes (4018ms)
✔ reruns test when file matching --watch-files changes (4014ms)
✔ reruns test when file matching --watch-files is added (4015ms)
✔ reruns test when file matching --watch-files is removed (4016ms)
✔ does not rerun test when file not matching --watch-files is changed (4021ms)
✔ picks up new test files when they are added (4021ms)
✔ reruns test when file matching --extension is changed (4028ms)
✔ reruns when "rs\n" typed (4031ms)
✔ reruns test when file starting with . and matching --extension is changed (4025ms)
✔ ignores files in "node_modules" and ".git" by default (4025ms)
✔ ignores files matching --watch-ignore (4023ms)
✔ reloads test files when they change (4029ms)
✔ reloads test dependencies when they change (4025ms)
✔ reloads all required dependencies between runs (4017ms)
✔ reloads all required dependencies between runs when mutated from a hook (4028ms)
✔ respects --fgrep on re-runs (4038ms)
✔ should not leak event listeners (15049ms)
✔ reruns test when watched test file is touched (4023ms)
✔ reruns test when watched test file is crashed (4025ms)
✔ mochaHooks.beforeAll runs as expected (4032ms)
✔ mochaHooks.beforeEach runs as expected (4027ms)
✔ mochaHooks.afterAll runs as expected (4025ms)
✔ mochaHooks.afterEach runs as expected (4025ms)
There was a problem hiding this comment.
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.
Great, sounds good to me! Full cache blasting is a 👍 from me. 🙂
test/node-unit/esm-utils.spec.js
Outdated
There was a problem hiding this comment.
[Praise] The test coverage in this PR is fantastic, nicely done!
JoshuaKGoldberg
added
the
semver-major
BREAKING CHANGE: before only watched files were busted
|
I bring sad news. I tried the changes at work, where we have a complex setup, with many tests and a few hooks. With Mocha 10.4 it fails upon reload in watch mode (expected). When I tried with this PR's branch, it got in a loop of erroring and attempting to rerun the tests. I added some traces (and a base36 uniqueId per I think that the issue is that, as this PR places I will see if there is any alternative, but from a quick look, it does not look not easy, as Here is the full trace (with tiny redactions, mostly on paths), in case it sheds some light, or anyone has some idea: |
|
In the meantime, the quick solution to this cache issue, for those facing it, is to run in parallel-watch mode ( |
| // Avoid deleting mocha binary | ||
| .filter( | ||
| file => | ||
| !file.includes('mocha/bin/mocha.js') && |
There was a problem hiding this comment.
Also doing some tests, to avoid extra invalidations, this should be node_modules/mocha/. Confirmed that that single filtering works with Mocha tests, standard mocha dependency/package, and a custom repository & branch path (at least under PNPM).
JoshuaKGoldberg
changed the title
JoshuaKGoldberg
added
status: needs review
|
Thanks for the investigation in #5155 (comment) @Kartones. Just to confirm, that means this PR is blocked from landing, right? I'd think we'd want a test of some sort to reproduce the buggy behavior you're seeing + a codefix to prevent the behavior. |
|
Hi @JoshuaKGoldberg, Indeed, it is blocked, and even if it passed the tests (it might in local, it's been a while, so I don't remember), it is still not complete, does not solve the issue at all times. I'll try to find time and build at least a bigger reproduction scenario, but I can't promise that it'll be soon. So feel free to close this PR, and I'll come up with a newer one once I have the time to properly reproduce the failing scenario and some idea of a fix. |
PR Checklist
status: accepting prs<- no, but I filled it 😄Overview
After filling #5149, I had some time to try to narrow the problem.
When running mocha in non-parallel watch mode, we are not spawning new processes, so the initial require hooks get loaded once, but on reloads/reruns, if they contained some stateful dependency, each subsequent run gets a new set of mocha requires for the test files, but not those global hooks, so they keep their initial state, and effectively the tests now run with a different context and can't affect/read the global hooks.
This issue can only be avoided by watching also the root hooks and their dependencies, which in large projects is not always easy to do (and can lead to big watched path lists in large repositories).
Explained with code:
watchRun.createWatcher->beforeRundoesconst newMocha = new Mocha(mocha.options);which means that the global hooks (stored insidemocha.optionskeep their initial load's closure.watchRun.createRerunner->runperforms ablastCache(), but that blasting is only aware of watched files.blastCache()causes the tests to re-import/re-require, creating a new closure for their stateful dependency; If all files are watched, that's fine, but just by not watching the global hooks (I added a test for this scenario),beforeRuncreates thenewMochabutmocha.optionsare the same, effectively now having two scopes, one with the original global hooks and another for the test run.My changes fix this scenarios by:
handleRequires()to a separate file (else there would be a circular dependency betweenrun-helpersandwatch-run)blastCacheto do a full cache blastrequire.cache, but forimportthere's no alternative than to play around with the querystring, so it changes its key (default is"", so only changes for non-parallel watch reruns)watchRun.createWatcher->beforeRunso that it reloads the global hooks (viahandleRequires()), which means that that each rerun will have the same newmocha.optionsclosure as the tests it'll run.I've added two test scenarios:
beforeHook, and the test hasbeforeandafterEachhooks to mutate it again to attempt to make it fail, so will only pass if the global hook is reloaded and the test picks the same closure.As mentioned, I wanted to avoid full blasts when not needed, so parallel watch mode is not affected. An alternative is to always do a full blast, and then we can remove some conditional logic and simplify a bit.
Let me know if I should further explain something.