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

Add ability to run tests in a mocha instance multiple times #4234

Merged
merged 19 commits into from
May 11, 2020

Conversation

nicojs
Copy link
Contributor

@nicojs nicojs commented Apr 21, 2020

Description of the Change

These changes allow mocha to run multiple times with the same test suite programmatically (without reload). It does this with minimal code changes.

It also adds to validations, for when people call mocha.run at the wrong time.

For more details and reasoning, please see #2783 (comment)

Fixes #2783

Alternate Designs

Alternatively, we could have totally removed all state from tests, suites and runnable, but we don't want to change the entire way of keeping state. Please see #2783 for the entire history.

Why should this be in core?

A lot of people seem interested in running a test suite multiple times without reloading files from disk. See #2783. I personally will use it to implement mutation switching in Stryker

Benefits

Huge performance gain for my mutation testing use case. Other people have valid use cases as well.

Possible Drawbacks

It should be backward compatible, but since I had to rewrite parts in the core, I might have introduced undesired side effects 🤷‍♂️. Although the integration test suite is awesome!

Applicable issues

#2783

Todo:

  • Test: implement reset
  • Suite: implement reset
  • Runnable: implement reset
  • Mocha: implement dispose and cleanReferencesAfterRun
  • Mocha: throw errors when run call is invalid.
  • Runner: make sure to not leak event listeners.
  • Runner: implement dispose and cleanReferencesAfterRun
  • Unit tests
  • Integration tests
  • Docs

@jsf-clabot
Copy link

jsf-clabot commented Apr 21, 2020

CLA assistant check
All committers have signed the CLA.

@coveralls
Copy link

coveralls commented Apr 22, 2020

Coverage Status

Coverage increased (+0.2%) to 93.268% when pulling f3f82c2 on nicojs:feat/hot-reload into 240cb3d on mochajs:master.

@nicojs nicojs marked this pull request as ready for review April 22, 2020 09:06
@nicojs
Copy link
Contributor Author

nicojs commented Apr 22, 2020

Coverage increased (+0.2%) to 93.213%

You're welcome 💁‍♂️

@nicojs
Copy link
Contributor Author

nicojs commented Apr 22, 2020

@boneskull the documentation website build failed because of an http error, not my doing. Please rerun that job if you can, thanks!

@afonsojramos
Copy link

I will be paying close attention to this PR, for now, it looks good!

@boneskull
Copy link
Contributor

Huh, I am not sure how to re-deploy this on netlify. I don't see the option.

* Implements listenerCount for 'uncaughtException'.
*/

process.listenerCount = function(name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

NOTE: this is an observation and not a request for changes.

I don't love how process is shimmed here. we might be able to get away with changing the prototype to the EventEmitter shim (loaded via browserify via events, because process should be an EventEmitter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I was surprised that that wasn't yet the case.

Copy link
Contributor

@boneskull boneskull left a comment

Choose a reason for hiding this comment

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

Thanks, great work here! good tests too.

I have a couple concerns which I've commented on, but overall looks excellent. If you're unable to do further work on it, I think it could land as-is, and I could take care of those myself later. I'd like to see if #4245 can consume it.

lib/errors.js Outdated Show resolved Hide resolved
lib/mocha.js Show resolved Hide resolved
lib/mocha.js Outdated Show resolved Hide resolved
lib/mocha.js Outdated Show resolved Hide resolved
lib/runner.js Outdated Show resolved Hide resolved
lib/runner.js Outdated Show resolved Hide resolved
lib/runner.js Show resolved Hide resolved
lib/suite.js Outdated
});
this.tests.forEach(function(test) {
test.reset();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

what of hooks?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure I know what you mean with hooks, but I think it is beforeEach and friends.

I super duper don't want them to be cleaned. They should stay, just like the test fn's themselves. We're going to need them for the next run.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, hooks are beforeEach etc. I think I am confused about why suites and tests are getting reset() called, but not hooks.

Copy link
Contributor

Choose a reason for hiding this comment

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

to be clear: I'm still confused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I now understand your criticism. Hooks have state as well. And they inherit from Runnable. Didn't realize. Will add it when I have some time. Thanks for pointing it out!

test/unit/runnable.spec.js Outdated Show resolved Hide resolved
@nicojs
Copy link
Contributor Author

nicojs commented Apr 24, 2020

I've implemented (most of) the requested changes.

I've also added one last bit. When disposed of, a Mocha instance will now unload files internally. That makes a lot of sense to me. If someone wants to create a new mocha instance in the same process and add the same files again, he should be able to.

@boneskull
Copy link
Contributor

ty for the update. looks like there's a conflict.

@nicojs
Copy link
Contributor Author

nicojs commented Apr 24, 2020

Conflict has gone 🤸‍♂️

@boneskull
Copy link
Contributor

@nicojs there's another conflict 😜

@nicojs
Copy link
Contributor Author

nicojs commented Apr 25, 2020

You're a busy guy. Fixed once more 🤸‍♂️

@nicojs
Copy link
Contributor Author

nicojs commented Apr 26, 2020

You're quite good at keeping me busy 🙊

I've implemented the reset on hooks. I saw that state and err could also be set on a test/hook during a run, so I've added those fields to Runnable.reset(). I've also solved the merge conflict.

As far as I know, I've fixed all issues/recommendations.

lib/errors.js Outdated
default:
throw new Error('unknown pluginType "' + pluginType + '"');
}

Choose a reason for hiding this comment

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

You seem to have missed a closing bracket.

Copy link
Contributor Author

@nicojs nicojs Apr 29, 2020

Choose a reason for hiding this comment

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

Yes, fixed now. Did a merge in the browser 🤷‍♂️

Fixed now. I'm hoping you guys do squash commit? If not, I could do it as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

yes, will squash.

@afonsojramos
Copy link

@boneskull can you take a look if it is good to go? 🚀

@boneskull
Copy link
Contributor

Seems to be yet another conflict.

@boneskull boneskull added type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels Apr 29, 2020
@boneskull
Copy link
Contributor

boneskull commented Apr 29, 2020

I would like to call this a "bugfix", since it really always should have worked properly. But I could see how others may consider this a feature. Thoughts, @mochajs/core?

EDIT: either way, this shouldn't be breaking.

@boneskull
Copy link
Contributor

Something's gone wonky with the changesets here.

I tried to rebase back onto master, but that didn't work, and I had more conflicts. then I did a merge via the GitHub UI to resolve those conflicts, and now I just have...more conflicts.

@nicojs Sorry, I tried to help. You'll need to pull your branch again from your fork, and pull master as well, then retry a rebase back onto master.

@boneskull
Copy link
Contributor

It looks as if there are changesets in here that shouldn't be in here;
66d9b29 and later. A rebase will need to ensure those changesets don't end up in this PR.

@nicojs I'll give you a chance, but please let me know if you want me to give it another go. I may have screwed it up myself!

Might even be best to not pull your branch back down, and instead just retry a rebase onto master from your working copy's branch.

Rename since we cannot dispose the entire mocha instance after a test run. We should keep the process.on('uncaughtException') handlers in place in order to not break existing functionality.
@boneskull
Copy link
Contributor

Will give @juergba 48h to look at this; if he declines to review I'll merge it

@boneskull boneskull added type: feature enhancement proposal semver-minor implementation requires increase of "minor" version number; "features" and removed type: bug a defect, confirmed by a maintainer semver-patch implementation requires increase of "patch" version number; "bug fixes" labels May 7, 2020
@boneskull
Copy link
Contributor

This is one of those "missing features" that is only a bug because it was missing. 😄

@nicojs
Copy link
Contributor Author

nicojs commented May 8, 2020

True.

To be fair, I think we same problem in Stryker itself. Creating an instance of Stryker and running mutation testing twice is probably not working 🙄 Just don't tell anyone 🤐

@nicojs
Copy link
Contributor Author

nicojs commented May 11, 2020

So... I guess this will be merged soon 🤗

@boneskull
Copy link
Contributor

yes

@boneskull boneskull merged commit fbe3ce4 into mochajs:master May 11, 2020
@boneskull
Copy link
Contributor

boneskull commented May 11, 2020

So it looks like I can't actually use this for the parallel runner. What I had hoped to do is re-use the same Mocha instance... except with different files (specifically, a single file) on every run.

@nicojs What I'm trying to do is:

  1. Initialize a Mocha instance. Set cleanReferencesAfterRun to false.
  2. Repeat for n files:
    1. Add a file to Mocha instance
    2. call Mocha#run()
    3. Upon completion, "reset" Mocha and remove all files from the Mocha#files array.

I attempted to "reset" by calling dispose() upon completion (step 2.iii.). But this fails at the next run, because:

Mocha instance is already disposed, cannot start a new test run. Please create a new mocha instance. Be sure to set disable `cleanReferencesAfterRun` when you want to reuse the same mocha instance for multiple test runs

Of course, if I don't call dispose(), things get really confused due to extra hooks hanging around.

Why can't I reuse the Mocha instance after dispose()?

@boneskull
Copy link
Contributor

Should I just not call cleanReferencesAfterRun? Would it work then?

@boneskull
Copy link
Contributor

(I suppose I'm rubbber-ducking now...)

@boneskull
Copy link
Contributor

I think, maybe, that it wasn't the problem you were trying to solve. but it seems to get most of the way there.

@boneskull
Copy link
Contributor

OK, so, I am thinking it's really not worth the bother to try to reuse Mocha instances in the manner I'm trying (in #4245), even with your code now merged. Too many things to worry about. Like dumping files out of the module cache--but we're still running root-level hooks from those files, root-level hooks being applied nondeterministically, etc. I mean, it's possible, just difficult to do correctly (for some definition of correctly).

anyway, thanks for the PR 😄

@nicojs
Copy link
Contributor Author

nicojs commented May 12, 2020

Yeah, it wasn't the problem I was trying to solve. The reason I added the behavior of throwing an error on run after dispose was just because thought that made the most sense. Feel free to change that behavior, no harm done since it wasn't released yet, right? As long as I can run the exact same suite without reloading files, I'm happy ;)

@craigtaub craigtaub added this to the next milestone May 21, 2020
craigtaub pushed a commit that referenced this pull request May 21, 2020
…loses #2783

* Add ability to run tests in a mocha instance multiple times

* Rename `autoDispsoe` to `cleanReferencesAfterRun`,

Rename since we cannot dispose the entire mocha instance after a test run. We should keep the process.on('uncaughtException') handlers in place in order to not break existing functionality.

* Allow `unloadFiles` to reset `referencesCleaned`.

* Complete rename of _cleanReferencesAfterRun

* Add integration test for running a suite multiple times

* improve api docs

* Docs: fix dead link

* Make sure tests run on older node versions

* Remove `.only` 😅

* Implement `process.listenerCount` in the browser

* Implement mocha states in a finite state machine

* Fix some small remarks

* Make integration tests more damp

* Keep `Runner` api backward compatible

* Unload files when disposed

* Runnable.reset should also reset `err` and `state`

* Also reset hooks

Co-authored-by: Christopher Hiller <boneskull@boneskull.com>
@birtles
Copy link

birtles commented May 26, 2020

This appears to cause breakage for me:
https://github.com/birtles/rikaichamp/runs/707671695#step:5:71
I am getting createMochaInstanceAlreadyRunningError when trying to update mocha.

I haven't had a chance to debug why yet. Any tips would be appreciated.

wKich added a commit to creevey/creevey that referenced this pull request May 27, 2020
Fix issue with ability run tests in multiple times
mochajs/mocha#4234
wKich added a commit to creevey/creevey that referenced this pull request May 27, 2020
Fix issue with ability run tests in multiple times
mochajs/mocha#4234
@craigtaub
Copy link
Contributor

@birtles Please could you submit this bug as a new issue? Please include details of versions as well as how you are calling mocha. Thanks

@birtles
Copy link

birtles commented Jun 2, 2020

@birtles Please could you submit this bug as a new issue? Please include details of versions as well as how you are calling mocha. Thanks

Hi. I went to submit a bug but the bug form was requiring a little bit more time than I can currently afford, unfortunately. I discovered that dropping a call to mocha.run() resolved the issue for me. I have no idea why the call to mocha.run() was needed in the first place.

For what it's worth, I'm running with karma and karma-firefox-launcher.

@audioscavenger
Copy link

i updated the thread #2314 and I found out that there must be a bug in the options parser.
by forcing _cleanReferencesAfterRun = false after you instantiated a new mocha, it actually let you re-run your tests! lost 2 hours on this one. You're welcome.

var mocha = new Mocha(mochaOptions)
mocha._cleanReferencesAfterRun = false

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" type: feature enhancement proposal
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Mocha can't run tests twice programmatically
9 participants