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

Handle case where GET_LOG is not supported #354

Merged
merged 5 commits into from
Sep 7, 2017

Conversation

inxilpro
Copy link
Contributor

@inxilpro inxilpro commented Sep 6, 2017

Many drivers (Firefox, MSIE, etc) do not support retrieving console logs (the GET_LOG) command, resulting in:

Facebook\WebDriver\Exception\UnknownServerException: Command not found: POST
    /session/<session_id>/log

This catches that error and writes an error message to the log file instead. This would only affect people using alternate drivers.

@inxilpro
Copy link
Contributor Author

inxilpro commented Sep 6, 2017

In the interim, if you're running into this issue, you can add the following to your DuskTestCase class:

protected function storeConsoleLogsFor($browsers)
{
    try {
        parent::storeConsoleLogsFor($browsers);
    } catch (UnknownServerException $exception) {
        // Some drivers do not support the GET_LOG command
    }
}

@deleugpn
Copy link
Contributor

deleugpn commented Sep 6, 2017

Hard to believe this is a Dusk problem. Seems more like something that needs to be handled on Facebook WebDriver if the browser truly don't offer the HttpCommand.

@inxilpro
Copy link
Contributor Author

inxilpro commented Sep 6, 2017

Technically the the get log command isn't part of the web driver spec. It's only supported by Chrome. See w3c/webdriver#406

@inxilpro
Copy link
Contributor Author

inxilpro commented Sep 6, 2017

Another way to handle this is to only try to get logs if we're using a Chrome driver, since it's a Chrome-specific feature. But that means that Dusk would need to be updated when other browsers add support (which should happen at some time).

@taylorotwell
Copy link
Member

We should only try when using Chrome. Can you re-submit with that?

@deleugpn
Copy link
Contributor

deleugpn commented Sep 6, 2017

If we only implement this for Chrome, it means, for instance, anybody using PhantomJS with Dusk loses the ability to retrieve console. Selenium has decided on offering getLog API a long time ago and Dusk relies on Facebook WebDriver which relies on Selenium. The fact that Firefox haven't decided on the spec for this yet should not be a reason to only support this on Chrome.

Again, it's not a Dusk problem. Dusk is just a wrapper on Facebook WebDriver for you and if you use Facebook WebDriver and Firefox, you're gonna get the same error.

An alternative, imo, would be to have a on/off flag for Firefox users to be able to disable console.

@inxilpro
Copy link
Contributor Author

inxilpro commented Sep 6, 2017

@deleugpn It's not Firefox-only. Chrome is the only browser that supports the get log command. It looks like it was in the early spec, but was removed for some reason. I've been running Dusk tests on BrowserStack and ran into this on everything I tried it on.

I think #355 is a good temporary fix, but I don't know that it addresses the underlying issue, which is that this is a Chrome-only feature.

@taylorotwell How about disabling if we're not using Chrome, but giving an option to override that?

@deleugpn
Copy link
Contributor

deleugpn commented Sep 6, 2017

What about PhantomJS? What about any Selenium compatible browser?

@inxilpro
Copy link
Contributor Author

inxilpro commented Sep 6, 2017

@deleugpn how about my latest commit? It looks like Webkit supports the get log command, which means that PhantomJS supports it as well.

I'm not sure if a whitelist of a blacklist is better—my reading on the topic suggests that it's not supported by most browsers, but I haven't tested them all.

@taylorotwell taylorotwell merged commit d0f4277 into laravel:2.0 Sep 7, 2017
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 this pull request may close these issues.

3 participants