kaliber5 / ember-cli-yadda

Write cucumber specs for ember-cli applications
MIT License
42 stars 22 forks source link

yadda's require() overwrites ember's require() #30

Closed benwalder closed 7 years ago

benwalder commented 8 years ago

I am running tests using ember-cli-yadda and yadda, and using ember-cli-code-coverage.

Yadda is setting ember's global require() to its own function. This makes the coverage reporting fail in the script inserted into tests/index.html at this line of code. Since global require() is no longer ember's function, require.entries is undefined.

requirejs and requireModule are both still set to ember's require method function.

Other than asking yadda to not clobber require(), do you have any suggestions on a good way to have ember-cli-yadda work around this?

One way may be to set require = requireModule; in the generated afterEach method.

benwalder commented 8 years ago

In addition to the above, yadda clobbering require() is also causing the TestLoader to put out the warning:

unable to require.unsee, please upgrade loader.js to >= v3.3.0

before any tests are run because unsee doesn't exist on yadda's require() function.

This seems like an undesirable side effect.

rox163 commented 8 years ago

@sfbwalder curious as to how to figured out yadda was overwriting Ember's require? I think mocha might have the same issue, as I am seeing the same error with ember-cli-code-coverage so I would like to see if I need to raise an issue in ember-cli-mocha.

albertjan commented 8 years ago

Oh that is going to be a difficult one. I'll have to investigate if it's even possible to scope the require function of yadda or something. I know ember-electron does something for their require function they rename the node require to requireNode so you can use both theirs and embers.

benwalder commented 8 years ago

@rox163 this line of code was throwing a "entries not found" error, so I put a break point in chrome dev tools, then inspected require() by typing it in the console, and it was set to a function that was not ember's. After some additional watching, I saw the function matched the definition of require that is set on the first line of the yadda's distributed js.

For the short term I forked this repo, and then set require=requireModule in the afterEach() of the generated test files. Then ember-cli-code-coverage worked, but that doesn't seem like good long term solution.

Hope that helps.

rox163 commented 8 years ago

ah thanks! so based on that, it looks like mocha does the same thing here ? https://github.com/mochajs/mocha/blob/master/mocha.js#L1

benwalder commented 8 years ago

I am somewhat a newbie, but it appears yadda is browserified, and in the browserify process it is doing a .require with expose option. This causes the browserified yadda distributed js file to have have require=(... which overrides the global require function ember wants to use. Yadda may do the .require with expose so other js such as ember-cli-yadda can do require('yadda').

This seems to have the following effect when running ember tests:

  1. When the ember app starts, vendor.js runs, and ember's require() is set.
  2. Then test-support.js runs, which includes yadda, and require() is overwritten with browserify's require function.
  3. From here on all calls made to require() are going through the browserify require function. Browserify's require is smart enough to save the original require, and call it if it can't find the requested file name with in yadda's js file.
  4. The means the require calls in TestLoader are going through be browserify's require, but still work because it ends up calling back to ember's require.
  5. While require continues to work, require.unsee and require.entries no longer work as those are specific to ember's require.

These side affects makes me feel using yadda with ember is somehow unsafe, but that may be my newbie side coming out.

albertjan commented 7 years ago

this has been addressed by the move to npm

benwalder commented 7 years ago

Would appreciate if you would publish the fix to this issue as 0.2.0 (or change the readme to match). Thanks.

albertjan commented 7 years ago

yeah will publish when it all works 😄 did you see #39?