Skip to content

Add ability to set MOCHA_GLOBAL_TIMEOUT, without success.#33

Closed
workflow wants to merge 1 commit into
Meteor-Community-Packages:masterfrom
workflow:mocha-global-timeout
Closed

Add ability to set MOCHA_GLOBAL_TIMEOUT, without success.#33
workflow wants to merge 1 commit into
Meteor-Community-Packages:masterfrom
workflow:mocha-global-timeout

Conversation

@workflow

@workflow workflow commented May 26, 2017

Copy link
Copy Markdown

FIXME: Looks like mochaInstance.options.timeout does not pass the timeout to mocha.

Related to practicalmeteor/meteor-mocha-core#9?

@aldeed aldeed left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Overall looks good, but see my comment about the default

Comment thread package/runtimeArgs.js
mochaOptions: {
grep: MOCHA_GREP || false,
invert: !!MOCHA_INVERT,
timeout: MOCHA_GLOBAL_TIMEOUT || 2000,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think there should be a 2000 default here. Any reason not to just let this be undefined and let the mocha default apply?

@aldeed

aldeed commented Jun 12, 2017

Copy link
Copy Markdown
Contributor

It's possible that setting mochaInstance.options.timeout does not actually work on the server. The server code is a bit different from client. You'd have to see where the value is stored by digging into the code in the mocha repo

@workflow
workflow force-pushed the mocha-global-timeout branch from 3c6cf39 to 2217ac3 Compare August 5, 2017 15:16
@workflow

workflow commented Aug 5, 2017

Copy link
Copy Markdown
Author

@aldeed Totally agree on the default, thx for catching that.

For some reason that I haven't figured out yet, setting the timeout doesn't work via mochaInstance.options (nor directly on the mochaInstance object itself) on the already instantiated mochaInstance, but instead only during the Mocha instance creation here.

Any ideas why?


Given those circumstances, I see two ways to fix it, happy to have a shot at option B) if you feel like it's cleaner:

A) Merge practicalmeteor/meteor-mocha-core#9 and document the choice of supported "pass-thru" environment variable name here in meteor-mocha

B) Instantiate Mocha in meteor-mocha instead of meteor-mocha-core, and wrap it through setupGlobals() there. Or, even better, export a PatchedMocha subclass from meteor-mocha-core, that already plays nice with fibers and can be instantiated from meteor-mocha-core.

Any better ideas?

@SimonSimCity

Copy link
Copy Markdown
Member

@aldeed would this be fixed both for server and for client tests by resolving practicalmeteor/meteor-mocha-core#9?

@aldeed

aldeed commented Aug 9, 2018

Copy link
Copy Markdown
Contributor

I suggest doing a new version of that PR against the meteortesting fork and then option A. Not sure if that will fix, and it might be different now with newer mocha, too.

@SimonSimCity

Copy link
Copy Markdown
Member

Fixed by setting the option in https://github.com/meteortesting/meteor-mocha-core. I've added your text to the readme though 😉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants