Hmhh, my review bandwidth is a bit overloaded at the moment.

How is this related to sadrul@'s work to add chromeOS bot to FYI waterfall? Also is Jon Ross's work to run Telemetry test suites (telemetry_unittests & telemetry_perf_unittests) on CQ related?

Can we sequence all the work in this area so I can take time to go through them in order?

sadrul@, jonross@, feel free to comment if I'm wrong.

- sadrul@ is adding a Linux bot. It will run ChromeOS on Linux, eventually. That's the bot I'm talking about in comment #1.

- jonross@ is focused on correctness. AFAIK he want's to be able to disable/enable unit tests on a ChromeOS on Linux configuration without affecting tests that run on normal Linux desktop configs ( crbug.com/758065 ). But, right now both configurations use same backends, i.e. desktop browser backend on linux platform backend. So, maybe this proposal can unblock him too.

Adding a new CroSLinuxBrowserBackend sounds like the right direction to me. Though I think there are some questions we need to answers:

1) How do we disable the test on this browser? Should it be @decorators.Disabled('chromeos') or @decorators.Disabled('linux-chromeos')?
2) How does Telemetry know to pick this backend in the following cases:
a) The commandline flag is --browser=release
b) The commandline flag is --browser=exact --browser-executable=<path to a binary that happens to be cros browser binary>
3) How do we add CQ testing for this browser for catapult repo?

Also worths noting that perezju@ is doing a bunch of work on refactoring browser_backend, so you may need to sync with him on sequencing

1) I think, for jonross@ use case, we want to disable tests for the new browser backend. So, if the browser type of the new backend is 'CrosLinuxBrowser', we want @decorators.Disabled('CrosLinuxBrowser') or maybe @decorators.Disabled('CrosLinuxBrowser', 'chromeos') if we want to disable them on actual ChromeOS bots, too, if they exist.

2) I was thinking to config the bot to use the following flag combination (which is not allowed right now):

--browser=cros-chrome --browser-executable=<path to the binary>

We can also add a new browser type --browser=cros-linux-chrome if reusing --browser=cros-chrome will be confusing.

I think we shouldn't change --browser=relase.

3) I'm not sure yet, but isn't it possible to write a unit test for the cros_linux_brower_finder.py by passing options (flags) mentioned in 2 to it?

perezju@, is there a doc about browser backend refactorings you are working on?

2) If we don't change  --browser=release, how would Telemetry knows to pick CrOSLinuxBrowserBackend or LinuxBorwserBackend on a linux bot?

To me the ideal design would be letting Telemetry figure out the type of the browser based on the Chrome binary itself. Using commandline flag to hint Telemetry's browser type would be fragile, e.g: we can run into the case of accidentally use Chrome Linux binary on Linux ChromeOS builder.

Is it possible to add s.t like a commandline flag to Chrome binary  that let Telemetry knows the browser's type? e.g: executing "./chrome --info" would print "Chrome Linux" or "ChromeOS Linux".

Another design would be to reuse  desktop browser back-end & figure out whether the browser is a CrOS one after CHrome is already launched (maybe through issuing a devtool command asking for browser info)

Re #9: I don't have a doc about it at the moment. The main changes I'm working on now are described in:
https://github.com/catapult-project/catapult/issues/1977#issuecomment-346814799

As part of this I was also thinking to write some documentation on the network_controller and how it normally interacts with browser backends and shared states.

Meanwhile, do cc me on CLs for the new browser backend to make sure we remain in sync, and let me know if you have any questions about network_controller thingies.

Re #10: IMHO, having Telemetry trying to "guess" the platform/browser from the binary is going to problematic. I think it's better to be as explicit as possible in the flags passed to Telemetry, and then have Telemetry complain if some combinations do not appear to make sense (e.g. selecting the wrong binary for a given platform or browser backend).

I find the existing "--browser=release" problematic for that same reason. "Release" is not a browser. We unavoidably have to ask "release version of what?", and thus rely on cues from other flags to "guess" the actual browser/platform one possibly means.

We should rather try to be as explicit as possible. In my ideal world one would type something like:

* --browser=cros-linux [--browser-executable=<path to the binary>]
* --browser=cros-linux --browser-version=release

Or something along those lines, which also allows to distinguish between the existing CrOSBrowserBackend (cros-chrome) and the new proposed CrosLinuxBrowserBackend (cros-linux).

On the longer term, this needs some more thought (certainly a design doc), on how to overhaul the entire browser selection for all of Telemetry.

In the short term, whatever solution we agree on for the specific cros-linux case, I hope it is geared towards being more rather than less explicit.

I'd +1 the comments in #11 by perezju@

Trying to "guess" the platform is also problematic in a few other cases
   -  running locally with with compile directory path naming
   -  exposing telemetry to bugs in chrome
   -  it's currently buggy see issue 773854

We ended up working around this by adding new chrome build configs solely to create new binary names to bypass the current guessing.

By making the type explicit as in #11, we'd gain a bit of initial setup complexity when first configuring the bots. However I think that would be worth it. Especially as the bot configs are already gaining complexity from including workarounds.


chiniforooshan@ regarding #8 your proposal there for @decorators.Disabled('CrosLinuxBrowser') would make the correctness easy. We'd be able to move away from --skip usage.

--version just prints something like: Chromium 64.0.3280.0. We could introduce a new flag that prints more info and exits.

With the proposal in #11, I think using "--browser=cros-linux" to hint Telemetry to pick up CrosLinuxBrowserBackend, then having a extra assertion that make sure it's the right browser sgtm.

However, I think it still important to have the guessing feature, otherwise people would always need to type "--browser=linux --browser-path=Release" or "--browser=cros-linux --browser-path=Release" if they run the command locally. 

Majority of Chromium devs only build one version of Chrome browser on their workstation, so we should try to make the local workflow simple for them.

E.g: here is a complaint in the past about how verbose it was to use Telemetry: https://groups.google.com/a/chromium.org/d/msg/blink-dev/m0X9L1gN_z4/pjn96FJ3CAAJ

Thanks for the feedback everyone. So it looks like the following plan should be OK (please let me know if you disagree):

Step 1: Add the new browser backend that can be selected only by --browser=cros-linux. Make sure @decorators.Disabled works with the new backend.

Step 2: Add a flag to chromium to print info about target OS.

Step 3: Make Telemetry's browser finder able to pick the CrosLinuxBrowserBackend automatically using the flag added in Step 2, and also verify that the chrome binary is the right one when --browser=cros-linux is specified.

#15: 

I think you would also need step 0 that refactor the "release", "debug", "default"... to the new "--browser-path=..." flag. 

The rest of steps sound great to me!