-
Notifications
You must be signed in to change notification settings - Fork 0
Add pending logging #5
base: freeagent-logging-branch
Are you sure you want to change the base?
Conversation
…_idle' fails and raises the error
…ait_for_idle' hit's TimeoutError
"because something took a very long time (for example a page load " \ | ||
"was slow). If so, setting the :timeout option to a higher value might " \ | ||
"help." | ||
message = "#{message}\n Connections still pending:\n #{pending_urls.join(', ')}" unless pending_urls.empty? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't a critique of your code here (because I use unless a lot) - but I read this the other day:
Read This Post 'Unless' You're Not A Ruby Developer
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you want to override the whole message string, or just append to it if there are pending urls? (it's replacing it atm)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like: (I think that's how you add to a string.. )
message = "#{message}\n Connections still pending:\n #{pending_urls.join(', ')}" unless pending_urls.empty? | |
if !pending_urls.empty? | |
message =+ "#{message}\n Connections still pending:\n #{pending_urls.join(', ')}" unless pending_urls.empty? | |
end |
if Utils::ElapsedTime.timeout?(start, timeout) | ||
if @page.browser.options.pending_connection_errors | ||
pendings = traffic.select(&:pending?) | ||
pending_urls = pendings.map(&:url).compact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does the pending connection always have a url here? I thought we saw that sometimes it was empty, because the pendings are a collection of Network Exchange objects, from which the url method is request&.url
. I could be wrong here - and it could also be that when we tested last time the exchange didn't have a request (and therefore no url) but when it's run normally they do.
if @page.browser.options.pending_connection_errors | ||
pendings = traffic.select(&:pending?) | ||
pending_urls = pendings.map(&:url).compact | ||
puts("[DEBUG] 'wait_for_idle' pending connections:\n#{pendings.map(&:inspect)}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice idea 👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few nitpicky comments and things to chat about, but I'm also very happy to ship this and iterate if there are future improvements to be made ?
In terms of shipping this PR though - would recommend just keeping it on a branch, then referencing that branch in freeagent, keeping the main/master branch the same as upstream 👍 |
Pointing this to |
Example of this being triggered (stress test run):
Backtrace doesn't show the
In this example the failure comes down to |
What
After reverting this fork back to the upstream
0.13.0
tag, we want to explicitly emit pending connections when we raise aTimeoutError
fromwait_for_idle
.There are three places that
TimeoutError
gets raised, each has it's own context, and so will need custom error handling if we want to get specific useful information out:How
Deliberately triggering a failure in a spec:
Example output:
We're also dumping as much of the pending context as possible: