Skip to content
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

Added unloading files feature from require cache #3388

Closed
wants to merge 8 commits into from
Closed

Added unloading files feature from require cache #3388

wants to merge 8 commits into from

Conversation

gytis-ivaskevicius
Copy link

Description of the Change

Added a feature which allows unloading test files from require cache.
This feature allows reusing data/objects since there is no need to create a new Mocha process.
(Feature is orientated to those that are accessing Mocha as a module, not CLI)

Alternate Designs

  1. The user would have to implement same logic in his own code
  2. The user would have to create a new Mocha process on each dataset modification

Why should this be in core?

  1. Because it allows reuse of objects
  2. Because it already has loadFiles function, so I would think that there should be a function that does the opposite.

Benefits

Better reusability, fewer headaches for programmers

Possible Drawbacks

None as far as I know

Applicable issues

This is just a small code enhancement, has no impact on current production code.

@jsf-clabot
Copy link

jsf-clabot commented May 22, 2018

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented May 22, 2018

Coverage Status

Coverage increased (+0.3%) to 90.307% when pulling d1902a2 on WithoutCaps:master into 0d95e3f on mochajs:master.

Copy link
Contributor

@plroebuck plroebuck left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that it has to be, but why doesn't the unloadFiles() implementation just call unloadFile() in files.forEach()?

@gytis-ivaskevicius
Copy link
Author

@plroebuck Fixed.
I did it in that way initially because I prefer to jump around the dynamic code as little as possible

mocha.addFile(filePath);
mocha.loadFiles();

expect(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can write something like this, which IMO is more appropriate:

expect(require.cache, 'to have property', require.resolve(filePath));

then later

expect(require.cache, 'not to have property', require.resolve(filePath));

});

describe('.unloadFiles()', function() {
it('should unload all test files from cache', function() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this test would be more meaningful if we added multiple files, then made the assertion they were all removed by looping through mocha.files.

@boneskull boneskull added type: feature enhancement proposal status: waiting for author waiting on response from OP - more information needed labels Jun 6, 2018
@boneskull
Copy link
Contributor

Hi, thanks for the PR.

The intent here seems to be to create a public API; methods shouldn't be marked private, and they should have complete JSDoc docstrings and be marked public.

Moreover, Mocha must consume this API somehow, or it simply adds maintenance burden with little benefit. The only place currently where it could and should be used is in bin/_mocha L607.

What's the use case for this, anyhow?

@boneskull boneskull added the semver-minor implementation requires increase of "minor" version number; "features" label Jun 6, 2018
@gytis-ivaskevicius
Copy link
Author

Unfortunately, I don't think that I would be able to use this feature in existing codebase, but what it does is: it improves public API and allows to reuse existing Mocha instance (In terms of when you need to change Mocha dataset)

@plroebuck
Copy link
Contributor

plroebuck commented Oct 7, 2018

@boneskull wrote:

Moreover, Mocha must consume this API somehow, or it simply adds maintenance burden with little benefit.

While I definitely understand this sentiment, Mocha.prototype.unloadFiles() would appear to be the missing half of the public API referenced here that allows programmatic reuse of the Mocha instance.

@WithoutCaps:
Having Mocha.prototype.unloadFiles() would seem perfectly adequate for retesting by itself.
I find the "per file" unload method much less useful for the public API myself; partial reloads
seem unnecessary. Is there a strong use case for Mocha.prototype.unloadFile(file)?

It would seem appropriate make the documentation for each of Mocha.prototype.unloadFiles(), Mocha.prototype.loadFiles(), and Mocha.prototype.run() reference each other, à la...

/**
 * Unloads registered files.
 * This will remove them from Node's `require` cache, allowing them to be safely reloaded.
 *
 * @public
 * @see {@link Mocha#loadFiles}
 * @see {@link Mocha#run}
 */
Mocha.prototype.unloadFiles = function() {

@plroebuck
Copy link
Contributor

plroebuck commented Oct 7, 2018

@boneskull

Tangentially related, I find no usage of the fn argument to Mocha.prototype.loadFiles(). It was added in a commit seemingly unrelated (by its title) to Mocha API changes. Since the method is noted as private access, fn should be removed.

@plroebuck
Copy link
Contributor

See PR #3534.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor implementation requires increase of "minor" version number; "features" status: waiting for author waiting on response from OP - more information needed type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants