Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
760653002:
Telemetry --device (Closed)
DescriptionTelemetry --device
Enable Telemetry to discover devices and find browsers for each device.
BUG=435723
Committed: https://crrev.com/d5f6ea98506d437a71afca8ea937c3fc380d1532
Cr-Commit-Position: refs/heads/master@{#314420}
Patch Set 1 #
Total comments: 4
Patch Set 2 : --device=list #Patch Set 3 : --browser=list hierarchical output #Patch Set 4 : browser selection #
Total comments: 4
Patch Set 5 : 3rd version #
Total comments: 3
Patch Set 6 : 4th version #
Total comments: 10
Patch Set 7 : review fix #
Total comments: 9
Patch Set 8 : nit #
Total comments: 2
Patch Set 9 : rebase #Patch Set 10 : review fix #Patch Set 11 : tests #
Total comments: 4
Patch Set 12 : review fix #
Total comments: 15
Patch Set 13 : review fix #
Total comments: 4
Patch Set 14 : review fix #Patch Set 15 : rebase #Messages
Total messages: 58 (7 generated)
zhenw@chromium.org changed reviewers: + dtu@chromium.org, tonyg@chromium.org
Given Nat's comment at https://code.google.com/p/chromium/issues/detail?id=435723#c7, I agree that Platform and Device have different life cycle. So currently I still implement Device separate from Platform. The 1st patch enables "--device=list" and moves some browser finder functionality to the device file, e.g., checking if there are available device/browser. Currently the call sequence is still run_tests->browser_finder->device->GetPlatform. I add FindAllAvailableBrowsers() member method to Device classes (not implmeneted yet). So in the future, it will become run_tests->device->browser_finder->GetPlatform. No tests are updated/added yet. Want to get some feedback on if this approach is worth pursuing first.
tonyg@chromium.org changed reviewers: + achuith@chromium.org
This is awesome. A couple of nits/questions -- other than that, I think this is about ready to go. https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/browser_options.py:80: help='The device to run on' Should this be: 'The target device identifier. If not specified, only 0 or 1 connected devices of each type are supported.' https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/browser_options.py:89: '--remote', Will this go away in favor of --device or are we keeping the concepts separate? https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/webdriver_desktop_device.py (right): https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/webdriver_desktop_device.py:19: class WebDriverDevice(device.Device): It's not clear to me that this should be separate from desktop. After all, there's one desktop Device which may have multiple Browsers. A browser may be webdriver based or Chrome based.
nednguyen@google.com changed reviewers: + nednguyen@google.com
https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... File tools/telemetry/telemetry/core/platform/device.py (right): https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... tools/telemetry/telemetry/core/platform/device.py:34: def FindAllAvailableBrowsers(self, options): Why is this is a Device method? Device represents the identity of physical device, it's not the API for inspecting or controlling the it, which is what platform is for. I rather keep this API minimal.
On 2014/12/04 02:48:35, nednguyen wrote: > https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... > File tools/telemetry/telemetry/core/platform/device.py (right): > > https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... > tools/telemetry/telemetry/core/platform/device.py:34: def > FindAllAvailableBrowsers(self, options): > Why is this is a Device method? Device represents the identity of physical > device, it's not the API for inspecting or controlling the it, which is what > platform is for. I rather keep this API minimal. Each device may have a different set of browsers. For instance, a user may have 2 android devices, one with chrome-stable and another with chrome-beta.
On 2014/12/04 02:50:32, tonyg wrote: > On 2014/12/04 02:48:35, nednguyen wrote: > > > https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... > > File tools/telemetry/telemetry/core/platform/device.py (right): > > > > > https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... > > tools/telemetry/telemetry/core/platform/device.py:34: def > > FindAllAvailableBrowsers(self, options): > > Why is this is a Device method? Device represents the identity of physical > > device, it's not the API for inspecting or controlling the it, which is what > > platform is for. I rather keep this API minimal. > > Each device may have a different set of browsers. For instance, a user may have > 2 android devices, one with chrome-stable and another with chrome-beta. I see that, I just don't see why it's a device's method. The device class is for creating the platform only, and we should keep it that way (single responsibility principle). 2 places I can see that this fits into: 1) platform::FindAllAvailableBrowser(options) 2) browser_finder.FindAllAvailableBrowser(platform, options) I do prefer option 2 because with the goal of turning telemetry to a general app testing framework, it should treat browser less special.
On 2014/12/04 03:10:51, nednguyen wrote: > On 2014/12/04 02:50:32, tonyg wrote: > > On 2014/12/04 02:48:35, nednguyen wrote: > > > > > > https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... > > > File tools/telemetry/telemetry/core/platform/device.py (right): > > > > > > > > > https://codereview.chromium.org/760653002/diff/1/tools/telemetry/telemetry/co... > > > tools/telemetry/telemetry/core/platform/device.py:34: def > > > FindAllAvailableBrowsers(self, options): > > > Why is this is a Device method? Device represents the identity of physical > > > device, it's not the API for inspecting or controlling the it, which is what > > > platform is for. I rather keep this API minimal. > > > > Each device may have a different set of browsers. For instance, a user may > have > > 2 android devices, one with chrome-stable and another with chrome-beta. > > I see that, I just don't see why it's a device's method. The device class is for > creating the platform only, and we should keep it that way (single > responsibility principle). 2 places I can see that this fits into: > 1) platform::FindAllAvailableBrowser(options) > 2) browser_finder.FindAllAvailableBrowser(platform, options) > > I do prefer option 2 because with the goal of turning telemetry to a general app > testing framework, it should treat browser less special. Good points. Option 2 sgtm
> I see that, I just don't see why it's a device's method. The device class is
for
> creating the platform only, and we should keep it that way (single
> responsibility principle). 2 places I can see that this fits into:
> 1) platform::FindAllAvailableBrowser(options)
> 2) browser_finder.FindAllAvailableBrowser(platform, options)
>
> I do prefer option 2 because with the goal of turning telemetry to a general
app
> testing framework, it should treat browser less special.
I think FindAllAvailableBrowser should be part of Device's responsibility. Or
more precisely, each device has its own browser finder, which finds browsers. So
browser_finder.FindAllAvailableBrowser is a private API for Device.
Device.FindAllAvailableBrowser is an API for run_tests.py.
My mental model is:
1. Find all devices.
2. Each device calls its own browser finder to find all available browsers.
3. Launch tests according to the devices and browsers available.
So "possible_browser = browser_finder.FindBrowser(args)" in run_tests.py will
eventually become:
devices = device_finder.GetAllAvailableDevices(args)
device.FindAllAvailableBrowsers(args)
The possible browsers are kept in each device. So when we launch tests, we know
the relationship between the device and its browsers.
Device.FindAllAvailableBrowsers() is merely a way to call to the device's
browser finder's FindAllAvailableBrowsers() function. I can either
1. Use something like Device.GetBrowserFinder().FindAllAvailableBrowsers().
2. Or use Device.FindAllAvailableBrowsers() to call the browser finder function.
I personally prefer 2, because run_tests.py does not care if there is a browser
finder. It only cares about devices and browsers (or apps), but not how to find
them. Using 2, we can also keep the found browsers in the device, so we have the
relationship between the device and its browsers. Notice that currently
browser_finder is not a class. I can create the BrowserFinder class and let
Device own BrowserFinder if necessary.
And I think Platform is not a good place to find browsers. Think about the flags
"--device=list" and "--browser=list". When listing all devices and/or browsers,
we do not necessarily "create" Platform (I think?). So finding the availble
browser is not Platform's job.
For general apps, likewise, we can have Device.FindAllAvailableApps(). I am not
sure whether this makes turning telemetry to a general app testing framework
harder or easier. To me, devices and browsers/apps are the key objects the tests
care about. browser_finder sounds like a way to get those objects, which the
tests may not care about.
Let me know if I am wrong somewhere. I am getting familiar with the code. :-)
On 2014/12/04 16:33:06, Zhen Wang wrote: > > I see that, I just don't see why it's a device's method. The device class is > for > > creating the platform only, and we should keep it that way (single > > responsibility principle). 2 places I can see that this fits into: > > 1) platform::FindAllAvailableBrowser(options) > > 2) browser_finder.FindAllAvailableBrowser(platform, options) > > > > I do prefer option 2 because with the goal of turning telemetry to a general > app > > testing framework, it should treat browser less special. > > I think FindAllAvailableBrowser should be part of Device's responsibility. Or > more precisely, each device has its own browser finder, which finds browsers. So > browser_finder.FindAllAvailableBrowser is a private API for Device. > Device.FindAllAvailableBrowser is an API for run_tests.py. > > My mental model is: > 1. Find all devices. > 2. Each device calls its own browser finder to find all available browsers. This is the step that the should be replaced by 2 steps: + Create a platform instance based on the device + find all available browser on that platform. > 3. Launch tests according to the devices and browsers available. > > So "possible_browser = browser_finder.FindBrowser(args)" in run_tests.py will > eventually become: > devices = device_finder.GetAllAvailableDevices(args) > device.FindAllAvailableBrowsers(args) > > The possible browsers are kept in each device. So when we launch tests, we know > the relationship between the device and its browsers. > > Device.FindAllAvailableBrowsers() is merely a way to call to the device's > browser finder's FindAllAvailableBrowsers() function. I can either > 1. Use something like Device.GetBrowserFinder().FindAllAvailableBrowsers(). > 2. Or use Device.FindAllAvailableBrowsers() to call the browser finder function. > > I personally prefer 2, because run_tests.py does not care if there is a browser > finder. It only cares about devices and browsers (or apps), but not how to find > them. Using 2, we can also keep the found browsers in the device, so we have the > relationship between the device and its browsers. Notice that currently > browser_finder is not a class. I can create the BrowserFinder class and let > Device own BrowserFinder if necessary. > > And I think Platform is not a good place to find browsers. Think about the flags > "--device=list" and "--browser=list". When listing all devices and/or browsers, > we do not necessarily "create" Platform (I think?). So finding the availble > browser is not Platform's job. You list all the devices by essentially using the host platform, which has USB connections to a bunch of physical device. To figure out what on each device, you make "connection" to them, which is what platform is for. Theoretically, you can ignore all these abstractions and just use python's sys + os library to implement these functionalities, but that's not the way to do thing in telemetry. We have the platform abstraction to begin with to help prevent people from shooting themselves with these system operations. > > For general apps, likewise, we can have Device.FindAllAvailableApps(). I am not > sure whether this makes turning telemetry to a general app testing framework > harder or easier. To me, devices and browsers/apps are the key objects the tests > care about. browser_finder sounds like a way to get those objects, which the > tests may not care about. The reasons we have browser_finder to begin with is telemetry support selecting the browser instances through command line flags. This model is not robust enough for general app testing scenario: i.e: how would you list a general application named X across 10 different platforms. And how to specify apps through command line flags if you have a user story test that involves multiple applications? I strongly believe we won't carry over the ability to specify which application to test through command line to general app testing model. While it's not feasible to deprecate the browser selection mechanism through command line flags at the moment, please make browser finder a special case rather than trying to extend the core platform / device API for this case. > > Let me know if I am wrong somewhere. I am getting familiar with the code. :-)
On 2014/12/04 17:10:42, nednguyen wrote: > On 2014/12/04 16:33:06, Zhen Wang wrote: > > > I see that, I just don't see why it's a device's method. The device class is > > for > > > creating the platform only, and we should keep it that way (single > > > responsibility principle). 2 places I can see that this fits into: > > > 1) platform::FindAllAvailableBrowser(options) > > > 2) browser_finder.FindAllAvailableBrowser(platform, options) > > > > > > I do prefer option 2 because with the goal of turning telemetry to a general > > app > > > testing framework, it should treat browser less special. > > > > I think FindAllAvailableBrowser should be part of Device's responsibility. Or > > more precisely, each device has its own browser finder, which finds browsers. > So > > browser_finder.FindAllAvailableBrowser is a private API for Device. > > Device.FindAllAvailableBrowser is an API for run_tests.py. > > > > My mental model is: > > 1. Find all devices. > > 2. Each device calls its own browser finder to find all available browsers. > > This is the step that the should be replaced by 2 steps: + Create a platform > instance based on the device + find all available browser on that platform. How about desktop browsers? Platform is created by PossibleBrowser.Create() (after possible browsers are found). Platform is not the generally necessity to find browsers/apps. Well, there was no DesktopDevice before. But I think Device is a simple and general way to hold all its possible browsers. And as you mentioned below, Platform is the abstraction for sys/os implementation details. > > > 3. Launch tests according to the devices and browsers available. > > > > So "possible_browser = browser_finder.FindBrowser(args)" in run_tests.py will > > eventually become: > > devices = device_finder.GetAllAvailableDevices(args) > > device.FindAllAvailableBrowsers(args) > > > > The possible browsers are kept in each device. So when we launch tests, we > know > > the relationship between the device and its browsers. > > > > Device.FindAllAvailableBrowsers() is merely a way to call to the device's > > browser finder's FindAllAvailableBrowsers() function. I can either > > 1. Use something like Device.GetBrowserFinder().FindAllAvailableBrowsers(). > > 2. Or use Device.FindAllAvailableBrowsers() to call the browser finder > function. > > > > I personally prefer 2, because run_tests.py does not care if there is a > browser > > finder. It only cares about devices and browsers (or apps), but not how to > find > > them. Using 2, we can also keep the found browsers in the device, so we have > the > > relationship between the device and its browsers. Notice that currently > > browser_finder is not a class. I can create the BrowserFinder class and let > > Device own BrowserFinder if necessary. > > > > And I think Platform is not a good place to find browsers. Think about the > flags > > "--device=list" and "--browser=list". When listing all devices and/or > browsers, > > we do not necessarily "create" Platform (I think?). So finding the availble > > browser is not Platform's job. > > You list all the devices by essentially using the host platform, which has USB > connections to a bunch of physical device. To figure out what on each device, > you make "connection" to them, which is what platform is for. > > Theoretically, you can ignore all these abstractions and just use python's sys + > os library to implement these functionalities, but that's not the way to do > thing in telemetry. We have the platform abstraction to begin with to help > prevent people from shooting themselves with these system operations. > > > > > For general apps, likewise, we can have Device.FindAllAvailableApps(). I am > not > > sure whether this makes turning telemetry to a general app testing framework > > harder or easier. To me, devices and browsers/apps are the key objects the > tests > > care about. browser_finder sounds like a way to get those objects, which the > > tests may not care about. > > The reasons we have browser_finder to begin with is telemetry support selecting > the browser instances through command line flags. This model is not robust > enough for general app testing scenario: i.e: how would you list a general > application named X across 10 different platforms. And how to specify apps > through command line flags if you have a user story test that involves multiple > applications? For one general application X, you can use: --device=Android-abcdefg --app=X I haven't thought about multi-app testing. > > I strongly believe we won't carry over the ability to specify which application > to test through command line to general app testing model. While it's not > feasible to deprecate the browser selection mechanism through command line flags > at the moment, please make browser finder a special case rather than trying to > extend the core platform / device API for this case. I don't think the API is extended. It is shifted from browser_finder.FindAllAvailableBrowsers() to Device.FindAllAvailableBrowsers(). And browser_finder becomes private to each device. > > > > > Let me know if I am wrong somewhere. I am getting familiar with the code. :-)
On 2014/12/04 17:45:11, Zhen Wang wrote: > On 2014/12/04 17:10:42, nednguyen wrote: > > On 2014/12/04 16:33:06, Zhen Wang wrote: > > > > I see that, I just don't see why it's a device's method. The device class > is > > > for > > > > creating the platform only, and we should keep it that way (single > > > > responsibility principle). 2 places I can see that this fits into: > > > > 1) platform::FindAllAvailableBrowser(options) > > > > 2) browser_finder.FindAllAvailableBrowser(platform, options) > > > > > > > > I do prefer option 2 because with the goal of turning telemetry to a > general > > > app > > > > testing framework, it should treat browser less special. > > > > > > I think FindAllAvailableBrowser should be part of Device's responsibility. > Or > > > more precisely, each device has its own browser finder, which finds > browsers. > > So > > > browser_finder.FindAllAvailableBrowser is a private API for Device. > > > Device.FindAllAvailableBrowser is an API for run_tests.py. > > > > > > My mental model is: > > > 1. Find all devices. > > > 2. Each device calls its own browser finder to find all available browsers. > > > > This is the step that the should be replaced by 2 steps: + Create a platform > > instance based on the device + find all available browser on that platform. > > How about desktop browsers? Platform is created by PossibleBrowser.Create() > (after possible browsers are found). Platform is not the generally necessity to > find browsers/apps. Well, there was no DesktopDevice before. But I think Device > is a simple and general way to hold all its possible browsers. And as you > mentioned below, Platform is the abstraction for sys/os implementation details. > > > > > > 3. Launch tests according to the devices and browsers available. > > > > > > So "possible_browser = browser_finder.FindBrowser(args)" in run_tests.py > will > > > eventually become: > > > devices = device_finder.GetAllAvailableDevices(args) > > > device.FindAllAvailableBrowsers(args) > > > > > > The possible browsers are kept in each device. So when we launch tests, we > > know > > > the relationship between the device and its browsers. > > > > > > Device.FindAllAvailableBrowsers() is merely a way to call to the device's > > > browser finder's FindAllAvailableBrowsers() function. I can either > > > 1. Use something like Device.GetBrowserFinder().FindAllAvailableBrowsers(). > > > 2. Or use Device.FindAllAvailableBrowsers() to call the browser finder > > function. > > > > > > I personally prefer 2, because run_tests.py does not care if there is a > > browser > > > finder. It only cares about devices and browsers (or apps), but not how to > > find > > > them. Using 2, we can also keep the found browsers in the device, so we have > > the > > > relationship between the device and its browsers. Notice that currently > > > browser_finder is not a class. I can create the BrowserFinder class and let > > > Device own BrowserFinder if necessary. > > > > > > And I think Platform is not a good place to find browsers. Think about the > > flags > > > "--device=list" and "--browser=list". When listing all devices and/or > > browsers, > > > we do not necessarily "create" Platform (I think?). So finding the availble > > > browser is not Platform's job. > > > > You list all the devices by essentially using the host platform, which has USB > > connections to a bunch of physical device. To figure out what on each device, > > you make "connection" to them, which is what platform is for. > > > > Theoretically, you can ignore all these abstractions and just use python's sys > + > > os library to implement these functionalities, but that's not the way to do > > thing in telemetry. We have the platform abstraction to begin with to help > > prevent people from shooting themselves with these system operations. > > > > > > > > For general apps, likewise, we can have Device.FindAllAvailableApps(). I am > > not > > > sure whether this makes turning telemetry to a general app testing framework > > > harder or easier. To me, devices and browsers/apps are the key objects the > > tests > > > care about. browser_finder sounds like a way to get those objects, which the > > > tests may not care about. > > > > The reasons we have browser_finder to begin with is telemetry support > selecting > > the browser instances through command line flags. This model is not robust > > enough for general app testing scenario: i.e: how would you list a general > > application named X across 10 different platforms. And how to specify apps > > through command line flags if you have a user story test that involves > multiple > > applications? > > For one general application X, you can use: > > --device=Android-abcdefg --app=X No, this doesn't work with user story because you can write a user story for email app, but the command line flag passed in can specify s.t like --app=calendar. It doesn't make sense to write the app's interactions in one place and selecting the app in another place. The browser_finder's model already does not fit with the model, i.e: --browser=android-chrome-beta but the test has nothing to do with browser app, but I would leave it a TODO to clean up later. > > I haven't thought about multi-app testing. > > > > > I strongly believe we won't carry over the ability to specify which > application > > to test through command line to general app testing model. While it's not > > feasible to deprecate the browser selection mechanism through command line > flags > > at the moment, please make browser finder a special case rather than trying to > > extend the core platform / device API for this case. > > I don't think the API is extended. It is shifted from > browser_finder.FindAllAvailableBrowsers() to Device.FindAllAvailableBrowsers(). > And browser_finder becomes private to each device. By API is extended, I meant the API of device in this case. browser_finder's logic doesn't fit into the new telemetry architecture, and please don't try to blend it with the well established APIs. > > > > > > > > > Let me know if I am wrong somewhere. I am getting familiar with the code. > :-)
> > I don't think the API is extended. It is shifted from > > browser_finder.FindAllAvailableBrowsers() to > Device.FindAllAvailableBrowsers(). > > And browser_finder becomes private to each device. > > By API is extended, I meant the API of device in this case. browser_finder's > logic doesn't fit into the new telemetry architecture, and please don't try to > blend it with the well established APIs. I agree that we should keep the browser case separate from the new general app testing framework. But I guess using Device.FindAllAvailableBrowsers() does not affect the new framework. Or will it? What are the other places using or will use Device? Using browser_finder to deal with Device and Browser hierarchy feels unnatural ...
On 2014/12/04 18:19:29, Zhen Wang wrote: > > > I don't think the API is extended. It is shifted from > > > browser_finder.FindAllAvailableBrowsers() to > > Device.FindAllAvailableBrowsers(). > > > And browser_finder becomes private to each device. > > > > By API is extended, I meant the API of device in this case. browser_finder's > > logic doesn't fit into the new telemetry architecture, and please don't try to > > blend it with the well established APIs. > > I agree that we should keep the browser case separate from the new general app > testing framework. But I guess using Device.FindAllAvailableBrowsers() does not > affect the new framework. Or will it? What are the other places using or will > use Device? It will be used everywhere. Any app testing story can use Device to construct the platform instance. The order of things in an app testing will be: device = AndroidDevice(...) platform = platform_module.GetPlatformForDevice(device). app = platform.LaunchApp(...) > Using browser_finder to deal with Device and Browser hierarchy feels > unnatural ... Device finding logic shouldn't live in browser_finder. browser_finder can be modified to take in device instance and return the possible browser. I am fine with either: browser_finder.FindAllAvailableBrowsers(device) or browser_finder.FindAllAvailableBrowsers(platform)
I have re-implemented the code (patch 2 to 4). The new version minimizes the changes to device files so that browser is kept as a special case from other apps. - device_finder.py finds all available devices. - field device_id is added to possible_app to indicate which device the app will run on. This is used to show a hierarchical view of the browsers. It is also used to filter the possible browsers by comparing with --device value when selecting the possible browser. - no tests are added yet. would like to get some feedback first. ptal
Relying on browser_finders_* and add the 'device_id' field to possible_browser/possible_app find devices seems like a hack to reuse the legacy device finding logic baked in browser_finder. I would rather us doing the right thing here, which is device_finder uses AndroidDevice, IosDevice, WebDriverDevice... to list all the possible devices. If command line options can pin-point to a unique device, then that device is used to create a platform. BrowserFinder then take in the platform & options to create the possible_browser instance. This flow is much saner and easier to reason. https://codereview.chromium.org/760653002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/browser_finder.py (right): https://codereview.chromium.org/760653002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_finder.py:73: if browser.device_id == options.device] This should be *possible_browser* to avoid confusion. https://codereview.chromium.org/760653002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:166: browsers = browser_finder.GetAllAvailableBrowsers(self) Ditto. https://codereview.chromium.org/760653002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/device_finder.py (right): https://codereview.chromium.org/760653002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/device_finder.py:31: device_names.append(browsers[0].device_id) I don't like this hack. https://codereview.chromium.org/760653002/diff/60001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/platform/cros_device.py (right): https://codereview.chromium.org/760653002/diff/60001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/platform/cros_device.py:26: return self._ssh_identity Strange, what is this diff about?
On 2015/01/07 18:18:10, nednguyen wrote: > Relying on browser_finders_* and add the 'device_id' field to > possible_browser/possible_app find devices seems like a hack to reuse the legacy > device finding logic baked in browser_finder. I would rather us doing the right > thing here, which is device_finder uses AndroidDevice, IosDevice, > WebDriverDevice... to list all the possible devices. > > If command line options can pin-point to a unique device, then that device is > used to create a platform. > > BrowserFinder then take in the platform & options to create the possible_browser > instance. > > This flow is much saner and easier to reason. Will adding those device files affect the work on general app? The original approach added those device files. But it seems to have some conflict with general app work. Or do you mean: we should add those device files, but just do not use Device.FindAllAvailableBrowsers; just pass device to browser finder somehow? Currently, browser finder is the central logic. We loop through all different types of browser finders to find possible browsers. How about adding some more logic in run_test to find devices first, then pass the discovered devices to browser finders? By the way, why adding device_id to possible app is a hack? There is already target_os in it. device_id is similar to target_os. It is just an information each possible_app can have.
On 2015/01/07 18:28:47, Zhen Wang wrote: > On 2015/01/07 18:18:10, nednguyen wrote: > > Relying on browser_finders_* and add the 'device_id' field to > > possible_browser/possible_app find devices seems like a hack to reuse the > legacy > > device finding logic baked in browser_finder. I would rather us doing the > right > > thing here, which is device_finder uses AndroidDevice, IosDevice, > > WebDriverDevice... to list all the possible devices. > > > > If command line options can pin-point to a unique device, then that device is > > used to create a platform. > > > > BrowserFinder then take in the platform & options to create the > possible_browser > > instance. > > > > This flow is much saner and easier to reason. > > Will adding those device files affect the work on general app? The original > approach added those device files. But it seems to have some conflict with > general app work. I don't see any conflict. The approach you took with AndroidDevice looks good to me, and that's the only point of potential conflict since most of the app work will focus on android plaform/device for now. > > Or do you mean: we should add those device files, but just do not use > Device.FindAllAvailableBrowsers; just pass device to browser finder somehow? > Currently, browser finder is the central logic. We loop through all different > types of browser finders to find possible browsers. How about adding some more > logic in run_test to find devices first, then pass the discovered devices to > browser finders? Yes, this is the flow that I am looking for. > > By the way, why adding device_id to possible app is a hack? There is already > target_os in it. device_id is similar to target_os. It is just an information > each possible_app can have. It's an information for possible_app can have if possible_app is meant to be used by DeviceFinder. But possible_app isn't meant to be used by DeviceFinder in such way. PossibleApp is the factory for for app, and that's pretty much it. Monolithic classes/components that can be used everywhere by everyone is a common downfall of OOP that I would try us to avoid here.
The 3rd version. ptal.
Why do the possible_browser* classes have the device_name & device_id? https://codereview.chromium.org/760653002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py (right): https://codereview.chromium.org/760653002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:40: return self._device.name Hmhh, why do we need this?
https://codereview.chromium.org/760653002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:175: if device.guid == browser.device_id] Nah, don't do it this way. Instead: for device in devices: possible_browsers = browser_finder.GetAllAvailableBrowsers(device) types = sort([possible_browsers.browser_type]) ...
https://codereview.chromium.org/760653002/diff/80001/tools/telemetry/telemetr... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/80001/tools/telemetry/telemetr... tools/telemetry/telemetry/core/browser_options.py:175: if device.guid == browser.device_id] On 2015/01/08 22:21:27, nednguyen wrote: > Nah, don't do it this way. Instead: > > for device in devices: > possible_browsers = browser_finder.GetAllAvailableBrowsers(device) > types = sort([possible_browsers.browser_type]) > ... Agreed, GetAllAvailableBrowsers should find the correct browser finder(s) given the device, and we should pass the correct platform into each BrowserFinder. The BrowserFinders shouldn't search the device list. I also kind of think the individual BrowserFinders should take a platform instead of a device.
On 2015/01/08 22:27:36, dtu wrote: > https://codereview.chromium.org/760653002/diff/80001/tools/telemetry/telemetr... > File tools/telemetry/telemetry/core/browser_options.py (right): > > https://codereview.chromium.org/760653002/diff/80001/tools/telemetry/telemetr... > tools/telemetry/telemetry/core/browser_options.py:175: if device.guid == > browser.device_id] > On 2015/01/08 22:21:27, nednguyen wrote: > > Nah, don't do it this way. Instead: > > > > for device in devices: > > possible_browsers = browser_finder.GetAllAvailableBrowsers(device) > > types = sort([possible_browsers.browser_type]) > > ... > > Agreed, GetAllAvailableBrowsers should find the correct browser finder(s) given > the device, and we should pass the correct platform into each BrowserFinder. The > BrowserFinders shouldn't search the device list. I also kind of think the > individual BrowserFinders should take a platform instead of a device. Yeah good point. Even greater if BrowserFinders takes a platform instead of a device.
> > Agreed, GetAllAvailableBrowsers should find the correct browser finder(s) > given > > the device, and we should pass the correct platform into each BrowserFinder. > The > > BrowserFinders shouldn't search the device list. I also kind of think the > > individual BrowserFinders should take a platform instead of a device. > > Yeah good point. Even greater if BrowserFinders takes a platform instead of a > device. Different browser finders have different ways to get the platform. But now we have a unified way to get the device (device_finder). Why not let each individual browser finder get its own platform based on its device?
On 2015/01/08 23:03:48, Zhen Wang wrote: > > > Agreed, GetAllAvailableBrowsers should find the correct browser finder(s) > > given > > > the device, and we should pass the correct platform into each BrowserFinder. > > The > > > BrowserFinders shouldn't search the device list. I also kind of think the > > > individual BrowserFinders should take a platform instead of a device. > > > > Yeah good point. Even greater if BrowserFinders takes a platform instead of a > > device. > > Different browser finders have different ways to get the platform. But now we > have a unified way to get the device (device_finder). Why not let each > individual browser finder get its own platform based on its device? Browser finders are divided by platform, so I don't think there are multiple ways of getting for browser finder to get its own platform. I can see the amount of code change you need to make that happens is not small, so I am fine with browser_finder.GetAllAvailableBrowsers(device) in this patch.
On 2015/01/08 23:03:48, Zhen Wang wrote: > > > Agreed, GetAllAvailableBrowsers should find the correct browser finder(s) > > given > > > the device, and we should pass the correct platform into each BrowserFinder. > > The > > > BrowserFinders shouldn't search the device list. I also kind of think the > > > individual BrowserFinders should take a platform instead of a device. > > > > Yeah good point. Even greater if BrowserFinders takes a platform instead of a > > device. > > Different browser finders have different ways to get the platform. But now we > have a unified way to get the device (device_finder). Why not let each > individual browser finder get its own platform based on its device? Isn't there a unified way to get the platform from the device? platform.GetPlatformForDevice()? (Actually I think I'd prefer this as device.GetPlatform().) Then you can pass the Platform to the BrowserFinder from within GetAllAvailableBrowsers().
Some browser finders get the platform by platform.GetPlatformForDevice(device), and then the platform will be passed to its possible_browser: android, cros Other browser finders get the platform by platform.GetHostPlatform(). This is done within possible_browser by _InitPlatformIfNeeded(): desktop, ios, trybot, webdriver.
On 2015/01/08 23:57:09, Zhen Wang wrote: > Some browser finders get the platform by platform.GetPlatformForDevice(device), > and then the platform will be passed to its possible_browser: android, cros > > Other browser finders get the platform by platform.GetHostPlatform(). This is > done within possible_browser by _InitPlatformIfNeeded(): desktop, ios, trybot, > webdriver. To me, that just means those browser finders can not take any platform other than host platform of the same type. They should just return empty list if the platform is unsupported.
ptal. This version still passes device to browser_finder instead of platform: browser_finder.GetAllAvailableBrowsers(device)
This looks quite solid to me. https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py (right): https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:19: def Type(): This does not add any value, remove this. https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:124: return [] I don't like this kind of name matching logic. For more sane, please do: if not isinstance(device, destkop_device.DesktopDevice): return [] https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_finder.py (right): https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_finder.py:30: def GetBrowserFinderForDevice(device): I don't think you need this method. If device is not supported by the browser_finder, browser_finder.FindAllAvailableBrowsers(options, device) gonna just return an empty list, right? all_possible_browsers = [] for browser_finder in BROWSER_FINDERS: all_possible_browsers += browser_finder.FindAllAvailableBrowsers(options, device) https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/desktop_device.py (right): https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/desktop_device.py:21: if platform.GetHostPlatform().GetOSName() == 'chromeos': Why?
On 2015/01/23 04:31:15, nednguyen wrote: > This looks quite solid to me. > > https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py > (right): > > https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:19: def > Type(): > This does not add any value, remove this. > > https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:124: > return [] > I don't like this kind of name matching logic. For more sane, please do: > if not isinstance(device, destkop_device.DesktopDevice): > return [] > > https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/browser_finder.py (right): > > https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/browser_finder.py:30: def > GetBrowserFinderForDevice(device): > I don't think you need this method. If device is not supported by the > browser_finder, browser_finder.FindAllAvailableBrowsers(options, device) gonna > just return an empty list, right? > > all_possible_browsers = [] > for browser_finder in BROWSER_FINDERS: > all_possible_browsers += browser_finder.FindAllAvailableBrowsers(options, > device) > > https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... > File tools/telemetry/telemetry/core/platform/desktop_device.py (right): > > https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... > tools/telemetry/telemetry/core/platform/desktop_device.py:21: if > platform.GetHostPlatform().GetOSName() == 'chromeos': > Why? Btw, does this change existing command line flag of telemetry for selecting browser? If so, what's your plan to maintain backward compatibility while we are migrating bots' setup?
This CL adds "--device=list" and use a hierarchical view for "--browser=list". It does not change current browser selection. The only caveat is that when more than 1 Android device is plugged in and if "--device" is not specified, it will only warn you and choose one to run the tests. The original behavior will report error with more than 1 Android device and stop. And I think all commands used on the bots have "--device=serial_number", right? Moving forward, in a separate CL, I plan to introduce something like "--device=android" to utilize all connected Android devices. And that option also maintains backward compatibility. I will add tests to cover the changes in this CL after there are no major design issues. https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py (right): https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:19: def Type(): On 2015/01/23 04:31:15, nednguyen wrote: > This does not add any value, remove this. Done. https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/desktop_browser_finder.py:124: return [] On 2015/01/23 04:31:15, nednguyen wrote: > I don't like this kind of name matching logic. For more sane, please do: > if not isinstance(device, destkop_device.DesktopDevice): > return [] Done. https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_finder.py (right): https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_finder.py:30: def GetBrowserFinderForDevice(device): On 2015/01/23 04:31:15, nednguyen wrote: > I don't think you need this method. If device is not supported by the > browser_finder, browser_finder.FindAllAvailableBrowsers(options, device) gonna > just return an empty list, right? > > all_possible_browsers = [] > for browser_finder in BROWSER_FINDERS: > all_possible_browsers += browser_finder.FindAllAvailableBrowsers(options, > device) > Done. https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/desktop_device.py (right): https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/desktop_device.py:21: if platform.GetHostPlatform().GetOSName() == 'chromeos': On 2015/01/23 04:31:15, nednguyen wrote: > Why? If the host platform is Chrome OS, the device is considered as cros instead of desktop.
https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/desktop_device.py (right): https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/desktop_device.py:21: if platform.GetHostPlatform().GetOSName() == 'chromeos': On 2015/01/23 18:57:10, Zhen Wang wrote: > On 2015/01/23 04:31:15, nednguyen wrote: > > Why? > > If the host platform is Chrome OS, the device is considered as cros instead of > desktop. Ah, I see. Cros such a weird case. Can you add documentation explain this? https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py (right): https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:19: def Type(): Remove this? https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:93: if cros_device.IsRunningOnCrOS(): This is why browser_finder_* should deal with platform directly rather than with device. All the logic for constructing platform from device is already in platform/__init__.py and we are duplicating it again here. Anyway, I am fine with going ahead with this and clean up this later. https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:78: if not ios_device.IsIosDeviceAttached(): You should not need to test this. If ios device is not attached, it shouldn't be constructed. https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/cros_device.py (right): https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/cros_device.py:56: return [CrOSDevice(options.cros_remote, options.cros_ssh_identity)] CrosDevice's constructor takes host_name, ssh_port, ssh_identity=None. I think this call does not match those params. https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/ios_device.py (right): https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/ios_device.py:23: def IsIosDeviceAttached(): Please remove the cache and make this method private: _IsIosDeviceAttached()
https://codereview.chromium.org/760653002/diff/140001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/device_finder.py (right): https://codereview.chromium.org/760653002/diff/140001/tools/telemetry/telemet... tools/telemetry/telemetry/core/device_finder.py:33: def GetAllAvailableDeviceNames(options): This method should not need to take the options.
tonyg@chromium.org changed reviewers: - tonyg@chromium.org
https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/desktop_device.py (right): https://codereview.chromium.org/760653002/diff/100001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/desktop_device.py:21: if platform.GetHostPlatform().GetOSName() == 'chromeos': On 2015/01/23 19:30:36, nednguyen wrote: > On 2015/01/23 18:57:10, Zhen Wang wrote: > > On 2015/01/23 04:31:15, nednguyen wrote: > > > Why? > > > > If the host platform is Chrome OS, the device is considered as cros instead of > > desktop. > > Ah, I see. Cros such a weird case. Can you add documentation explain this? Done. https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py (right): https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/cros_browser_finder.py:19: def Type(): On 2015/01/23 19:30:36, nednguyen wrote: > Remove this? Done. https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py (right): https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/backends/chrome/ios_browser_finder.py:78: if not ios_device.IsIosDeviceAttached(): On 2015/01/23 19:30:36, nednguyen wrote: > You should not need to test this. If ios device is not attached, it shouldn't be > constructed. Done. https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/cros_device.py (right): https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/cros_device.py:56: return [CrOSDevice(options.cros_remote, options.cros_ssh_identity)] On 2015/01/23 19:30:36, nednguyen wrote: > CrosDevice's constructor takes host_name, ssh_port, ssh_identity=None. I think > this call does not match those params. Fixed https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/platform/ios_device.py (right): https://codereview.chromium.org/760653002/diff/120001/tools/telemetry/telemet... tools/telemetry/telemetry/core/platform/ios_device.py:23: def IsIosDeviceAttached(): On 2015/01/23 19:30:36, nednguyen wrote: > Please remove the cache and make this method private: _IsIosDeviceAttached() Done. https://codereview.chromium.org/760653002/diff/140001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/device_finder.py (right): https://codereview.chromium.org/760653002/diff/140001/tools/telemetry/telemet... tools/telemetry/telemetry/core/device_finder.py:33: def GetAllAvailableDeviceNames(options): On 2015/01/23 19:32:16, nednguyen wrote: > This method should not need to take the options. This is needed for cros_device, which needs --remote, --remote-ssh-port and --identity.
lgtm Can you check with Achuith to make sure that your change doesn't break cros?
https://codereview.chromium.org/760653002/diff/200001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_finder.py (right): https://codereview.chromium.org/760653002/diff/200001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_finder.py:136: def GetAllAvailableBrowsers(options, device): Who call this? https://codereview.chromium.org/760653002/diff/200001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_finder.py:154: return browsers Also this method is returning 'possible_browsers', not 'browsers'
Hi Achuith, can you take a look at the Chrome OS cases? https://codereview.chromium.org/760653002/diff/200001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_finder.py (right): https://codereview.chromium.org/760653002/diff/200001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_finder.py:136: def GetAllAvailableBrowsers(options, device): On 2015/01/26 22:14:08, nednguyen wrote: > Who call this? GetAllAvailableBrowserTypes() below and in browser_options.py for the case "--browser=list" https://codereview.chromium.org/760653002/diff/200001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_finder.py:154: return browsers On 2015/01/26 22:14:08, nednguyen wrote: > Also this method is returning 'possible_browsers', not 'browsers' fixed
I'd actually like to try this CL out on cros, please give me a few hours to get to it
OK. Thanks! Take your time. I will also need to try it on Mac this evening ... Best -Zhen On Mon, Jan 26, 2015 at 2:53 PM, <achuith@chromium.org> wrote: > I'd actually like to try this CL out on cros, please give me a few hours > to get > to it > > https://codereview.chromium.org/760653002/ > To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:165: sys.stdout.write('Available devices:\n') nit: print 'Available devices:' https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:166: sys.stdout.write(' %s\n' % '\n '.join(devices)) for device in devices: print ' ', device https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:186: except browser_finder_exceptions.BrowserFinderException, ex: style nit: use "as" instead of comma https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:187: sys.stderr.write('ERROR: ' + str(ex)) print >> sys.stderr, 'ERROR:', ex https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/device_finder.py (right): https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/device_finder.py:43: return None Use an assertion or throw a ValueError in this case. https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/device_finder.py:48: 'Device %s is not found.' % options.device) The caller should take care of the logging. You have two users of this function that require different messages. https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/device_finder.py:53: return devices[0] I think you should return the entire list (or empty list if not found) - you're going to need it to run unit tests with multiple devices at once.
https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:165: sys.stdout.write('Available devices:\n') On 2015/01/26 23:44:21, dtu wrote: > nit: print 'Available devices:' Done. Any reason to prefer print over sys.stdout? I see the original code is using stdout. https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:166: sys.stdout.write(' %s\n' % '\n '.join(devices)) On 2015/01/26 23:44:21, dtu wrote: > for device in devices: > print ' ', device Done. https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:186: except browser_finder_exceptions.BrowserFinderException, ex: On 2015/01/26 23:44:21, dtu wrote: > style nit: use "as" instead of comma Done. https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:187: sys.stderr.write('ERROR: ' + str(ex)) On 2015/01/26 23:44:21, dtu wrote: > print >> sys.stderr, 'ERROR:', ex Done. https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/device_finder.py (right): https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/device_finder.py:43: return None On 2015/01/26 23:44:22, dtu wrote: > Use an assertion or throw a ValueError in this case. Done. https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/device_finder.py:48: 'Device %s is not found.' % options.device) On 2015/01/26 23:44:21, dtu wrote: > The caller should take care of the logging. You have two users of this function > that require different messages. Given that we are returning a list, the logging info is no longer needed. https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/device_finder.py:53: return devices[0] On 2015/01/26 23:44:21, dtu wrote: > I think you should return the entire list (or empty list if not found) - you're > going to need it to run unit tests with multiple devices at once. Return a list now.
lgtm with nits https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/220001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:165: sys.stdout.write('Available devices:\n') On 2015/01/27 04:37:32, Zhen Wang wrote: > On 2015/01/26 23:44:21, dtu wrote: > > nit: print 'Available devices:' > > Done. > > Any reason to prefer print over sys.stdout? I see the original code is using > stdout. I think in this case in particular, using print leads to less complex and easier-to-read code. I would use sys.stdout.write if you're doing more complex string or newline manipulations. https://codereview.chromium.org/760653002/diff/240001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/240001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:190: sys.stdout.write('Available browsers:\n') nit: change these to print too https://codereview.chromium.org/760653002/diff/240001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:191: for device_name in sorted(browser_types.keys()): Print something like "No devices were found" if no devices. Same with browsers.
https://codereview.chromium.org/760653002/diff/240001/tools/telemetry/telemet... File tools/telemetry/telemetry/core/browser_options.py (right): https://codereview.chromium.org/760653002/diff/240001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:190: sys.stdout.write('Available browsers:\n') On 2015/01/27 23:37:14, dtu wrote: > nit: change these to print too Done. https://codereview.chromium.org/760653002/diff/240001/tools/telemetry/telemet... tools/telemetry/telemetry/core/browser_options.py:191: for device_name in sorted(browser_types.keys()): On 2015/01/27 23:37:14, dtu wrote: > Print something like "No devices were found" if no devices. Same with browsers. Done.
Hi Achuith, can you check on Chrome OS? Thanks!
On 2015/01/31 00:11:00, Zhen Wang wrote: > Hi Achuith, can you check on Chrome OS? Thanks! ping
nednguyen@google.com changed reviewers: + lafeenstra@google.com
On 2015/02/03 16:02:45, nednguyen wrote: +Laurens: Zhen's effort to add CQ bot for android devices is blocked on this patch. Can you please look into this patch as soon as possible?
On 2015/02/03 16:03:46, nednguyen wrote: > On 2015/02/03 16:02:45, nednguyen wrote: > > +Laurens: Zhen's effort to add CQ bot for android devices is blocked on this > patch. Can you please look into this patch as soon as possible? Sorry, I just forgot about this. Give me a few hours.
lgtm. This works on cros. Thanks for your patience
New patchsets have been uploaded after l-g-t-m from achuith@chromium.org
The CQ bit was checked by zhenw@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/760653002/270001
Message was sent while issue was closed.
Committed patchset #15 (id:270001)
Message was sent while issue was closed.
Patchset 15 (id:??) landed as https://crrev.com/d5f6ea98506d437a71afca8ea937c3fc380d1532 Cr-Commit-Position: refs/heads/master@{#314420} |
