search

mattheworiordan / capybara-screenshot

Automatically save screen shots when a Capybara scenario fails
MIT License
1.02k stars 168 forks source link

Do not raise #208

Closed ywen closed 7 years ago

ywen commented 7 years ago

Motivation

See the screenshot: screen shot 2017-06-26 at 1 47 36 pm

When calling page.current_path it raises a UnhandledAlertError. There is nowhere the spinach handles the exceptions so the hooks broke and no report is written into XML file (when using jUnit report formatter). That means the result report will say no failures. It makes debug very hard

screen shot 2017-06-26 at 7 15 27 pm

I went back and forth to think should I handle from spinach or from this gem. I could handle it from spinach, however spinach has no context on how to handle the exceptions. In this gem there is not a very good way to take screenshots when current_path raises so I can accept in this situation no screenshot is taken.

Please let me know your thoughts, thanks you

ywen commented 7 years ago

@mattheworiordan Thanks for the comments. They are all good advice. I made the first commit 5b5f44d to address all your comments. Then I went ahead and made another commit 5f450cfc066e91ca1e774c16e5eb2c4598862950 to print a warning message when page.current_path is empty.

When I am at it, I made the third commit 131977f8bda35932f049602c59407927a0c06157 to print out warning if save_html and save_screenshot raise. And when save_html fails I allow save_screenshot to continue so that we may have one output anyway.

Please let me know your thoughts. I am totally fine if you don't like the latest 2 commits.

Thanks!

mattheworiordan commented 7 years ago

Thanks @ywen, small bit of feedback on 5f450cf though

ywen commented 7 years ago

@mattheworiordan The 2 new comments you have are linked: to be able to print out only one warning message. I used the block approach. When the page.current_path raises the block is not called at all so no warning message is printed within those.

I add one more commit to add one more test to describe this.

Thanks!

mattheworiordan commented 7 years ago

Thanks @ywen, good to merge :)

ywen commented 7 years ago

@mattheworiordan Thank you for quick response! If it is convenient would you please release a new version, or you would like to wait a little longer since you just did one 4 days ago? Thanks again.

mattheworiordan commented 7 years ago

v1.0.17 is now released

ywen commented 7 years ago

Thank you!