LinkedInAttic / dustjs-helpers

Helpers for dustjs-linkedin
MIT License
114 stars 70 forks source link

failing tests with mocha #36

Closed seriousManual closed 11 years ago

seriousManual commented 11 years ago

Every library that has dusjs-helpers as dependency and is tested via mocha is failing the tests.

dustjs-helpers manipulates the global namespace (dust = require('dustjs-helpers');) thus mocha criticizes the global leak.

vybs commented 11 years ago

@jairodemorais can you please take a look at this?

jairodemorais commented 11 years ago

@zaphod1984 could u give us more feeback about this issue?

It will be really useful if u send us the steps to reproduce it.

seriousManual commented 11 years ago

This is easy: Any test scenario where the mocha testing framwork is involved will fail when dustjs-helpers are involved.

zaphod@sanchez:~$ mkdir dustbug && cd dustbug
zaphod@sanchez:~/dustbug$ mkdir node_modules
zaphod@sanchez:~/dustbug$ npm install dustjs-helpers
[...]
zaphod@sanchez:~/dustbug$ npm install dustjs-linkedin
[...]
zaphod@sanchez:~/dustbug$ npm install mocha
[...]
zaphod@sanchez:~/dustbug$ echo "describe('f00', function(){ it('bar', function(){ var a = require('dustjs-helpers'); }); });" > test.js

zaphod@sanchez:~/dustbug$ mocha

  ..

  ? 1 of 1 test failed:

  1) f00 bar:
     Error: global leak detected: dust
[...]
jimmyhchan commented 11 years ago

I'm (slowly) in the process of making Dust core and this helper AMD friendly using the UMD module pattern.

Does my branch of dust-helpers fix the mocha problem? https://github.com/jimmyhchan/dustjs-helpers/tree/AMDFriendly

If it does I too have a pull request may address the issue https://github.com/linkedin/dustjs-helpers/pull/33

seriousManual commented 11 years ago

Hi, I'm not very much into the whole AMD thing, but probably your solution will solve the global object leak issue. Essentially it goes down to the fact that inside the main function the variable 'dust' is set in a way that it pollutes the global namespace.

seriousManual commented 11 years ago

resolved, what about a npm package push?