add error message detail for page load failure #311

Closed
wants to merge 8 commits into
from

Conversation

Projects
None yet
4 participants

to easily debug "Unable to load URL" errors.

When we use capybara-webkit with cucumber, sometimes we hit WebkitInvalidResponseError, "Unable to load URL..." but have difficulty finding the particular asset that is failing. This change is meant to speed up debugging of these problems by collecting the actual failing URL from Webkit and packing it into the error message.

@jferris jferris commented on an outdated diff Apr 3, 2012

src/WebPage.cpp
@@ -134,7 +135,11 @@ bool WebPage::isLoading() const {
}
QString WebPage::failureString() {
- return QString("Unable to load URL: ") + currentFrame()->requestedUrl().toString();
+ QString message = QString("Unable to load URL: ") + currentFrame()->requestedUrl().toString();
+ if(!m_errorPageMessage.isEmpty())
@jferris

jferris Apr 3, 2012

Owner

Can you reverse the clauses here so we don't have an "if not?"

@jferris jferris and 1 other commented on an outdated diff Apr 3, 2012

spec/driver_spec.rb
@@ -4,7 +4,7 @@
describe Capybara::Driver::Webkit do
subject { Capybara::Driver::Webkit.new(@app, :browser => $webkit_browser) }
before { subject.visit("/hello/world?success=true") }
- after { subject.reset! }
+ after { subject.reset! rescue nil }
@jferris

jferris Apr 3, 2012

Owner

This will swallow potentially valid errors. What's the rescue for here?

@joshuanapoli

joshuanapoli Apr 3, 2012

This was the simplest way to deal with reset! raising WebkitInvalidResponseError after a failed #visit.

When PageLoadingCommand gets an error (as happens in this pull with the new rspec example), the driver can raise WebkitInvalidResponseError twice. From the initial #visit, PageLoadingCommand::pendingLoadFinished returns an error status. The next command (in this case, Reset) will also return an error status because Connection::m_pageSuccess will be false.

I think that changing the driver so that Reset returns success after a failed Visit (and removing "rescue nil") would be an improvement. How does that sound?

@jferris

jferris Apr 3, 2012

Owner

That sounds perfect.

Owner

jferris commented Apr 3, 2012

Thanks for the pull request. I think this is a useful change, but I had a couple of comments about the code. Can you take a look at those?

Joshua Napoli added some commits Apr 4, 2012

Joshua Napoli page load failure should not cause the next command to fail 5831441
Joshua Napoli add error message detail for page load failure
to easily debug "Unable to load URL" errors.
142047c

Ok, all done. A page load failure no longer causes the subsequent command to fail (and the corresponding "rescue nil" is removed), and the condition in the message formatting logic is ordered to avoid the unnecessary negation.

@jferris would you like any other changes to this pull request?

Owner

jferris commented Apr 9, 2012

@joshuanapoli I'm sorry, but I haven't gotten a chance to take another look yet. I'll get to this as soon as I can.

@halogenandtoast halogenandtoast commented on the diff May 4, 2012

spec/driver_spec.rb
+ @app = lambda do |env|
+ case env["PATH_INFO"]
+ when "/inner-not-found"
+ [404, {}, []]
+ when "/outer"
+ body = <<-HTML
+ <html>
+ <body>
+ <iframe src=\"/inner-not-found\"></iframe>
+ </body>
+ </html>
+ HTML
+ [200,
+ { 'Content-Type' => 'text/html', 'Content-Length' => body.length.to_s },
+ [body]]
+ else
@halogenandtoast

halogenandtoast May 4, 2012

Contributor

Is this else needed?

@joshuanapoli

joshuanapoli May 31, 2012

The else clause handles the block at line 6, before { subject.visit("/hello/world?success=true") }

Joshua Napoli added some commits May 4, 2012

Joshua Napoli remove debug output 9cbf7d2
Joshua Napoli Merge branch 'master' of https://github.com/thoughtbot/capybara-webkit
Conflicts:
	src/WebPage.cpp
	src/WebPage.h
7b4a3ba
Joshua Napoli Merge branch 'multiple-loadFinished-with-timers'
Conflicts:
	spec/driver_spec.rb
0aac807

@mike-burns mike-burns and 1 other commented on an outdated diff May 31, 2012

spec/driver_spec.rb
+ @app = lambda do |env|
+ case env["PATH_INFO"]
+ when "/success"
+ [200, {'Content-Type' => 'text/html'}, ['<html><body></body></html>']]
+ when "/not-found"
+ [404, {}, []]
+ when "/outer"
+ body = <<-HTML
+ <html>
+ <head>
+ <script>
+ // emits WebPage::loadFinished(true) when called from timer
+ function goodFrame(){var divTag = document.createElement("div");divTag.innerHTML = "<iframe src='/success'></iframe>";document.body.appendChild(divTag);};
+
+ // emits WebPage::loadFinished(false) when called from timer
+ function badFrame(){var divTag = document.createElement("div");divTag.innerHTML = "<iframe src='/not-found'></iframe>";document.body.appendChild(divTag);};
@mike-burns

mike-burns May 31, 2012

Owner

How about naming these functions emit_true_load_finished and emit_false_load_finished, and deleting the comments.

@joshuanapoli

joshuanapoli May 31, 2012

I used the names that you suggested in commit 000f327.

@mike-burns mike-burns commented on the diff May 31, 2012

src/WebPage.cpp
if (extension == ChooseMultipleFilesExtension) {
QStringList names = QStringList() << getLastAttachedFileName();
static_cast<ChooseMultipleFilesExtensionReturn*>(output)->fileNames = names;
return true;
}
+ else if (extension == QWebPage::ErrorPageExtension) {
+ ErrorPageExtensionOption *errorOption = (ErrorPageExtensionOption*) option;
+ m_errorPageMessage = "Because of error loading " + errorOption->url.toString() + ": " + errorOption->errorString;
@mike-burns

mike-burns May 31, 2012

Owner

This message is combined with Unable to load URL: http://example.com/ with a colon; not sure if the formatting you have here makes sense for that.

How about this: drop the colon on line 161 and downcase the 'B' in "Because".

Unable to load URL http://example.com/ because of an error loading http://example.net/: could not find The Net.
@joshuanapoli

joshuanapoli May 31, 2012

I improved the message formatting as you suggested in commit 1b99d2d.

Owner

mike-burns commented May 31, 2012

The syntax you use for conditionals are inconsistent---spacing before the paren (if(foo) vs if (foo)) and the placement of newlines are all different.

I changed the spacing to be consistent with the gem's style in commit 000f7da.

Thanks for your suggestions! I've addressed all of the issues.

Owner

jferris commented May 31, 2012

Thanks. I merged this into master.

jferris closed this May 31, 2012

Awesome! Thanks!

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