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.

Sign up for GitHub

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

Jump to bottom

Detect javascript errors when visiting a new web page #185

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Detect javascript errors when visiting a new web page #185

wants to merge 2 commits into from

Conversation

TerjeBr
Copy link

@TerjeBr TerjeBr commented Nov 23, 2017 •
edited
Loading

This makes it possible to see what is going on if you visit a web page and it has javascript errors, or the page fails to load for other reasons (f.ex. the server might be down).

Without this fix, after you visit you just get http code 0 instead of 200, and you will not know what is going on.

aik099
aik099 reviewed Nov 23, 2017
View reviewed changes
src/ZombieDriver.php
@@ -125,7 +125,10 @@ public function visit($url)
}
});
JS;
$this->server->evalJS($js);
$out = $this->server->evalJS($js);
Copy link
Member

Choose a reason for hiding this comment

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

  1. Are you sure, that JavaScript errors (if any) will be returned as method call result?
  2. Does same error catching technique apply to other js-enabled drivers, like Selenium?

Copy link
Member

Choose a reason for hiding this comment

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

@aik099 visiting a URL in Selenium uses a native Selenium method. So we don't need to implement error handling (Selenium does).

Copy link
Member

Choose a reason for hiding this comment

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

@aik099 see the JS code just above. On error, it outputs some stuff to the stream.

Copy link
Author

Choose a reason for hiding this comment

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

The zombie "browser" object's method "visit" has a callback as its second argument that will be called if an error occurs. Currently this error information is just thrown away. I found it very helpful to have this error information in my project when making behat tests.

Copy link
Author

Choose a reason for hiding this comment

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

@stof has been quiet on this. I guess it is ok as it is then?

stof
stof reviewed Nov 23, 2017
View reviewed changes
src/ZombieDriver.php Outdated
$this->server->evalJS($js);
$out = $this->server->evalJS($js);
if (!empty($out)) {
throw new DriverException(sprintf('Error when loading page %s: %s', $url, $out));
Copy link
Member

Choose a reason for hiding this comment

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

The right fix would rather be to prefix the error output with ZombieServer::ERROR_PREFIX and then evalJS will throw the exception for us.

Copy link
Author

@TerjeBr TerjeBr Nov 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Then you will not get the same degree of specialized error message. You will get a more general error message, and you will not readily see the url that it tried to visit.

You can also see that the same coding pattern is applied to the click function at line 719, and also in triggerBrowserEvent at line 890

@TerjeBr
Copy link
Author

TerjeBr commented Jan 26, 2018

@aik099 Anything I can do to get this PR accepted?

@aik099
Copy link
Member

aik099 commented Jan 26, 2018

I guess you can show error output example of your code compared to what it would be for @stof recommended approach. That should help @stof decide if your approach would be better.

@TerjeBr
Copy link
Author

TerjeBr commented May 14, 2018 •
edited
Loading

Here is an example of what happens when the php server is runnung at the not expected port.
First with my fix:

    And I am on the web page "view quote" with id "300"                                              # WebContext::iAmOnTheWebPageWithId()
      Error when loading page http://localhost:8383/quote/300: "TypeError: connect ECONNREFUSED 127.0.0.1:8383\n    at /usr/lib/node_modules/zombie/lib/pipeline.js:89:15\n    at tryCatcher (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/util.js:16:23)\n    at Promise._settlePromiseFromHandler (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/promise.js:512:31)\n    at Promise._settlePromise (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/promise.js:569:18)\n    at Promise._settlePromise0 (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/promise.js:614:10)\n    at Promise._settlePromises (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/promise.js:689:18)\n    at Async._drainQueue (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/async.js:133:16)\n    at Async._drainQueues (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/async.js:143:10)\n    at Immediate.Async.drainQueues (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/async.js:17:14)\n    at runCallback (timers.js:651:20)\n    at tryOnImmediate (timers.js:624:5)\n    at processImmediate [as _immediateCallback] (timers.js:596:5)" (Behat\Mink\Exception\DriverException)

Here is how it will look if we do it the way @stof suggested:

    And I am on the web page "view quote" with id "300"                                              # WebContext::iAmOnTheWebPageWithId()
      Error "TypeError: connect ECONNREFUSED 127.0.0.1:8383
          at /usr/lib/node_modules/zombie/lib/pipeline.js:89:15
          at tryCatcher (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/util.js:16:23)
          at Promise._settlePromiseFromHandler (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/promise.js:512:31)
          at Promise._settlePromise (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/promise.js:569:18)
          at Promise._settlePromise0 (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/promise.js:614:10)
          at Promise._settlePromises (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/promise.js:689:18)
          at Async._drainQueue (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/async.js:133:16)
          at Async._drainQueues (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/async.js:143:10)
          at Immediate.Async.drainQueues (/usr/lib/node_modules/zombie/node_modules/bluebird/js/release/async.js:17:14)
          at runCallback (timers.js:651:20)
          at tryOnImmediate (timers.js:624:5)
          at processImmediate [as _immediateCallback] (timers.js:596:5)" while executing code: pointers = [];
      browser.visit("http://localhost:8383/quote/300", function (err) {
        if (err) {
          stream.end('CAUGHT_ERROR:' + JSON.stringify(err.stack));
        } else {
          stream.end();
        }
      }); (Behat\Mink\Exception\DriverException)

You see that the second error message has no information the url of that zombie tried to connect to when the error happened.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@stof stof stof left review comments

@aik099 aik099 aik099 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@TerjeBr @aik099 @stof