karma-runner / karma-jasmine

A Karma plugin - adapter for Jasmine testing framework.
MIT License
542 stars 162 forks source link

Remove `Order.sort` function passing to Karma callback #272

Closed twolfson closed 4 years ago

twolfson commented 4 years ago

We received a bug report in karma-electron, https://github.com/twolfson/karma-electron/issues/47, which eventually traced back to Electron being unable to pass a function to between Karma contexts due to IPC fixing passing references between processes

This PR resolve that bug by removing the function which was being attempted to pass as a reference between processes

In this PR:

Feel free to request commit message changes (or apply them yourself). I tried to find documentation on them but was running out of time =/

twolfson commented 4 years ago

👍 Taking today off so will revisit tomorrow

On Tue, Aug 11, 2020, 9:35 AM johnjbarton notifications@github.com wrote:

@johnjbarton requested changes on this pull request.

The commit message first line would be like fix(adapter): filter functions from result

In src/adapter.js https://github.com/karma-runner/karma-jasmine/pull/272#discussion_r468712998 :

@@ -191,8 +191,13 @@ function KarmaReporter (tc, jasmineEnv) { // Any errors in top-level afterAll blocks are given here. handleGlobalErrors(result)

Looks like Object.assign() is not supported on IE11. https://caniuse.com/#search=object.assign

I think we could loop over Object.keys() and copy if typeof result[key] !== 'function'.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/karma-runner/karma-jasmine/pull/272#pullrequestreview-465237420, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG4KWHDEBFRUYSZBLENBVLSAFXMFANCNFSM4O3G6YRQ .

twolfson commented 4 years ago

Oh wow, I somehow assumed this repo was using a transpiler somehow (usually avoid them myself). Going to also drop the let to avoid browser compatibility issues

twolfson commented 4 years ago

Also using getOwnPropertyNames as keys can iterate over prototype keys as well (which I believe we want to avoid)

twolfson commented 4 years ago

Alright, that should do it. Tested with repro repo here from original issue https://gist.github.com/twolfson/5122c4398147df6bb2bfaeb37a59997b

I'll update the commit message now with what you gave me

twolfson commented 4 years ago

Decided to just squash the commits as I don't know how the commits are merged/squashed on this repo =/

karmarunnerbot commented 4 years ago

:tada: This PR is included in version 4.0.1 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

dpwatrous commented 4 years ago

Thanks for keeping on top of this @twolfson and @johnjbarton thanks for getting this merged. I'll be glad to remove our kludgey workaround for this!