mocha no-ops when calling require() on file that is in node cache

[finnigantime]

Prerequisites

• Checked that your issue isn't already filed by cross referencing issues with the common mistake label
• Checked next-gen ES issues and syntax problems by using the same environment and/or transpiler configuration without Mocha to ensure it isn't just a feature that actually isn't supported in the environment in question or a bug in your code.
• 'Smoke tested' the code to be tested by running it outside the real test suite to get a better sense of whether the problem is in the code under test, your usage of Mocha, or Mocha itself
• Ensured that there is no discrepancy between the locally and globally installed versions of Mocha. You can find them with:
  node node_modules/.bin/mocha --version(Local) and mocha --version(Global). We recommend avoiding the use of globally installed Mocha.

Description

Mocha no-ops when calling require() on file that is in node cache. This occurs when using mocha programmatically. We have a test script that calls mocha twice - the first time, we don't actually run tests, we just build up the mocha suites and tests, and we run some filtering on them. We return the subset of files that include tests matching these filters. On the second time we call mocha, we actually run the tests on the set of matching files. However, in some node environments I am seeing require(file) pick up the file from the node cache instead of actually loading it and parsing it, so the require(file) call ends up being a no-op for mocha (mocha depends on the file being executed during require(file) to invoke describe() and it() statements and emit test code). Seems like this problem can be easily avoided by ensuring the file is not in the require cache before calling require(file).

Steps to Reproduce

Invoke mocha in two passes in a test script.

In the first pass, load files and look over mocha.suite.eachTest and do some filtering. Return a subset of files that we want to run tests on for this script run.

  mocha.files = files;
  mocha.loadFiles();

In the second pass, with a separate mocha instance, for each matched file we add it to mocha and then call mocha.run().

  mocha.addFile(file);
  mocha.run();

Expected behavior: In the second pass, mocha parses tests in the file and runs them.

Actual behavior: In the second pass, mocha no-ops.

Reproduces how often: 100% (in my node env - node v6.11.0)

Versions

mocha --version: 4.0.1
node --version: 6.11.0
OSX Sierra 10.12.6
zsh

Additional Information

N/A

[ORESoftware]

You may have to manually delete the cache for the file, I think it's

delete require.cache(file);

[ScottFreeCode]

There's an entire list of past discussions of this behavior at #2176 (comment), but I don't recall whether it was ever determined that clearing the cache can be done correctly by Mocha automatically.

[finnigantime]

@ORESoftware Yes - that's how I would propose mocha fix it - mocha is the one requiring the files and expecting that call to load and parse the files, so it makes sense to me that mocha would ensure the files it's going to call require on are not in the cache.

As a workaround, in our repo we are calling decache manually on the entire list of files we add to mocha.
(We had been calling decache() just on mocha but that broke in dwyl/decache#32 and was just fixed in dwyl/decache#34, and took a few hours to debug). But, IMO it's not great that consumers of mocha need to be aware they need to do this decaching. It seems like a leaky implementation detail of mocha to me.

What's the harm in this proposed fix (https://github.com/mochajs/mocha/pull/3085/files)? If we wanted to delete the decache dependency and just delete from the node cache more directly, that would be fine. I'm not familiar enough with nodejs to know why clearing the cache may not be able to be done automatically by Mocha, but explicitly purging the test modules we are going to call require() on seems to work fine for me - @ScottFreeCode are you worried about transitive dependencies or?

[ScottFreeCode]

@finnigantime Off the top of my head:

• Many people who want to use Mocha multiple times through the programmatic API are recreating --watch, but in those cases you specifically want your other, potentially updated modules to be refreshed too, so it needs to bust more than just the test files in at least one common use case.
• On the other hand, at least one user has declared a need to keep some files in the require cache, for whatever reason, so simply wiping the whole cache is unlikely to be OK for everyone unless there's some way to opt-out.

Therefore, any solution is probably going to have to be a little more sophisticated than just clearing some/all cache entries, if I'm not mistaken. (Mocha may be in control of the actual require call, but it's requireing the user's file's at the user's request, and users have different cases.) Whereas the status quo of leaving the cache-busting to the user...

• Lets them pick what works for them,
• Is consistent with behavior/usage outside of Mocha (as the cache is a Node feature -- tangentially, there's always risk of breaking people's expectations when changing the behavior of standard features), and
• Is hardly the only thing the user needs to know how to do right when using the programmatic API (documenting those things is another issue, of course).

That's not to say it's out of the question that Mocha could resolve this cleverly or even in a more brute-force manner (such as running in a virtual sandbox or busting the whole cache altogether, even if that does break some obscure use cases), it's just to say that (as far as I recall) "needing a PR that implements cache-busting" isn't the thing it's hung up on -- somebody's got to go through, find the various pieces of the issue among the many times it's been brought up and opinions weighed in, and demonstrate that a given solution would be adequate given all of those pieces. Unless I missed some recent resolution, that is.

Sorry it isn't simpler!

[finnigantime]

Hi @ScottFreeCode,

Thank you for taking the time to compose this thorough answer. I appreciate that this issue requires a more complete investigation before proceeding ahead with a "fix". I wonder if it may also make sense to add this as an opt-in config option if the concern is over the proposed change here being incomplete or breaking some other scenario. In any case, our team has knowledge of this behavior now and has worked around it externally, so we're good to move on - feel free to close the issue and PR I put up.

[ScottFreeCode]

I wonder if it may also make sense to add this as an opt-in config option if the concern is over the proposed change here being incomplete or breaking some other scenario.

Worth considering at least!

In any case, our team has knowledge of this behavior now and has worked around it externally, so we're good to move on...

If anyone reading this would like to put in some documentation updates to make this case clearer, it should be an easy contribution -- something like "Note that run (via loadFiles, which it calls) relies on Node's require to execute the test interface functions and will be subject to the cache (i.e. not re-run) if you use it twice; if you want to run tests multiple times, you may need to clear the cache before subsequent calls in whichever manner best suits your needs." -- since the programmatic API is pretty much undocumented on Mocha's readme and site, I guess it just belongs in the JSDoc in the source file and on the wiki page...

...feel free to close the issue and PR I put up.

Thanks! Although it is definitely something we'd like to improve if we can, I'll close this one to avoid further spacing out the discussion given the others linked above.

The PR on the other hand -- I think we have at least one other attempting to resolve this, so I'm not sure at this point they shouldn't just both stay open till we come to a decision. We might go through and close them all at whatever point we find time to refocus the issue, but I guess we'll cross that bridge when we come to it.

[finnigantime]

@ScottFreeCode Good idea. I updated the wiki here: https://github.com/mochajs/mocha/wiki/Using-mocha-programmatically and put up a PR to add a code comment: #3116