[chromedriver] Add an error autoreporting feature that automatically raises errors from browser logs

[chromedriver] Add an error autoreporting feature that automatically raises errors from browser logs

BUG=chromedriver:543
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=245432

[frankf, 2014-01-08 01:23:49]

lgtm w/ nits

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/cli...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/cli...
chrome/test/chromedriver/client/chromedriver.py:319:
self.ExecuteCommand(Command.SET_AUTO_REPORTING_ENABLED, {'enable': enable})
i'd name enable -> enabled

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/tes...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/tes...
chrome/test/chromedriver/test/run_py_tests.py:639:
self.assertRaises(chromedriver.UnknownError, self._driver.Load, url)
Can you actually check the message using assertRaisesRegexp?

[chrisgao (Use stgao instead), 2014-01-09 22:11:55]

You could consider addressing some of my comments in a followup CL if you feel
like it.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/cli...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/cli...
chrome/test/chromedriver/client/chromedriver.py:319:
self.ExecuteCommand(Command.SET_AUTO_REPORTING_ENABLED, {'enable': enable})
On 2014/01/08 01:23:49, frankf wrote:
> i'd name enable -> enabled

+1

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:153: std::string
WebDriverLog::GetFirstErrorMessage() const {
How do you handle error messages resulted from the page of navigation1 while we
are on the page of navigation2?

I found there is one DevTools event "Console.messagesCleared". You may have a
check.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:162: if
(level.compare(kLevelToName[Log::kError]) == 0) {
level == kLevelToName[Log::kError] ?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:165: return message;
If we do not clear the error message from the list and the client side doesn't
pull the log(As we return a UnknownError, the client side couldn't tell the
error from others.), in all following commands, we will always hit the same
error or maybe a newer one.

Any idea to handle it?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:170: return "";
"" -> std::string()

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
File chrome/test/chromedriver/logging.h (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.h:42: // Finds the first error message in the
log and returns it. If none exist,
nit: exist -> exists ?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.h:44: std::string GetFirstErrorMessage() const;
What's the consideration behind only returning the first error message, not all
available error messages?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ser...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ser...
chrome/test/chromedriver/server/http_handler.cc:530: "SetAutoReportingEnabled",
"SetAutoReporting" ? It could be both enabled and disabled.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session.cc:102: if
((*it)->type().compare(WebDriverLog::kBrowserType) == 0) {
(*it)->type() == WebDriverLog::kBrowserType ?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session.cc:108: return "";
"" -> std::string()

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands.cc:627: return
ExecuteIsAutoReportingEnabled(session, params, value);
Hmm, do we want to return the new value? It seems there is no such convention
for webdriver api.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_commands_unittest.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands_unittest.cc:120: StatusCode status;
Rename to |status_code|?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands_unittest.cc:127:
value.get()->GetAsBoolean(&enabled);
ASSERT_TRUE?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands_unittest.cc:139:
value.get()->GetAsBoolean(&enabled);
ASSERT_TRUE?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands_unittest.cc:147:
value.get()->GetAsBoolean(&enabled);
ASSERT_TRUE?

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/win...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/win...
chrome/test/chromedriver/window_commands.cc:237: if (status.IsOk() &&
session->auto_reporting_enabled) {
We already check for errors in method ExecuteSessionCommandOnSessionThread of
file chrome/test/chromedriver/commands.cc. Why do we need to check again here?

A window command will be wrapped up into a session command. That's say, if a
window command is executed, we will check for errors twice.

[samuong, 2014-01-16 00:29:28]

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/cli...
File chrome/test/chromedriver/client/chromedriver.py (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/cli...
chrome/test/chromedriver/client/chromedriver.py:319:
self.ExecuteCommand(Command.SET_AUTO_REPORTING_ENABLED, {'enable': enable})
On 2014/01/08 01:23:49, frankf wrote:
> i'd name enable -> enabled

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
File chrome/test/chromedriver/logging.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:153: std::string
WebDriverLog::GetFirstErrorMessage() const {
On 2014/01/09 22:11:55, chrisgao wrote:
> How do you handle error messages resulted from the page of navigation1 while
we
> are on the page of navigation2?
> 
> I found there is one DevTools event "Console.messagesCleared". You may have a
> check.

In this case the error from navigation1 will get autoreported, even though we're
on the page of navigation2. I think this is the best way to do it as it ensures
that error messages don't get missed.

Chromedriver already prepends a URL to each error message, so this should make
it obvious to users which page an error message belongs to.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:162: if
(level.compare(kLevelToName[Log::kError]) == 0) {
On 2014/01/09 22:11:55, chrisgao wrote:
> level == kLevelToName[Log::kError] ?

Done. Obviously I'm still working on kicking the Java habit.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:165: return message;
On 2014/01/09 22:11:55, chrisgao wrote:
> If we do not clear the error message from the list and the client side
doesn't
> pull the log(As we return a UnknownError, the client side couldn't tell the
> error from others.), in all following commands, we will always hit the same
> error or maybe a newer one.
> 
> Any idea to handle it?

When an error is auto-reported, I think most clients will either bail or call
GET_LOGS (which will clear any subsequent error messages). But if they don't,
then I agree that it is a bit weird to throw the same error at them over and
over again.

As you suggested, let's address this in a followup CL, so that we can discuss
exactly how chromedriver should behave in this situation.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.cc:170: return "";
On 2014/01/09 22:11:55, chrisgao wrote:
> "" -> std::string()

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
File chrome/test/chromedriver/logging.h (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.h:42: // Finds the first error message in the
log and returns it. If none exist,
I think it's "if none exist" or "if nothing exists", but not "if none exists".
Frank, let me know if I'm wrong.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/log...
chrome/test/chromedriver/logging.h:44: std::string GetFirstErrorMessage() const;
On 2014/01/09 22:11:55, chrisgao wrote:
> What's the consideration behind only returning the first error message, not
all
> available error messages?

The first error message gets returned to the client, and gets used as the
exception message. I don't think it's necessary to return all error messages,
but if the client is interested it can always call the GET_LOG command.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ser...
File chrome/test/chromedriver/server/http_handler.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ser...
chrome/test/chromedriver/server/http_handler.cc:530: "SetAutoReportingEnabled",
On 2014/01/09 22:11:55, chrisgao wrote:
> "SetAutoReporting" ? It could be both enabled and disabled.

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session.cc:102: if
((*it)->type().compare(WebDriverLog::kBrowserType) == 0) {
On 2014/01/09 22:11:55, chrisgao wrote:
> (*it)->type() == WebDriverLog::kBrowserType ?

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session.cc:108: return "";
On 2014/01/09 22:11:55, chrisgao wrote:
> "" -> std::string()

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_commands.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands.cc:627: return
ExecuteIsAutoReportingEnabled(session, params, value);
On 2014/01/09 22:11:55, chrisgao wrote:
> Hmm, do we want to return the new value? It seems there is no such convention
> for webdriver api.

I thought it might be useful for clients but you're right that the convention
does not exist, so I won't do this for consistency with the rest of the
WebDriver API.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
File chrome/test/chromedriver/session_commands_unittest.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands_unittest.cc:120: StatusCode status;
On 2014/01/09 22:11:55, chrisgao wrote:
> Rename to |status_code|?

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands_unittest.cc:127:
value.get()->GetAsBoolean(&enabled);
On 2014/01/09 22:11:55, chrisgao wrote:
> ASSERT_TRUE?

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands_unittest.cc:139:
value.get()->GetAsBoolean(&enabled);
On 2014/01/09 22:11:55, chrisgao wrote:
> ASSERT_TRUE?

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/ses...
chrome/test/chromedriver/session_commands_unittest.cc:147:
value.get()->GetAsBoolean(&enabled);
On 2014/01/09 22:11:55, chrisgao wrote:
> ASSERT_TRUE?

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/tes...
File chrome/test/chromedriver/test/run_py_tests.py (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/tes...
chrome/test/chromedriver/test/run_py_tests.py:639:
self.assertRaises(chromedriver.UnknownError, self._driver.Load, url)
On 2014/01/08 01:23:49, frankf wrote:
> Can you actually check the message using assertRaisesRegexp?

Done.

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/win...
File chrome/test/chromedriver/window_commands.cc (right):

https://codereview.chromium.org/101203012/diff/1/chrome/test/chromedriver/win...
chrome/test/chromedriver/window_commands.cc:237: if (status.IsOk() &&
session->auto_reporting_enabled) {
On 2014/01/09 22:11:55, chrisgao wrote:
> We already check for errors in method ExecuteSessionCommandOnSessionThread of
> file chrome/test/chromedriver/commands.cc. Why do we need to check again here?
> 
> A window command will be wrapped up into a session command. That's say, if a
> window command is executed, we will check for errors twice.

Done.

[chrisgao (Use stgao instead), 2014-01-17 01:46:46]

lgtm

[commit-bot: I haz the power, 2014-01-17 01:47:43]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/samuong@chromium.org/101203012/130001

[commit-bot: I haz the power, 2014-01-17 06:16:58]

Change committed as 245432