Set screenshot dir #197
Set screenshot dir #197
Conversation
| @@ -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. | |||
There was a problem hiding this comment.
I don't know if this is actually true, or useful to document.
| default_path = File.cwd!() | ||
|
|
||
| path1 = take_screenshot() | ||
| :timer.sleep(1000) |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Tests speed is not that much of an issue yet, but is the granularity of the file name 1 second?
There was a problem hiding this comment.
@tuvistavie yeah, based on the way the filename is currently generated, it will only be unique down to the second.
There was a problem hiding this comment.
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.
| setup do | ||
| screenshot_dir = "tmp/screenshots/" | ||
| Application.put_env(:hound, :screenshot_dir, screenshot_dir) | ||
| change_session_to(:with_opts, [screenshot_dir: screenshot_dir]) |
There was a problem hiding this comment.
These seem to happen too late in the application lifecycle. I'm not sure if there's an earlier event I can hook into?
There was a problem hiding this comment.
This will normally be configured by config.exs anyway, so let's stick with what you have here.
There was a problem hiding this comment.
@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
|
Thank you very much. I just left one comment, as soon as this is confirmed I think we can merge. |
Implements #196
WIP: line 66 of
screenshot_test.exsis failing because I haven't found a way to get the ConnectionServer to pick up the updated:screenshot_dirconfig value since the supervisor has already started it. I've tried changing sessions, setting the env variable, stoping/starting the agent (my limited experience with OTP kept me from successfully going down this path, but I assume there's an easier way anyway).If you know of a good way to update those config values, or have a better way of testing that, I'm definitely open to suggestions 😀