Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Karma times out when using Electron 9 and client.useIframe = false #47

Closed
dpwatrous opened this issue Jul 15, 2020 · 20 comments · Fixed by karma-runner/karma-jasmine#272

Comments

@dpwatrous
Copy link

Description

When switching from Electron 8 to 9, I noticed our Karma/Electron tests were now timing out despite the tests passing. It looks like something is keeping the Electron process alive even when running with singleRun: true and autoWatch: false.

Reproduction Steps

I've created a repo with a fairly minimal reproduction of the problem. I've been able to see the issue using Node 12.18.2 on both Ubuntu 18 and MacOS.

Clone the repo:

  1. Clone the repo: git clone https://github.com/dpwatrous/karma-electron-repro.git
  2. Install dependencies: npm install
  3. Run the test with iframes (this should succeed): npm run test-iframe
  4. Run the test iframes disabled (this should hang until Karma times out): npm run test-no-iframe
@twolfson
Copy link
Owner

Hmm, interesting. I'm heads down at the moment but should be able to look into this by EOD, if not sooner. Thanks for the reproduction repo so far! =)

@dpwatrous
Copy link
Author

Sure thing, and thanks for the super quick reply! It's not particularly urgent on my end, but definitely let me know if you need any more info.

@twolfson
Copy link
Owner

Looking into this now

@twolfson
Copy link
Owner

Alright, had 1 distraction but so far I was able to upgrade to Electron@9 for our tests with only a module.id/module.parent change due to core Node.js behavior in our test suite but nothing else

It looks like I'm getting pulled away to another task at the moment but should continue soon after that hopefully

@twolfson
Copy link
Owner

Alright, got a little more time. Let's see if we can hammer this out

@twolfson
Copy link
Owner

I've successfully reproduced your error. Going to re-read the karma-electron documentation around iframe and dig into your code further

@twolfson
Copy link
Owner

Hmmm, when I make Electron visible and open the console of the new window. I see this error. At first glance, this looks like a serialization error for Karma across either its window to window bridge or its window to node bridge

electron/js2c/renderer_init.js:126 Uncaught (in promise) Error: An object could not be cloned.
    at EventEmitter.t.ipcRendererInternal.send.t.ipcRendererInternal.invoke (electron/js2c/renderer_init.js:126)
    at Object.BrowserWindowProxy.postMessage (electron/js2c/renderer_init.js:166)
    at postMessageCallParentKarmaMethod (context.js:293)
    at ContextKarma.boundProxyMethod [as complete] (context.js:199)
    at KarmaReporter.jasmineDone (adapter.js?e14e485cb2a825ba86577e6749e91dda740e6c04:196)
    at UserContext.fn (jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6488)
    at QueueRunner.attempt (jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6365)
    at QueueRunner.run (jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6406)
    at QueueRunner.execute (jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6273)
    at queueRunnerFactory (jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:1458)
t.ipcRendererInternal.send.t.ipcRendererInternal.invoke @ electron/js2c/renderer_init.js:126
BrowserWindowProxy.postMessage @ electron/js2c/renderer_init.js:166
postMessageCallParentKarmaMethod @ context.js:293
boundProxyMethod @ context.js:199
KarmaReporter.jasmineDone @ adapter.js?e14e485cb2a825ba86577e6749e91dda740e6c04:196
fn @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6488
attempt @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6365
QueueRunner.run @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6406
QueueRunner.execute @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6273
queueRunnerFactory @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:1458
dispatch @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6471
(anonymous) @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6437
(anonymous) @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:1650
(anonymous) @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:6422
channel.port1.onmessage @ jasmine.js?fbc7eaf339ea249d8c912b258548d4e7b4dc180c:2712

I'm going to see if that appeared in Electron@8 or not to confirm if it's a red herring or not

@twolfson
Copy link
Owner

Yep, that's definitely the issue/change. We get a CSP error in Electron@8 and 9 but no similar postMessage error in @8

@twolfson
Copy link
Owner

It looks like something in the window.postMessage handler must have changed to not parse data as we'd expect. Guessing it's attempting to clone now rather than serialize/similar but going to dig a little deeper

@twolfson
Copy link
Owner

Alright, so it looks like there have been changes to the Electron IPC to avoid passing references between contexts. I'm guessing the error is arising from karma-jasmine passing a function along the bridge as it similarly errors when I try to do so as well

Also that would explain why karma-mocha in karma-electron tests didn't have a similar issue

So the fix should be to update karma-jasmine to stop sending a function in their done handler (or at least it looks similar to that so far)

Selection_186

@twolfson
Copy link
Owner

Yep, a hack in karma-jasmine's adapter.js fixed it =)

    delete result.order.sort;
    tc.complete({
      order: result.order,
      coverage: window.__coverage__
    })

Going to search around for a bit for something more formal but we've got an answer and a hack-ish solution for now =)

@dpwatrous
Copy link
Author

Interesting - thanks for digging into this! I'll take a closer look tomorrow and see if the hack works for me as well.

@twolfson
Copy link
Owner

About to submit a PR which you can then use in place of karma-jasmine in your repo for the meantime =)

@twolfson
Copy link
Owner

Damn, they wrote their library in a way where git:// won't work so easily a dependency. I'm getting pulled off to another task but should be back to put a bow on this soon

@twolfson
Copy link
Owner

Alright, if you use the following in your package.json, it should use the patched code. Opening PR shortly now

// In place of "karma-jasmine": "^3.3.1"
"karma-jasmine": "twolfson/karma-jasmine#dev/stop.passing.functions.in.callbacks.usable.fork"

@twolfson
Copy link
Owner

twolfson commented Jul 16, 2020

Aaaand PR opened =) karma-runner/karma-jasmine#272

Edit: Also uploaded the modified git repro repo to here: https://gist.github.com/twolfson/5122c4398147df6bb2bfaeb37a59997b

@dpwatrous
Copy link
Author

I can verify that the fix works for me. I added a kludgy workaround in our code for now (just doing the same function deletion in our custom reporter), but I'll keep an eye on that karma-jasmine PR.

Thanks again for looking into this!

@twolfson
Copy link
Owner

Deciding to close this issue for now since it's been concluded that this is not caused by karma-electron

@twolfson
Copy link
Owner

Alright, the PR has been landed and released on karma-jasmine@4.0.1 so you should be good to go =)

@dpwatrous
Copy link
Author

Awesome, thanks for the fix!

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 a pull request may close this issue.

2 participants