thoughtbot / capybara-webkit

A Capybara driver for headless WebKit to test JavaScript web apps
https://thoughtbot.com/open-source
MIT License
1.97k stars 428 forks source link

Allow JavaScript errors to be raised as exceptions #988

Closed odlp closed 7 years ago

odlp commented 7 years ago

When configured, upon taking any action:

#<Capybara::Webkit::JavaScriptError: {:line_number=>5, :message=>"ReferenceError: Can't find variable: undefinedFunc", :source=>"http://127.0.0.1:56520/"}>

Motivation

On a project using capybara-webkit the test-suite passed despite a critical JavaScript minification bug. Because the specs didn't defensively assert that error_messages are empty on each navigation, the bug wasn't caught.

I think this option acts as a good safeguard — a strict mode if you will — and will also aid debugging during the TDD cycle. If a failing scenario (now implemented) is still failing, this could speed up figuring out that the failure is due to a different reason than you thought it was.

This feature is inspired by Poltergeist which has an option to raise JS errors as exceptions.


Questions

odlp commented 7 years ago

Disregarding the Hound single-quoted strings warnings because most of the changes match the current formatting of the files changed, and the thoughtbot Ruby styleguide states:

Prefer double quotes for strings.

We could disable this check by adding a .rubocop.yml entry for this rule like this.

jferris commented 7 years ago

I think this is a good general idea, but I'm not sure where the errors should actually be raised. Raising errors in visit will only catch errors that happen when you first load the page. If the error occurs in a click handler, Ajax request, or something else, it will be raised in the next visit (or not at all).

We may actually want to build this into the command layer between the client and the server, so that the driver will raise an error if there are ever page errors after performing a command.

odlp commented 7 years ago

Ok that's a good direction to head in, I think making the error check apply to all actions makes sense. I'll push this down to the command layer.