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

Added ability to fetch the console log #73

Merged
merged 1 commit into from
Mar 25, 2016
Merged

Conversation

TheoAndersen
Copy link
Contributor

Using Hound for some javascript accept tests, it found myself needing to assert on the log output.

I have created a small addition, which can fetch the log and fetch errors written to the log.

@HashNuke
Copy link
Owner

HashNuke commented Mar 6, 2016

Hey @TheoAndersen Thanks for the PR. I missed looking into this.

Nice feature. If you can fix the build for selenium, I'll go ahead and merge it in.

@vitalis
Copy link
Contributor

vitalis commented Mar 8, 2016

👍

@vitalis
Copy link
Contributor

vitalis commented Mar 8, 2016

@TheoAndersen, @HashNuke
The test can't be fixed because of this issue: SeleniumHQ/selenium#1161
Right now selenium with firefox does not support console.log output (requires extensions for firefox).
What we can do is to raise not supported exception for selenium with firefox

@HashNuke
Copy link
Owner

@TheoAndersen Thank you for clarifying. We do a hacky thing to disable multi-session tests for Phantom (because all sessions share the same cookie jar). Something similar could be done to disable tests for firefox in this case.
@vitalis Yes a NotSupported exception would be nice for browsers that don't support it.

@HashNuke
Copy link
Owner

@tuvistavie I think we should support a NotSupported exception for features that are supported by particular browsers. Like this issue.

@danhper
Copy link
Collaborator

danhper commented Mar 17, 2016

@HashNuke I agree, let's add this to exceptions.ex. To keep the name consistent with other modules defining exceptions, I think Hound.NotSupportedError is appropriate, what do you think?

@HashNuke
Copy link
Owner

@tuvistavie totally fine. Let's run with it.

@danhper
Copy link
Collaborator

danhper commented Mar 17, 2016

@HashNuke Ok, let's go with this then! Do you want me to add it?

@HashNuke
Copy link
Owner

@tuvistavie sure. That's totally fine. Please do.

@vitalis
Copy link
Contributor

vitalis commented Mar 17, 2016

@tuvistavie, @HashNuke it will be nice to add feature that will fail the test on js error by default with option to disable it.
Also maybe instead of concatenating the errors to one message it will be better to parse them as hound error message

@danhper
Copy link
Collaborator

danhper commented Mar 22, 2016

@TheoAndersen
I just added NotSupportedError so it would be great if you could update the PR to raise it when using Selenium.
Thank you very much!

@TheoAndersen
Copy link
Contributor Author

@tuvistavie
Sorry that I've been a bit absent.
I'll update it as soon as i have time one of the next days.
/Theo

@danhper
Copy link
Collaborator

danhper commented Mar 22, 2016

@TheoAndersen Whenever you have time, thank you very much! 😄

@TheoAndersen
Copy link
Contributor Author

There..
But is there a cleaner way to check for the current web-driver being Selenium? :)
/Theo

end

defp is_webdriver_selenium() do
IO.inspect "webdriver: #{System.get_env("WEBDRIVER")}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can remove this ✂️

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Woops

@danhper
Copy link
Collaborator

danhper commented Mar 25, 2016

@TheoAndersen Thank you very much!
I added a few comments. Also, could you squash everything into a single commit please?

@@ -7,6 +7,8 @@ defmodule Hound.Helpers.Log do
Fetches console.log() from the browser as a single string of log-lines.
"""
def fetch_log() do
fail_if_webdriver_selenium("fetch_log()")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Logs doesn't work only for Firefox on selenium, so I'm not sure it's correct to fail on selenium webdriver, if there is a way to know that the browser is Firefox and then fail it will be much better

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hound.driver_info also returns the browser, so we can do something like

if match?({:ok, %{driver: "selenium", browser: "firefox"}, Hound.driver_info) do
  # fail badly
end

@TheoAndersen
Copy link
Contributor Author

Ahh. okay sure.

Just trying to get my head around untanging a slight rebasing mess. Did a merge between my changes, and now i need to rebase around it somehow to squash it to one commit..

edit: figured it out... rebase --onto is handy ;)

end

defp fail_if_webdriver_selenium(function) do
if match?({:ok, %{driver: "selenium", browser: "firefox"}}, Hound.driver_info) do
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hound.NotSupportedError.raise_for

already does the check for you, so you can drop the condition!

@danhper
Copy link
Collaborator

danhper commented Mar 25, 2016

Great, thank you very much!
I left a last little comment and we are good to merge! 😃

@TheoAndersen
Copy link
Contributor Author

Super, I'll look at it.

And thanks. I'm learning alot.. still new to elixir :)

Log functions raises NotSupportedError on Selenium
Selenium dosen't support the log-fetching methods, so we throw
NotSupportedError on those.
@danhper
Copy link
Collaborator

danhper commented Mar 25, 2016

Perfect, thank you very much! 🎉

@danhper danhper merged commit 4145c69 into HashNuke:master Mar 25, 2016
@HashNuke
Copy link
Owner

Thank you @TheoAndersen & @tuvistavie ~!

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.

4 participants