ember-cli-code-coverage icon indicating copy to clipboard operation
ember-cli-code-coverage copied to clipboard

Incompatible with ember-electron

Open xn opened this issue 7 years ago • 11 comments

https://github.com/felixrieseberg/ember-electron/issues/259 @anulman

There are two issues that need to be resolved to get this working.

  1. ember-electron redefines require and thus breaks ember-cli-code-coverage. if ember-cli-code-coverage were to use requireModule instead of require, we could side step this. I did that here: https://github.com/xn/ember-cli-code-coverage/commit/fa901e71818fc07d8dcff64d03c4a792d08bab60

  2. Middleware is not accessible in the electron context. To get around this, I built a little express app that I ran before I ran my tests. You can see it here: https://gist.github.com/xn/d9d71acb3af59eb90d921ecdf8e738e0 I also had to change the host. This could be configured with an ENV var. https://github.com/xn/ember-cli-code-coverage/commit/ff3d271f25ac25d88d2c2e3091035ce3a72dcb16

Thoughts?

xn avatar Dec 07 '17 15:12 xn

Thanks @xn. Hi @kategengler!

Was wondering if a PR enabling ember-electron support would be accepted, and if so if someone here could help coordinate as we don't know where there be dragons + are not yet familiar with e-cli-cc's guts / test suite.

The gist would be to wrap @xn's fixes linked above in checks for presence of process.env.EMBER_CLI_ELECTRON. This could be adopted as part of e-cli-cc, or we could create a separate addon for e-cli-cc-electron.

Happy to answer questions, or discuss how we might better handle this in e-electron instead as well. Overall, I believe e-cli-cc is the right home for this logic, but at this point this is just a loosely held opinion.

anulman avatar Dec 07 '17 16:12 anulman

Hi @xn and @anulman is this still an issue if you install ember-cli-code-coverage 1.0.0-beta.2?

RobbieTheWagner avatar Feb 08 '18 02:02 RobbieTheWagner

I'm under a time crunch, I can get to this next week and will report back. But from a quick look at the commits, I don't see how those two issues are addressed.

xn avatar Feb 08 '18 19:02 xn

@xn it's essentially a full rewrite, so please check when you get a chance and let me know!

RobbieTheWagner avatar Feb 08 '18 21:02 RobbieTheWagner

Sure thing. found a moment to try out. Same problem. You are still using require here: https://github.com/kategengler/ember-cli-code-coverage/blob/master/lib/templates/test-body-footer.html#L8

And the middleware is still inaccessible.

xn avatar Feb 08 '18 23:02 xn

@xn does requireModule fix it still? I see there were two things needed to fix it.

RobbieTheWagner avatar Feb 09 '18 03:02 RobbieTheWagner

Yes, both changes are needed

xn avatar Feb 09 '18 15:02 xn

Hi, I know this is a pretty old issue but I recently came across this same issue (using ember-electron) and by updating the URL for '/write-coverage' to 'http://localhost:port/write-coverage', I as able to generate the coverage report. 'http://localhost:port/write-coverage' was where I hosted my own express middleware as @xn proposed in his changes. However, going into the node_module and editing the code there is not something we can ideally do. I was wondering if anyone else has faced this issue recently. And if yes, would it be possible to maybe make the '/write-coverage' URL into a variable that we can update via our application (eg: having a process.env variable) Also, we are using ember-electron v3.1.1 and ember-cli-code-coverage v2.0.0-beta.4

saloniy avatar Nov 03 '22 16:11 saloniy

@saloniy It doesn't look like the existing code determines the host https://github.com/kategengler/ember-cli-code-coverage/blob/e3dcf629745e866dd236a75e444d46f1d927f930/packages/ember-cli-code-coverage/lib/attach-middleware.js#L203

I think a PR to make the endpoint configurable would be fine, though.

kategengler avatar Nov 03 '22 16:11 kategengler

Sounds good! I'll take that on in a few days 👍

saloniy avatar Nov 03 '22 16:11 saloniy

Hi! I have created a Pull Request for the change discussed. Would appreciate if it can get reviewed and merged!

saloniy avatar Jan 03 '23 16:01 saloniy