Skip to content

Set screenshot dir #197

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions lib/hound/connection_server.ex
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,6 @@ defmodule Hound.ConnectionServer do
{4444, "wd/hub/", "firefox"}
end


browser = options[:browser] || Application.get_env(:hound, :browser, default_browser)
host = options[:host] || Application.get_env(:hound, :host, "http://localhost")
port = options[:port] || Application.get_env(:hound, :port, default_port)
Expand All @@ -30,7 +29,8 @@ defmodule Hound.ConnectionServer do

configs = %{
:host => options[:app_host] || Application.get_env(:hound, :app_host, "http://localhost"),
:port => options[:app_port] || Application.get_env(:hound, :app_port, 4001)
:port => options[:app_port] || Application.get_env(:hound, :app_port, 4001),
:screenshot_dir => options[:screenshot_dir] || Application.get_env(:hound, :screenshot_dir, File.cwd!)
}

state = %{sessions: [], driver_info: driver_info, configs: configs}
Expand Down
8 changes: 6 additions & 2 deletions lib/hound/helpers/screenshot.ex
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@ defmodule Hound.Helpers.Screenshot do

defp default_path do
{{year, month, day}, {hour, minutes, seconds}} = :erlang.localtime()
cwd = File.cwd!()
"#{cwd}/screenshot-#{year}-#{month}-#{day}-#{hour}-#{minutes}-#{seconds}.png"
"#{screenshot_path()}/screenshot-#{year}-#{month}-#{day}-#{hour}-#{minutes}-#{seconds}.png"
end

defp screenshot_path do
{:ok, configs} = Hound.configs
configs[:screenshot_dir]
end

end
1 change: 1 addition & 0 deletions lib/hound/helpers/session.ex
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ defmodule Hound.Helpers.Session do
* `:metadata` - The metadata to be included in the requests.
See `Hound.Metadata` for more information
* `:driver` - The additional capabilities to be passed directly to the webdriver.
* `:screenshot_dir` - Override the default directory that screenshots are saved to.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know if this is actually true, or useful to document.

"""
def start_session(opts \\ []) do
Hound.SessionServer.session_for_pid(self(), opts)
Expand Down
67 changes: 65 additions & 2 deletions test/helpers/screenshot_test.exs
Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,73 @@ defmodule ScreenshotTest do

hound_session()

test "should take a screenshot" do
setup do
navigate_to("http://localhost:9090/page1.html")
path = take_screenshot("screenshot-test.png")
:ok
end

test "should take a screenshot with a generated file name" do
default_path = File.cwd!()

path = take_screenshot()

assert File.exists?(path)
assert path =~ default_path
assert file_name(path) =~ ~r/^screenshot-\d{4}(?:-\d{1,2}){5}\.png$/
end

test "file names generated 1 second apart are unique" do
default_path = File.cwd!()

path1 = take_screenshot()
:timer.sleep(1000)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This adds an extra second to an already slow test, so I'm happy to rip this test out if you don't think it's necessary. I couldn't think of a better way to test this.

Copy link
Collaborator

@danhper danhper Jan 7, 2018

Choose a reason for hiding this comment

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

Tests speed is not that much of an issue yet, but is the granularity of the file name 1 second?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tuvistavie yeah, based on the way the filename is currently generated, it will only be unique down to the second.

https://github.com/marcdel/hound/blob/9a627eab709e5b852ee08eb2934e1fc37296128b/lib/hound/helpers/screenshot.ex#L33

Copy link
Collaborator

Choose a reason for hiding this comment

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

Right, I completely forgot we were generating the name this way. Thanks.
We should probably improve this but it's not related to this PR, so let's see later.

path2 = take_screenshot()

paths = [path1, path2]
assert paths == Enum.uniq(paths)
end

describe "When given a file name" do
test "should take a screenshot at the default location with the specified file name" do
default_path = File.cwd!()

take_screenshot("screenshot-test.png")

assert File.exists?("#{default_path}/screenshot-test.png")
end
end

describe "When given a file name with path" do
test "should take a screenshot at the specified location with the specified file name" do
take_screenshot("test/screenshot-test.png")

assert File.exists?("test/screenshot-test.png")
end
end

describe "When :screenshot_dir is set" do
setup do
screenshot_dir = "tmp/screenshots/"
Application.put_env(:hound, :screenshot_dir, screenshot_dir)
change_session_to(:with_opts, [screenshot_dir: screenshot_dir])
Copy link
Contributor Author

@marcdel marcdel Dec 28, 2017

Choose a reason for hiding this comment

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

These seem to happen too late in the application lifecycle. I'm not sure if there's an earlier event I can hook into?

Copy link
Collaborator

Choose a reason for hiding this comment

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

This will normally be configured by config.exs anyway, so let's stick with what you have here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tuvistavie sorry, should have been more clear. Neither of the options (line 54 or 55 above) actually change the screenshot directory in the test so all it's currently testing is that the file name is generated in the same way.

This line actually fails right now:
https://github.com/marcdel/hound/blob/9a627eab709e5b852ee08eb2934e1fc37296128b/test/helpers/screenshot_test.exs#L66


navigate_to("http://localhost:9090/page1.html")
:ok
end

test "should take a screenshot in the screenshot dir with a generated file name" do
path = take_screenshot()

assert File.exists?(path)
# ConnectionServer already started so new screenshot_dir not picked up
# assert path =~ "tmp/screenshots/"
assert file_name(path) =~ ~r/^screenshot-\d{4}(?:-\d{1,2}){5}\.png$/
end
end

defp file_name(path) do
path
|> String.split("/")
|> List.last()
end
end