Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
20672002:
[telemetry] Add a webdriver backend with support for IE. (Closed)
|
Created:
7 years, 5 months ago by chrisgao (Use stgao instead) Modified:
7 years, 4 months ago 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. |
Description[telemetry] Add a webdriver backend with support for IE.
BUG=none
TEST=tools/perf/run_measurement --browser=internet-explorer octane & kraken
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=215785
Patch Set 1 #
Total comments: 29
Patch Set 2 : Address comments. #
Total comments: 23
Patch Set 3 : Address comments. #
Total comments: 6
Patch Set 4 : move core/webdriver/* to core/backends/webdriver. #Patch Set 5 : Rebase. #Patch Set 6 : Fix errors after rebase. #
Messages
Total messages: 17 (0 generated)
https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:37: # TODO Currently require that IEDriver.exe is in PATH. IEDriver.exe -> IEDriverServer.exe https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:57: '64' : os.getenv('PROGRAMFILES')} According to https://code.google.com/p/selenium/wiki/InternetExplorerDriver, the "bit-ness" is determined by which version of IEDriverServer.exe is used. We might need to upload both 32-bit and 64-bit of IEDriverServer.exe later on when needed.
Hi Tony, This CL makes telemetry run with IE. But some of the telemetry unittests fail with IE. Do you think we should fix them in this CL? Best wishes, Shuotao Gao
This is awesome. I'm amazing at how small and clean this is and how well it fit into the existing interface. All my comments are minor nits. Dave and/or Nat should also review. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/browser_finder.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/browser_finder.py:53: webdriver_desktop_browser_finder.FindAllAvailableBrowsers(options)) Note: You'll have to rebase after: https://chromiumcodereview.appspot.com/19972003/diff/23001/tools/telemetry/te... https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py:17: self._supports_extensions = supports_extensions Hmm... I wonder whether this needs to be False. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py:52: def supports_tab_control(self): Is there any hope for supporting this with web driver or will it be a limitation? If we'll be able to support it some day, please add a TODO. If it is always going to be unsupported, please add a comment explaining. This comment applies to other not implemented methods in this file. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py:83: class DoNothingForwarder(object): Can this code be shared with browser_backend.py? https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:17: '..', '..', '..', '..', '..', 'third_party', util.GetChromiumSrcDir() https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:22: 'webdriver.ie32', Since these are user-facing names, can we call them "internetexplorer" and "internetexplorer_x64" (for consistency with desktop_browser_finder.py). https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:26: class PossibleIEDesktopBrowser(possible_browser.PossibleBrowser): Should this really be called PossibleIE or just PossibleWebDriver? I'm wondering whether Firefox would use the same class. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:37: # TODO Currently require that IEDriver.exe is in PATH. Can we check for this and display a user-friendly message if it is not in the path? Better yet, can we search for it and add it to the path? https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:57: '64' : os.getenv('PROGRAMFILES')} There is also code in desktop_browser_finder.py that does this. Can we share it? Should we? https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:59: for bit, path in win_search_paths.iteritems(): s/bit/architecture/ https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/webdriver/webdriver_tab_backend.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_tab_backend.py:92: self._browser_backend.driver.set_page_load_timeout(timeout * 1000) Should this be set_script_timeout instead of set_page_load_timeout? https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_tab_backend.py:94: 'return eval(\'%s\')' % expr.replace('\'', '\\\'').replace('\n', '')) Maybe we should replace '\n' with ' '. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/wpr_server.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/wpr_server.py:56: if browser_backend.browser_type.startswith('webdriver.'): I don't think it is good for the wpr_server to explicitly know about browsers. Can we pass a use_dns_forwarding var to __init__ instead?
Thank Tony for the detailed review and suggestions. Will add David and Nat as reviewers. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/browser_finder.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/browser_finder.py:53: webdriver_desktop_browser_finder.FindAllAvailableBrowsers(options)) On 2013/07/26 18:21:07, tonyg wrote: > Note: You'll have to rebase after: > https://chromiumcodereview.appspot.com/19972003/diff/23001/tools/telemetry/te... Done. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py:17: self._supports_extensions = supports_extensions On 2013/07/26 18:21:07, tonyg wrote: > Hmm... I wonder whether this needs to be False. I suggest we leave it as it is. Because Firefox do support extensions. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py:52: def supports_tab_control(self): On 2013/07/26 18:21:07, tonyg wrote: > Is there any hope for supporting this with web driver or will it be a limitation? Base on webdriver protocol API, only closing a tab is supported while active/new a tab is not. Have tried several ways to open a new tab in IE and Firefox(CONTROL+T, click on a link, window.open), but the driver failed to control the new-opened tab. CONTROL+TAB can active tabs iteratively, but can not do it randomly as tab_switching.py does. So, for IE/Firefox drivers, it will be a limitation unless we can do it by ourselves and get LGTM from the owner. I think the work for firefoxdriver won't be that hard, but I doubt whether we should do it right now, because a new version of firefoxdriver is under development. This only impacts tab_switching.py, right? I vote to mark it as limitation. > > If we'll be able to support it some day, please add a TODO. If it is always > going to be unsupported, please add a comment explaining. > > This comment applies to other not implemented methods in this file. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py:83: class DoNothingForwarder(object): On 2013/07/26 18:21:07, tonyg wrote: > Can this code be shared with browser_backend.py? Done. I think it is not a good idea for webdriver backend to depend on chrome backend. So I moved browser_backend.DoNothingForwarder from core/chrome/browser_backend.py up to core/util.py I can do a revert if you don't like it. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:17: '..', '..', '..', '..', '..', 'third_party', On 2013/07/26 18:21:07, tonyg wrote: > util.GetChromiumSrcDir() Thx, this makes it easier and cleaner. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:22: 'webdriver.ie32', On 2013/07/26 18:21:07, tonyg wrote: > Since these are user-facing names, can we call them "internetexplorer" and > "internetexplorer_x64" (for consistency with desktop_browser_finder.py). Done. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:26: class PossibleIEDesktopBrowser(possible_browser.PossibleBrowser): On 2013/07/26 18:21:07, tonyg wrote: > Should this really be called PossibleIE or just PossibleWebDriver? I'm wondering > whether Firefox would use the same class. I don't think firefox can share the same class, as the ways to create a webdriver instance are different. But we can create a shared class PossibleWebDriver and make subclass for IE and firefox. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:37: # TODO Currently require that IEDriver.exe is in PATH. On 2013/07/26 16:58:00, chrisgao wrote: > IEDriver.exe -> IEDriverServer.exe Done. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:37: # TODO Currently require that IEDriver.exe is in PATH. On 2013/07/26 18:21:07, tonyg wrote: > Can we check for this and display a user-friendly message if it is not in the path? We can delegate this to the underlying IE Driver as it already does this. > Better yet, can we search for it and add it to the path? Yes, we can do that after we get both 32-bit and 64-bit IEDriverServer.exe into code repos. Which directory should IEDriverServer.exe live in? tools/telemetry/third_party/webdrivers? https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:57: '64' : os.getenv('PROGRAMFILES')} On 2013/07/26 18:21:07, tonyg wrote: > There is also code in desktop_browser_finder.py that does this. Can we share it? > Should we? I'm afraid we can't share much of the code, because it seems quite different from that in desktop_browser_finder.py. Let's keep as it is. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:59: for bit, path in win_search_paths.iteritems(): On 2013/07/26 18:21:07, tonyg wrote: > s/bit/architecture/ Done. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/webdriver/webdriver_tab_backend.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_tab_backend.py:92: self._browser_backend.driver.set_page_load_timeout(timeout * 1000) On 2013/07/26 18:21:07, tonyg wrote: > Should this be set_script_timeout instead of set_page_load_timeout? Whoops.. After double check on the webdriver protocol, set_script_timeout is for asynchronous JS execution and not for synchronous execution/evaluation. Thus, removed. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/webdriver/webdriver_tab_backend.py:94: 'return eval(\'%s\')' % expr.replace('\'', '\\\'').replace('\n', '')) On 2013/07/26 18:21:07, tonyg wrote: > Maybe we should replace '\n' with ' '. Done. https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... File tools/telemetry/telemetry/core/wpr_server.py (right): https://codereview.chromium.org/20672002/diff/1/tools/telemetry/telemetry/cor... tools/telemetry/telemetry/core/wpr_server.py:56: if browser_backend.browser_type.startswith('webdriver.'): On 2013/07/26 18:21:07, tonyg wrote: > I don't think it is good for the wpr_server to explicitly know about browsers. > Can we pass a use_dns_forwarding var to __init__ instead? Done as discussed offline.
pretty neat. some comments. https://codereview.chromium.org/20672002/diff/9001/tools/perf/benchmarks/octa... File tools/perf/benchmarks/octane.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/perf/benchmarks/octa... tools/perf/benchmarks/octane.py:25: if (result_divs[r].id && result_divs[r].id.indexOf('Result-') == 0) { lets land this fix right away as a separate cl https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/chrome/browser_backend.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/chrome/browser_backend.py:35: self.webdriver_based = False why is this needed? This is like having a isCrOS on the browserbackend, which of course we'd say no to. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:21: from selenium import webdriver # pylint: disable=F0401 What if this import fails for some reason? Can we make this fail more gracefully? We need to update the telemetry bootstrap if you're going to make this a "telemetry will explode if this dir isn't found" kind of error https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:25: 'internetexplorer_x64']) i think we have a naming convention for multi words... look at android content shell ... i forget what the convention is, but lets be consistent. 64 is just another word, so it should either be internet-explorer-64 or internet_explorer_64 dependnign on precident https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:35: if sys.platform.startswith('linux'): would a telemetry.core.platform.util.CreatePlatformBackendForCurrentOS() be appropriate here and in desktop browser as well? https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:54: return True really? probably a TODO here no? https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:70: # TODO Currently require that IEDriverServer.exe is in PATH. # TODO(username): Is our convention. Also, phrase this as a todo. This reads as a requirement statement rather than saying what needs to be done. What is our plan for followup tasks? Telemetry bugs? Lets get one filed. TODOs come in two flavors: things that are bugs that we can live with never fixing, or things that need fixing. This is I think a "needs fixing" one, so we should file a bug for it. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/wpr_server.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/wpr_server.py:56: if browser_backend.webdriver_based: lets do this by constructing the options list first,and browser_backend.CustomizeReplayServerWithOptions(options) Then create the replay server. That lets the backends have a last-second chance to customize the server, and you avoid the webdriver_based flag
Looking really close. +1 to all of Nat's comments. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/browser_finder.py (left): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/browser_finder.py:20: The style guide wants 2 blank lines between top level elements, so you can leave this line in if you like. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py:12: WEBPAGEREPLAY_HOST = '127.0.0.1' Is this unused? If not, can we share it with the same constant elsewhere?
https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/chrome/browser_backend.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/chrome/browser_backend.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. I think the right thing to do is create another folder and put browser_backend in there, so both chrome and webdriver can inherit from it. telemetry/core/backends/browser_backend.py telemetry/core/backends/chrome/chrome_browser_backend.py telemetry/core/backends/webdriver/webdriver_browser_backend.py This way, browser_backend is accessible by both, but still kept separate from the "public" methods in telemetry/core/ It may seem unjustified to do a big rename just to move one file, but I think we're going to have a lot more things moving from core/chrome/ to there as this effort progresses.
On 2013/07/31 01:34:48, Dave Tu wrote: > https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... > File tools/telemetry/telemetry/core/chrome/browser_backend.py (right): > > https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... > tools/telemetry/telemetry/core/chrome/browser_backend.py:1: # Copyright (c) 2012 > The Chromium Authors. All rights reserved. > I think the right thing to do is create another folder and put browser_backend > in there, so both chrome and webdriver can inherit from it. > > telemetry/core/backends/browser_backend.py > telemetry/core/backends/chrome/chrome_browser_backend.py > telemetry/core/backends/webdriver/webdriver_browser_backend.py > > This way, browser_backend is accessible by both, but still kept separate from > the "public" methods in telemetry/core/ > > It may seem unjustified to do a big rename just to move one file, but I think > we're going to have a lot more things moving from core/chrome/ to there as this > effort progresses. That's probably best as its own patch.
Many thanks for your comments and suggestions. 26 telemetry unittests fail with IE, do you think we should fix/disable them, or just ignore them. If we want to fix them, I'd like to do it in a separate CL. https://codereview.chromium.org/20672002/diff/9001/tools/perf/benchmarks/octa... File tools/perf/benchmarks/octane.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/perf/benchmarks/octa... tools/perf/benchmarks/octane.py:25: if (result_divs[r].id && result_divs[r].id.indexOf('Result-') == 0) { On 2013/07/30 06:49:45, nduca wrote: > lets land this fix right away as a separate cl Done in https://codereview.chromium.org/20987007/. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/browser_finder.py (left): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/browser_finder.py:20: On 2013/07/30 19:35:34, tonyg wrote: > The style guide wants 2 blank lines between top level elements, so you can leave > this line in if you like. Whopps, Done. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/chrome/browser_backend.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/chrome/browser_backend.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/31 01:34:49, Dave Tu wrote: > I think the right thing to do is create another folder and put browser_backend > in there, so both chrome and webdriver can inherit from it. > > telemetry/core/backends/browser_backend.py > telemetry/core/backends/chrome/chrome_browser_backend.py > telemetry/core/backends/webdriver/webdriver_browser_backend.py > > This way, browser_backend is accessible by both, but still kept separate from > the "public" methods in telemetry/core/ > > It may seem unjustified to do a big rename just to move one file, but I think > we're going to have a lot more things moving from core/chrome/ to there as this > effort progresses. Thanks, Dave. Will do it in next upload and a separate CL. Per your suggestion, we'd better to move telemetry/core/webdriver/* to telemetry/core/backends/webdriver, right? https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/chrome/browser_backend.py:35: self.webdriver_based = False On 2013/07/30 06:49:45, nduca wrote: > why is this needed? This is like having a isCrOS on the browserbackend, which of > course we'd say no to. Removed. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_browser_backend.py:12: WEBPAGEREPLAY_HOST = '127.0.0.1' On 2013/07/30 19:35:34, tonyg wrote: > Is this unused? If not, can we share it with the same constant elsewhere? Found chrome backend and webdriver backend share a lot of code, including this constant. Added core/browser_backend.py that can be shared by backends of chrome and webdriver. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:21: from selenium import webdriver # pylint: disable=F0401 On 2013/07/30 06:49:45, nduca wrote: > What if this import fails for some reason? Can we make this fail more > gracefully? > > We need to update the telemetry bootstrap if you're going to make this a > "telemetry will explode if this dir isn't found" kind of error Let's do it later. crbug.com/266177 was filed. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:25: 'internetexplorer_x64']) On 2013/07/30 06:49:45, nduca wrote: > i think we have a naming convention for multi words... look at android content > shell ... i forget what the convention is, but lets be consistent. 64 is just > another word, so it should either be internet-explorer-64 or > internet_explorer_64 dependnign on precident Changed to "internet-explorer-x64". Used x64 because we already have release_x64 and debug_x64 for desktop chrome. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:35: if sys.platform.startswith('linux'): On 2013/07/30 06:49:45, nduca wrote: > would a telemetry.core.platform.util.CreatePlatformBackendForCurrentOS() be > appropriate here and in desktop browser as well? Done. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:54: return True On 2013/07/30 06:49:45, nduca wrote: > really? probably a TODO here no? Done. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/webdriver/webdriver_desktop_browser_finder.py:70: # TODO Currently require that IEDriverServer.exe is in PATH. On 2013/07/30 06:49:45, nduca wrote: > # TODO(username): Is our convention. > > Also, phrase this as a todo. This reads as a requirement statement rather than > saying what needs to be done. > > What is our plan for followup tasks? Telemetry bugs? Lets get one filed. TODOs > come in two flavors: things that are bugs that we can live with never fixing, or > things that need fixing. This is I think a "needs fixing" one, so we should file > a bug for it. Thanks for detailed explanation. crbug.com/266170 was filed. https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/wpr_server.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/wpr_server.py:56: if browser_backend.webdriver_based: On 2013/07/30 06:49:45, nduca wrote: > lets do this by constructing the options list first,and > > browser_backend.CustomizeReplayServerWithOptions(options) > > Then create the replay server. > > That lets the backends have a last-second chance to customize the server, and > you avoid the webdriver_based flag Good idea. Done. But we need to override webpagereplay.ReplayServer._AddDefaultReplayOptions. https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/webdriver/webdriver_tab_list_backend.py (right): https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/webdriver/webdriver_tab_list_backend.py:17: raise NotImplementedError() Add a comment with a reason of not-impl. https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/wpr_server.py (right): https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/wpr_server.py:22: """Override.Because '--no-dns_forwarding' is added by default in parent "Override.Because" -> "Override. Because"
The list of failed unittests is as below: [ FAILED ] 26 tests, listed below: [ FAILED ] BrowserTest.testVersionDetection [ FAILED ] ComponentExtensionTest.testComponentExtensionBasic [ FAILED ] ExtensionTest.testDisconnect [ FAILED ] ExtensionTest.testExtensionBasic [ FAILED ] MultipleExtensionTest.testMultipleExtensions [ FAILED ] TabConsoleTest.testConsoleOutputStream [ FAILED ] InspectorMemoryTest.testGetDOMStats [ FAILED ] InspectorPageTest.testCustomActionToNavigate [ FAILED ] InspectorPageTest.testScriptToEvaluateOnCommit [ FAILED ] InspectorRuntimeTest.testRuntimeEvaluateOfSomethingThatCantJSONize [ FAILED ] InspectorRuntimeTest.testRuntimeEvaluateThatFails [ FAILED ] InspectorTimelineTabTest.testGotTimeline [ FAILED ] ClickElementActionTest.testClickWithTextWaitForRefChange [ FAILED ] PlayActionTest.testPlayWaitForEnded [ FAILED ] PlayActionTest.testPlayWithAllSelector [ FAILED ] PlayActionTest.testPlayWithNoSelector [ FAILED ] PlayActionTest.testPlayWithVideoSelector [ FAILED ] ScrollActionTest.testScrollAction [ FAILED ] SeekActionTest.testSeekWithAllSelector [ FAILED ] SeekActionTest.testSeekWithNoSelector [ FAILED ] SeekActionTest.testSeekWithVideoSelector [ FAILED ] BrowserTest.testCommandLineOverriding [ FAILED ] TabTest.testRendererCrash [ FAILED ] UserAgentTest.testUserAgent [ FAILED ] PageRunnerTests.testHandlingOfCrashedTab [ FAILED ] PageRunnerTests.testUserAgent
lg2m from my perspective. Please wait for dtu@ to approve. Regarding the unittests, I think we should leave them broken in the CL and then fix them bit by bit in follow up patches. With each unittest we fix, we'll probably get support for additional Measurements. https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/util.py (right): https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/util.py:12: def CreatePlatformBackendForCurrentOS(): Maybe this should go in platform/__init__.py instead of creating a util.py.
lgtm https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... File tools/telemetry/telemetry/core/chrome/browser_backend.py (right): https://codereview.chromium.org/20672002/diff/9001/tools/telemetry/telemetry/... tools/telemetry/telemetry/core/chrome/browser_backend.py:1: # Copyright (c) 2012 The Chromium Authors. All rights reserved. On 2013/07/31 19:05:41, chrisgao wrote: > On 2013/07/31 01:34:49, Dave Tu wrote: > > I think the right thing to do is create another folder and put browser_backend > > in there, so both chrome and webdriver can inherit from it. > > > > telemetry/core/backends/browser_backend.py > > telemetry/core/backends/chrome/chrome_browser_backend.py > > telemetry/core/backends/webdriver/webdriver_browser_backend.py > > > > This way, browser_backend is accessible by both, but still kept separate from > > the "public" methods in telemetry/core/ > > > > It may seem unjustified to do a big rename just to move one file, but I think > > we're going to have a lot more things moving from core/chrome/ to there as > this > > effort progresses. > > Thanks, Dave. Will do it in next upload and a separate CL. > Per your suggestion, we'd better to move telemetry/core/webdriver/* to > telemetry/core/backends/webdriver, right? Yep.
Stuff withOUT regard to webdriver backend are separated out into CL https://codereview.chromium.org/21406004/. https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/platform/util.py (right): https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/platform/util.py:12: def CreatePlatformBackendForCurrentOS(): On 2013/07/31 20:59:33, tonyg wrote: > Maybe this should go in platform/__init__.py instead of creating a util.py. Done in https://codereview.chromium.org/21406004/. https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/webdriver/webdriver_tab_list_backend.py (right): https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/webdriver/webdriver_tab_list_backend.py:17: raise NotImplementedError() On 2013/07/31 19:05:41, chrisgao wrote: > Add a comment with a reason of not-impl. Done. https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... File tools/telemetry/telemetry/core/wpr_server.py (right): https://codereview.chromium.org/20672002/diff/26001/tools/telemetry/telemetry... tools/telemetry/telemetry/core/wpr_server.py:22: """Override.Because '--no-dns_forwarding' is added by default in parent On 2013/07/31 19:05:41, chrisgao wrote: > "Override.Because" -> "Override. Because" Done in https://codereview.chromium.org/21406004/.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisgao@chromium.org/20672002/60001
Retried try job too often on mac_rel for step(s) remoting_unittests http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/chrisgao@chromium.org/20672002/60001
Message was sent while issue was closed.
Change committed as 215785 |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
