Issue 23572025: [telemetry] Add webdriver_ie_backend. - Code Review

Chromium Code Reviews

chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out

(292)

My Issues | Starred Open | Closed | All

Issue 23572025: [telemetry] Add webdriver_ie_backend. (Closed)

Created:
7 years, 3 months ago by chrisgao (Use stgao instead)

Modified:
6 years, 3 months ago

Reviewers:
dtu

CC:
chromium-reviews, chrome-speed-team+watch_google.com, telemetry+watch_chromium.org

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Visibility:
Public.

More Reviews

Description

[telemetry] Add webdriver_ie_backend. CTRL+C will make IEDriverServer exits and leaves IE running. During cleanup, kill the IE processes when needed. BUG=282480 Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=221606

Patch Set 1 #

Total comments: 4

Patch Set 2 : Fix nit. #

Total comments: 16

Patch Set 3 : Address comments. #

Total comments: 2

Patch Set 4 : Address comments. #

Created: 7 years, 3 months ago

Download [raw] [tar.bz2]

		Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+91 lines, -37 lines)			Patch
	M	tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py	View	1 2	2 chunks	+1 line, -5 lines	0 comments	Download
	M	tools/telemetry/telemetry/core/backends/webdriver/webdriver_desktop_browser_finder.py	View		4 chunks	+8 lines, -7 lines	0 comments	Download
	A	tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py	View	1 2 3	1 chunk	+43 lines, -0 lines	0 comments	Download
	M	tools/telemetry/telemetry/core/platform/win_platform_backend.py	View	1 2 3	2 chunks	+39 lines, -25 lines	0 comments	Download

Messages

Total messages: 9 (0 generated)

Expand Messages | Collapse Messages

chrisgao (Use stgao instead)

https://codereview.chromium.org/23572025/diff/1/tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py File tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py (right): https://codereview.chromium.org/23572025/diff/1/tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py#newcode7 tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py:7: from telemetry.core import platform remove https://codereview.chromium.org/23572025/diff/1/tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py#newcode27 tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py:27: self.win = ...

7 years, 3 months ago (2013-08-31 00:56:21 UTC) #1

chrisgao (Use stgao instead)

Hey Dave, would you mind taking a review of this CL? https://codereview.chromium.org/23572025/diff/1/tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py File tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py (right): ...

7 years, 3 months ago (2013-09-03 19:59:27 UTC) #2

dtu

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py File tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py (right): https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py#newcode65 tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py:65: def GetStandardOutput(self): Do you have to implement this at ...

7 years, 3 months ago (2013-09-03 22:39:02 UTC) #3

chrisgao (Use stgao instead)

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py File tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py (right): https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py#newcode65 tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py:65: def GetStandardOutput(self): On 2013/09/03 22:39:02, Dave Tu wrote: > ...

7 years, 3 months ago (2013-09-04 00:47:44 UTC) #4

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
File
tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py
(right):

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
tools/telemetry/telemetry/core/backends/webdriver/webdriver_browser_backend.py:65:
def GetStandardOutput(self):
On 2013/09/03 22:39:02, Dave Tu wrote:
> Do you have to implement this at all? Can you just leave it out?

Hm., it seems it is never used. Removed.

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
File tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py
(right):

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py:25:
def pid(self):
On 2013/09/03 22:39:02, Dave Tu wrote:
> It seems unfortunate that WebDriver doesn't either clean up for us or provide
us
> with its child's pid...

Yes, WebDriver doesn't provide much details.
But we can get the pids of its children by win_platform_backend.GetChildPids

By "clean up", do you mean that IEDriverServer should clean up IE sub-processes
when it exits?

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py:39:
except:  # pylint: disable=W0702
On 2013/09/03 22:39:02, Dave Tu wrote:
> What is the exception you're expecting?

For CTRL+C, it is urllib2.URLError.
I'm not sure whether there is any other corner case. That's why I didn't specify
the exact exception.

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py:45: _,
ppid, pid = ppid_pid.split(',')
On 2013/09/03 22:39:02, Dave Tu wrote:
> Looks similar to the pid property above. Can we merge the two?

Moved to win_platform_backend.GetSystemProcessInfo

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
File tools/telemetry/telemetry/core/platform/win_platform_backend.py (right):

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
tools/telemetry/telemetry/core/platform/win_platform_backend.py:103: def
KillProcess(self, pid, kill_process_tree=False):
On 2013/09/03 22:39:02, Dave Tu wrote:
> Comment that os.kill for Windows is Python 2.7

Done.

https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/tel...
tools/telemetry/telemetry/core/platform/win_platform_backend.py:124: _,
creation, curr_ppid, curr_pid = creation_ppid_pid.split(',')
On 2013/09/03 22:39:02, Dave Tu wrote:
> Can we move more of this output parsing to GetSystemProcessInfo? I'd be fine
> with it even if it always returned a dict with all 5 fields.

I had the same thought.
Done. Now it always returns 5 fields.

chrisgao (Use stgao instead)

Hi Dave, Would you mind taking a review of this CL? Thanks, Shuotao Gao

7 years, 3 months ago (2013-09-05 17:24:50 UTC) #5

dtu

lgtm https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py File tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py (right): https://chromiumcodereview.appspot.com/23572025/diff/5001/tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py#newcode25 tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py:25: def pid(self): On 2013/09/04 00:47:45, chrisgao wrote: > ...

7 years, 3 months ago (2013-09-05 20:26:02 UTC) #6

chrisgao (Use stgao instead)

https://codereview.chromium.org/23572025/diff/5001/tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py File tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py (right): https://codereview.chromium.org/23572025/diff/5001/tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py#newcode25 tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py:25: def pid(self): On 2013/09/05 20:26:02, Dave Tu wrote: > ...

7 years, 3 months ago (2013-09-06 01:17:18 UTC) #7

https://codereview.chromium.org/23572025/diff/5001/tools/telemetry/telemetry/...
File tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py
(right):

https://codereview.chromium.org/23572025/diff/5001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py:25:
def pid(self):
On 2013/09/05 20:26:02, Dave Tu wrote:
> On 2013/09/04 00:47:45, chrisgao wrote:
> > On 2013/09/03 22:39:02, Dave Tu wrote:
> > > It seems unfortunate that WebDriver doesn't either clean up for us or
> provide
> > us
> > > with its child's pid...
> > 
> > Yes, WebDriver doesn't provide much details.
> > But we can get the pids of its children by win_platform_backend.GetChildPids
> > 
> > By "clean up", do you mean that IEDriverServer should clean up IE
> sub-processes
> > when it exits?
> 
> Yeah, would be nice :)

Unfortunately, IEDriverServer doesn't do that -_-

https://codereview.chromium.org/23572025/diff/5001/tools/telemetry/telemetry/...
tools/telemetry/telemetry/core/backends/webdriver/webdriver_ie_backend.py:39:
except:  # pylint: disable=W0702
On 2013/09/05 20:26:02, Dave Tu wrote:
> On 2013/09/04 00:47:45, chrisgao wrote:
> > On 2013/09/03 22:39:02, Dave Tu wrote:
> > > What is the exception you're expecting?
> > 
> > For CTRL+C, it is urllib2.URLError.
> > I'm not sure whether there is any other corner case. That's why I didn't
> specify
> > the exact exception.
> 
> Please, never catch all exceptions. We can add more cases as needed. The fact
> that it's UrlError is weird, where is that being thrown from? It should be a
> KeyboardInterrupt exception. Is someone else catching the wrong exception and
> throwing a different one?

This exception is a different one from the KeyboardInterrupt.
None catches KeyboardInterrupt.

After each test run, page_runner will stop the browser in a try-finally clause
(https://code.google.com/p/chromium/codesearch#chromium/src/tools/telemetry/te...).

And the error is thrown by the webdriver python client which sends an HTTP
request to IEDriverServer to close IE. But IEDriverServer exits already at that
moment, and that results in the URLError.

https://codereview.chromium.org/23572025/diff/15001/tools/telemetry/telemetry...
File tools/telemetry/telemetry/core/platform/win_platform_backend.py (right):

https://codereview.chromium.org/23572025/diff/15001/tools/telemetry/telemetry...
tools/telemetry/telemetry/core/platform/win_platform_backend.py:126: pi['Name']
= parts[-3]
On 2013/09/05 20:26:02, Dave Tu wrote:
> Please use a consistent naming style.

Done.

commit-bot: I haz the power

CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisgao@chromium.org/23572025/23001

7 years, 3 months ago (2013-09-06 01:19:45 UTC) #8

Message was sent while issue was closed.

Change committed as 221606

Expand Messages | Collapse Messages

Powered by Google App Engine

This is Rietveld 408576698