jfirebaugh / konacha

Test your Rails application's JavaScript with the mocha test framework and chai assertion library
Other
1.05k stars 117 forks source link

Disable leak detection #22

Closed joliss closed 12 years ago

joliss commented 12 years ago

Mocha's leak detection is giving a slew of problems. It happened in issue #19, and it also happened in this test in Konacha's test suite (after I tried removing the jQuery include tag):

screenshot

The top-most line referred to by the stack trace is

this.fail(test, new Error('global leaks detected: ' + leaks.join(', ') + ''));

Note that the second Array#sum test passes fine. I have no idea what's going on, I just know that the leak detection is being really unhelpful.

I say we should set ignoreLeaks = true. I just haven't figured out yet how to set it on the runner object.

jfirebaugh commented 12 years ago

This was fixed upstream. I've updated the vendored copy of mocha.

joliss commented 12 years ago

Cool, thanks for fixing!

Reading through their issue, I still wonder if we should disable leak detection because it's so brittle. This feature scares me.

jfirebaugh commented 12 years ago

I'd rather not deviate from Mocha's defaults without good reason. I worked up support for arbitrary mocha configuration options, but it doesn't feel right doing it Ruby-side:

Konacha.configure do |config|
  config.mocha.ignoreLeaks = true
end
joliss commented 12 years ago

I'd rather not deviate from Mocha's defaults without good reason.

Fair enough. I might go whine upstream about the default if it keeps bothering me. :)

Re support for arbitrary options, I think your code would be a huge improvement already. I agree it should more naturally live on the JS side though. We could have a konacha.mochaOptions object (to be set in spec_helper.js), but really I'm wondering if it'd be more explicit to just tell people to call mocha.setup themselves, and pass whatever options they want.

joliss commented 12 years ago

... except that we have to call mocha.setup before declaring any of the tests, or describe won't be defined. I think I get the problem now.

I'll let this gestate for a while. Maybe I'll have some inspiration later.

jfirebaugh commented 12 years ago

Here's my proposal:

joliss commented 12 years ago

Or,

That might be easier to implement, because you just need to set some variable in setup that can be checked externally, instead of making sure that setup can be run multiple times.

joliss commented 12 years ago

In the absence of an obvious (and clean) solution without patching Mocha, I'm wondering if we should delay this for Konacha 2.0. Cramming a patch into the Mocha upstream, pulling an untested vendor version, and instantly making a major release is just asking for trouble in my view.

I'd rather get 1.0 out the door without stress and headache, and then when we have more users to test it, we can make changes like this on master.

What do you think?

jfirebaugh commented 12 years ago

I agree. However, I would also like to remove config.interface for the 1.0 release in that case, since we know it's otherwise likely to be removed later, and I don't anticipate that many people will want to change it anyway.

joliss commented 12 years ago

I personally wouldn't worry about changing it with the next major-version bump. But feel free to remove it if you prefer to keep it out of the 1.0 release.