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

Plugin fails on systems w/o Edge #17

Closed
forbesjo opened this issue Nov 7, 2016 · 10 comments · Fixed by #18
Closed

Plugin fails on systems w/o Edge #17

forbesjo opened this issue Nov 7, 2016 · 10 comments · Fixed by #18

Comments

@forbesjo
Copy link

forbesjo commented Nov 7, 2016

The latest update broke non-Windows users:

.../node_modules/karma-detect-browsers/index.js:24
                browserPaths = browser.DEFAULT_CMD[process.platform] || [],
                                                  ^

TypeError: Cannot read property 'darwin' of undefined

https://github.com/litixsoft/karma-detect-browsers/blob/master/browsers/Edge.js#L7

The early return is making the Edge browser object undefined

@nickserv
Copy link
Contributor

nickserv commented Nov 7, 2016

That's odd, this seems to work fine for me on macOS. While the error handling could be improved in my opinion, resolve.sync() still isn't expected to fail. Please try clearing your node_modules and npm installing again, and let me know if you still run into issues. Also, can you reproduce this same error by running the karma-detect-browsers test suite on your machine?

@forbesjo
Copy link
Author

forbesjo commented Nov 7, 2016

Still fails for me.

@forbesjo
Copy link
Author

forbesjo commented Nov 7, 2016

Seems to be the same failure here https://travis-ci.org/litixsoft/karma-detect-browsers/jobs/173933385

@nickserv
Copy link
Contributor

nickserv commented Nov 7, 2016

It seems like the karma-safaritechpreview-launcher dependency only supports Node 5+, due to usage of ES6/7/something syntax. Ideally this package should have that noted in the engines field of package.json, but you should be able to fix this by using at least Node 5. If switching Node versions does indeed fix your issue, please rename this issue.

See marcoscaceres/karma-safaritechpreview-launcher#5.

EDIT: I'm not sure if switching Node versions would fix this, or if it's an unrelated issue.

@gkatsev
Copy link
Contributor

gkatsev commented Nov 7, 2016

I'm not sure it's just karma-safaritechpreview-launcher that's the issue. We have a videojs build that runs on codeship and it fails: videojs/video.js#3754 and we do not have the safari tech preview launcher installed: https://github.com/videojs/video.js/blob/master/package.json#L90-L101

@gkatsev
Copy link
Contributor

gkatsev commented Nov 7, 2016

If I change this line locally to be CMD = '', then everything works fine.

@nickserv
Copy link
Contributor

nickserv commented Nov 7, 2016

Hm, it seems like I've been discussing two unrelated issues that both started happening recently. There's the original CMD issue, and a regression in a dependency of karma-safaritechpreview-launcher (which I have an issue for in its repo).

@gkatsev Would it be possible for you to send me the log for your failing codeship build? It would help if I can reproduce this locally first. Also, I agree that CMD should be reset when resolve.sync() fails (instead of returning).

@gkatsev
Copy link
Contributor

gkatsev commented Nov 8, 2016

Here's a stack trace of when I run locally via grunt karma --stack --verbose --debug:

TypeError: Cannot read property 'darwin' of undefined
    at getInstalledBrowsers (/Users/gkatsevman/p/video.js/node_modules/karma-detect-browsers/index.js:25:51)
    at DetectBrowsers (/Users/gkatsevman/p/video.js/node_modules/karma-detect-browsers/index.js:65:28)
    at Array.invoke (/Users/gkatsevman/p/video.js/node_modules/karma/node_modules/di/lib/injector.js:75:15)
    at null.get (/Users/gkatsevman/p/video.js/node_modules/karma/node_modules/di/lib/injector.js:48:43)
    at /Users/gkatsevman/p/video.js/node_modules/karma/lib/server.js:143:20
    at Array.forEach (native)
    at null.Server._start (/Users/gkatsevman/p/video.js/node_modules/karma/lib/server.js:142:21)
    at null.invoke (/Users/gkatsevman/p/video.js/node_modules/karma/node_modules/di/lib/injector.js:75:15)
    at null.Server.start (/Users/gkatsevman/p/video.js/node_modules/karma/lib/server.js:103:18)
    at Object.<anonymous> (/Users/gkatsevman/p/video.js/node_modules/grunt-karma/tasks/grunt-karma.js:136:14)
    at Object.<anonymous> (/Users/gkatsevman/p/video.js/node_modules/grunt/lib/grunt/task.js:264:15)
    at Object.thisTask.fn (/Users/gkatsevman/p/video.js/node_modules/grunt/lib/grunt/task.js:82:16)
    at Object.<anonymous> (/Users/gkatsevman/p/video.js/node_modules/grunt/lib/util/task.js:301:30)
    at Task.runTaskFn (/Users/gkatsevman/p/video.js/node_modules/grunt/lib/util/task.js:251:24)
    at Task.<anonymous> (/Users/gkatsevman/p/video.js/node_modules/grunt/lib/util/task.js:300:12)
    at /Users/gkatsevman/p/video.js/node_modules/grunt/lib/util/task.js:227:11
    at nextTickCallbackWith0Args (node.js:420:9)
    at process._tickCallback (node.js:349:13)
    at Function.Module.runMain (module.js:443:11)
    at startup (node.js:139:18)
    at node.js:968:3

LitixThomas added a commit that referenced this issue Nov 8, 2016
gkatsev added a commit to gkatsev/karma-detect-browsers that referenced this issue Nov 8, 2016
This makes it line up with the other browser files and fixes litixsoft#17.
@kumavis
Copy link

kumavis commented Nov 9, 2016

+1

@gkatsev
Copy link
Contributor

gkatsev commented Nov 10, 2016

Thanks @LitixThomas!

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.

4 participants