@mark-wiemer is using new Date() allowed in the CLI code? An ESLint rule complains about it. If not is there any way I can get the current time to compare to file timestamps?

Also, let me know if you have any opinions about if/how I should test this change. It would be nice if I could inject some kind of chokidar mock so I can control timing of the file events to be able to verify in a test that it runs before the ready event is emitted...but the only easy way I think of to do that would be to call things in watch-run.js directly from the test process instead of executing mocha --watch in a separate process.

Marking as draft until tests are added

@mark-wiemer I want to run some changes by you before I dive into them...

I realized that GlobFilesTracker synchronously globs for the watched/ignored files, which mostly defeats the purpose of starting tests before the watcher is ready, since all that globbing would take about as long. So I think we should make it glob asynchronously.

Also...

• It globs each watch pattern and each ignore pattern separately...but it seems like we can just pass all of the watch and ignore patterns to one glob call?
• The existing code re-globs everything when the watcher gets a path added event, which could be a lot of redundant work...instead I think we should check if the added path is already in the watched or ignored set, and if not and it's a directory, initiate a glob on just that directory?

Hey @jedwards1211, the current glob code is not ideal for sure for exactly the reasons you mentioned and probably more. The best discussion around this is #5379 and in short we don't want to touch this code until v12. We as maintainers don't have a ton of experience with glob matching and don't want to break anything, but we do want to keep Chokidar at v4. Mocha 11.2.1 used Chokidar v3, we upgraded to v4 in Mocha 11.2.2, but Chokidar v4 dropped glob support so we had to use our own. Learning more about globs is definitely a priority, but I'm still getting the ropes on the rest of Mocha as a new maintainer, so I won't have a ton of updates here.

In short, feel free to update or change or even rewrite the glob handling. Just keep #5379 in mind as another alternative and add as many tests as you can to help me and other maintainers verify any changes :)

Related issues include #5355 and #5374

Yeah I kind of thought about doing the same thing as that PR.

One question is how to change blastCache - if we start and possibly even rerun before the watcher is finished scanning, we can't rely on getting the watched paths from chokidar. I already had to fix this in this PR, by getting the paths to delete from the GlobFilesTracker.

Instead we would have to delete all require.cache paths matching the filter. I worry a bit about how well that would perform compared to just deleting the list of matched paths we know about though. If there's a long list of watch patterns it could be a lot of work determining matches.

I wouldn't worry about performance too much in the abstract. We had a concrete issue with #5374 that has effectively been resolved with #5379, and in practice I can't imagine folks are using many regexes for watch paths. If we get a bug report about perf, we can look into it, but as long as our basic stress tests pass, I'd prefer simpler changes first.

On another note, @mochajs/maintenance-crew I'm thinking of marking this as "semver-major" given the complexity impacted and possibility of bugs. Since we want Mocha 12 to have minimal changes we'll probably hold this one for Mocha 13. (#5357)

Okay I guess I'll eventually piggyback this on #5379 and blast the cache with the new filtering from that PR.

Also, I'm not sure what to do about getting the launch time to compare file mtimes to. Date etc could get monkeypatched by test utils like Sinin fake timers... do you have any thoughts?

I guess it won't get monkeypatched before we run global setup though. But I got an ESLint error about using Date, it must be for good reason.

I'm not sure why our linter warns about using Date. Could you see if that's a recommended rule? There's a real chance that our linter is very out of date. We've had some discussions about it but haven't prioritized updates there for a while.

@mark-wiemer looks like the rule was explicitly added to guard against exactly the danger of fake timers I was talking about: #237

(This issue is explicitly linked in your ESLint config)

Note, it would probably be possible to grab a reference to the original Date constructor, Date.now() before it can get monkeypatched, and use that if necessary throughout Mocha.

As I was saying though, in this case I only need to get the launch date before running any global setup where userland code might monkeypatch Date, so I guess it's okay to suppress the error in this case

Thanks for researching. The core concern still applies, but I think the workaround does too: just have Date = global.Date at the top-level and then call Date() within your function as needed. Sinon mocks global.Date once the tests start running, but if we do this at top-level, it's done before the tests are run. As long as we never call global.Date() within a function we are good. I think... once the code is ready I'll definitely ping other maintainers to review it!

👋 Just checking in @jedwards1211, are you waiting on us for anything?

No, I'll pick this back up soon now that that related PR is merged

@mark-wiemer okay, to test timing-sensitive cases, I used an IPC channel to send mock chokidar events and tell the mocha process when to pretend global setup is done. I turned this mode on by passing a MOCHA_TEST_WATCH environment variable, but I'm not sure if you'd want this...if not how do you want me to accomplish the mocking?

okay I'll outline more of a proposal in an issue soon

https://github.com/mochajs/mocha/actions/runs/22022724150/job/63634561899?pr=5409 technically shows one new failure, but I'm not concerned. As always, ref #5361.

huh okay, I just merged main again, if it still fails I'll see if I can figure out why

All checks passed :) I've reached out to @mochajs/maintenance-crew to prioritize this one, thanks for keeping an eye on it!

On another note, @mochajs/maintenance-crew I'm thinking of marking this as "semver-major" given the complexity impacted and possibility of bugs.

I agree, this is a lot of really intricate stuff. Mocha 11 is very stable. No matter how thoroughly this PR is tested, the nature of its deep changes is that some folks will be broken by it.

Since we want Mocha 12 to have minimal changes we'll probably hold this one for Mocha 13. (#5357)

How about we wait until Mocha 12.1 or 12.2? Given that this is technically only bugfixes (albeit with real behavioral changes), it'd be nice to land it sooner. Giving folks a 12.0.x line with the first round of 12 stability, then following up with a subsequent minor release, means we'd get it in front of folks sooner.

@jedwards1211 we finally did it :) thank you so much for the initiative and patience here, this is great work that will be a major celebration in the Mocha 12 announcement :)

Oh no, @JoshuaKGoldberg I did kinda merge this one, would you like me to revert? 😬 I checked for a status: blocked tag or similar and didn't see one, we should align on how to mark PRs as "approved but not ready to merge for X reason," my bad!

I'm going to revert to reduce the risk of merge conflicts overall. We will get this in for 12.x pretty early, I will handle merge conflicts, assuming I'm not super sick again. Feels bad to revert but it's for the greater good!

Oop! Yeah a revert makes sense. Agreed.

Yay! Thank y'all for being open to this :)

I've been pretty swamped with work lately, so I haven't had time to work on the CJS/ESM errors yet. Hopefully soon...