Add CommandLineArgumentParameter and use it for WebView tests

Add CommandLineArgumentParameter and use it for WebView tests

Introduce the new parameter: CommandLineArgumentParameter and
use it for running Android WebView instrumentation tests in
two modes: single-process and with sandboxed renderer.

The tests that currently fail in the sandboxed renderer mode
are marked with empty @ParameterizedTest.Set annotation to
override the superclass annotation.

BUG=526885
Committed: https://crrev.com/0d1904576d3765b0eb9ad618ec58c0fd8338bbea
Cr-Commit-Position: refs/heads/master@{#358459}

[mnaganov (inactive), 2015-10-26 22:49:46]

Description was changed from

==========
[Android WebView] Try running tests with sandboxed renderer

BUG=526885
==========

to

==========
Add CommandLineArgumentParameter and use it for WebView tests

Introduce the new parameter: CommandLineArgumentParameter and
use it for running Android WebView instrumentation tests in
two modes: single-process and with sandboxed renderer.

The tests that currently fail in the sandboxed renderer mode
are marked with empty @ParameterizedTest.Set annotation to
override the superclass annotation.

BUG=526885
==========

[mnaganov (inactive), 2015-10-26 22:49:46]

mnaganov@chromium.org changed reviewers:
+ jbudorick@chromium.org

[mnaganov (inactive), 2015-10-26 22:55:15]

Hi John,

Finally, we are getting close to our aim! Please tell me what do you think.

[jbudorick, 2015-10-27 16:14:37]

https://codereview.chromium.org/1414403002/diff/60001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnScaleChangedTest.java
(right):

https://codereview.chromium.org/1414403002/diff/60001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnScaleChangedTest.java:18:
@ParameterizedTest.Set
What exactly are these doing...?

https://codereview.chromium.org/1414403002/diff/60001/base/test/android/javat...
File
base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/CommandLineArgumentParameter.java
(right):

https://codereview.chromium.org/1414403002/diff/60001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/CommandLineArgumentParameter.java:34:
public class CommandLineArgumentParameter extends BaseParameter {
I'm not crazy about having both this and @CommandLineFlags.Add/Remove. Maybe we
could merge them somehow...

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:35: class
TestRunner(base_test_runner.BaseTestRunner):
beware that I'm actively working on deprecating this entire class in favor of
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:188: [(["--flag-to-add"],
[]), ([], ["--flag-to-remove"]), ([], [])]
This would be more readable if it returned a collections.namedtuple instead of a
tuple. A client could then refer to them via e.g. flags.to_add instead of
flags[0].

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:399: cmdline_parameters =
self._GetTestCmdlineParameters(test)
This should be part of test list generation, not test execution.

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:405: if
self._IsFreTest(test):
We should be able to remove this in light of
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/syn...

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:430: result.GetName() + '
with {' + ' '.join(cmdline_modifiers[0]) + '}')
This name should be generated once, presumably before the try block.

[mnaganov (inactive), 2015-10-27 23:53:30]

Thanks for brilliant comments, but I disagree with some of them, please see my
replies.

https://codereview.chromium.org/1414403002/diff/60001/android_webview/javates...
File
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnScaleChangedTest.java
(right):

https://codereview.chromium.org/1414403002/diff/60001/android_webview/javates...
android_webview/javatests/src/org/chromium/android_webview/test/AwContentsClientOnScaleChangedTest.java:18:
@ParameterizedTest.Set
On 2015/10/27 16:14:36, jbudorick wrote:
> What exactly are these doing...?

Overriding @ParameterizedTest.Set from AwTestBase. See _GetTestsFromProguard in
instrumentation_test_instance.py -- they just take the annotation dictionaries
and collapse them, starting from superclasses and down to the test method.

https://codereview.chromium.org/1414403002/diff/60001/base/test/android/javat...
File
base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/CommandLineArgumentParameter.java
(right):

https://codereview.chromium.org/1414403002/diff/60001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/CommandLineArgumentParameter.java:34:
public class CommandLineArgumentParameter extends BaseParameter {
On 2015/10/27 16:14:36, jbudorick wrote:
> I'm not crazy about having both this and @CommandLineFlags.Add/Remove. Maybe
we
> could merge them somehow...

I thought about that, but what I disliked is wordiness of ParameterizedTest
syntax, so instead of:

@CommandLineFlags.Add(ChromeSwitches.ENABLE_APP_INSTALL_ALERTS)

we will now have:

@ParameterizedTest(parameters = {
  @Parameter(
    tag = CommandLineArgumentParameter.PARAMETER_TAG,
      arguments = {
        @Parameter.Argument(
          name = CommandLineArgumentParameter.ADD_ARG, stringArray =
{ChromeSwitches.ENABLE_APP_INSTALL_ALERTS})
})})})

Kind of gross, isn't it?

So I would reserve ParameterizedTest for the cases where we actually need to
alternate flags.

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:35: class
TestRunner(base_test_runner.BaseTestRunner):
On 2015/10/27 16:14:37, jbudorick wrote:
> beware that I'm actively working on deprecating this entire class in favor of
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...

No problem, how do you run it?

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:188: [(["--flag-to-add"],
[]), ([], ["--flag-to-remove"]), ([], [])]
On 2015/10/27 16:14:36, jbudorick wrote:
> This would be more readable if it returned a collections.namedtuple instead of
a
> tuple. A client could then refer to them via e.g. flags.to_add instead of
> flags[0].

Thanks! I'm learning neat Python tricks with you :)

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:399: cmdline_parameters =
self._GetTestCmdlineParameters(test)
On 2015/10/27 16:14:37, jbudorick wrote:
> This should be part of test list generation, not test execution.

But... isn't test list consist of just test names? And then the test name is
just passed directly to the instrumentation command.

I guess, implementing your suggestion will require a big change in how the
information about tests is passed around the test framework.

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:405: if
self._IsFreTest(test):
On 2015/10/27 16:14:37, jbudorick wrote:
> We should be able to remove this in light of
>
https://code.google.com/p/chromium/codesearch#chromium/src/chrome/android/syn...

Oh, cool. I thought that maybe it was again important for someone that this is a
@Feature.

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:430: result.GetName() + '
with {' + ' '.join(cmdline_modifiers[0]) + '}')
On 2015/10/27 16:14:37, jbudorick wrote:
> This name should be generated once, presumably before the try block.

We can't change the 'test' string, because it is then used for finding some
meta-info about the test. That's why I'm changing the name of the result only.
Since we are changing it before adding to the 'results', it should be OK.

I've changed the code to update the result name and append it to results after
the 'try' block, so it's only done once now.

[jbudorick, 2015-10-28 16:36:35]

https://codereview.chromium.org/1414403002/diff/60001/base/test/android/javat...
File
base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/CommandLineArgumentParameter.java
(right):

https://codereview.chromium.org/1414403002/diff/60001/base/test/android/javat...
base/test/android/javatests/src/org/chromium/base/test/util/parameter/parameters/CommandLineArgumentParameter.java:34:
public class CommandLineArgumentParameter extends BaseParameter {
On 2015/10/27 23:53:30, mnaganov wrote:
> On 2015/10/27 16:14:36, jbudorick wrote:
> > I'm not crazy about having both this and @CommandLineFlags.Add/Remove. Maybe
> we
> > could merge them somehow...
> 
> I thought about that, but what I disliked is wordiness of ParameterizedTest
> syntax, so instead of:
> 
> @CommandLineFlags.Add(ChromeSwitches.ENABLE_APP_INSTALL_ALERTS)
> 
> we will now have:
> 
> @ParameterizedTest(parameters = {
>   @Parameter(
>     tag = CommandLineArgumentParameter.PARAMETER_TAG,
>       arguments = {
>         @Parameter.Argument(
>           name = CommandLineArgumentParameter.ADD_ARG, stringArray =
> {ChromeSwitches.ENABLE_APP_INSTALL_ALERTS})
> })})})
> 
> Kind of gross, isn't it?

Yeah, that's pretty nasty.

What about something like

public class CommandLineFlags {
  @interface Add
  @interface Remove
  public class Parameter extends BaseParameter {
    ...
  }

}

?

That way the flags are all handled in one place, but we retain the simple
@CommandLineFlags.Add case.

> 
> So I would reserve ParameterizedTest for the cases where we actually need to
> alternate flags.

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:35: class
TestRunner(base_test_runner.BaseTestRunner):
On 2015/10/27 23:53:30, mnaganov wrote:
> On 2015/10/27 16:14:37, jbudorick wrote:
> > beware that I'm actively working on deprecating this entire class in favor
of
> >
>
https://code.google.com/p/chromium/codesearch#chromium/src/build/android/pyli...
> 
> No problem, how do you run it?

It's a bit verbose at the moment, but I'm working on switching our wrapper
scripts to it:

./build/android/test_runner.py instrumentation --enable-platform-mode -e local
--apk-under-test out/Debug/apks/AndroidWebView.apk --test-apk
out/Debug/apks/AndroidWebViewTest.apk --isolate-file-path
android_webview/android_webview_test_apk.isolate

https://codereview.chromium.org/1414403002/diff/60001/build/android/pylib/ins...
build/android/pylib/instrumentation/test_runner.py:399: cmdline_parameters =
self._GetTestCmdlineParameters(test)
On 2015/10/27 23:53:30, mnaganov wrote:
> On 2015/10/27 16:14:37, jbudorick wrote:
> > This should be part of test list generation, not test execution.
> 
> But... isn't test list consist of just test names? And then the test name is
> just passed directly to the instrumentation command.

Oh right, that's true in this version. In the new one, tests are dicts:

{
  'class': '...',
  'method': '...',
  'annotations': { ... }
}

so yeah, this suggestion isn't implementable in this version.

> 
> I guess, implementing your suggestion will require a big change in how the
> information about tests is passed around the test framework.

[mnaganov (inactive), 2015-10-28 20:21:33]

John, I have addressed the two remaining comments.

One thing I'm not sure about is the branch in
LocalDeviceInstrumentationTestRun._RunTest where 'test' is a list. Do we need to
support command-line params in this case, and how?

[mnaganov (inactive), 2015-10-30 16:13:52]

John, a friendly ping -- could you please take a look sometime today?

[jbudorick, 2015-10-30 16:14:57]

On 2015/10/30 16:13:52, mnaganov wrote:
> John, a friendly ping -- could you please take a look sometime today?

Sure, sorry.

[jbudorick, 2015-11-02 14:49:46]

I'm concerned that this will only work on local devices -- i.e., it won't work
on appurify. Making it work would likely require doing most of this work on the
device side in one of two ways:
 - have OnDeviceInstrumentationDriver duplicate a lot of this, or
 - have the APK handle the command-line flag stuff and have the test runner only
tell the APK which run it's on.

Both of those are pretty tricky, though, and I'm not sure they're necessary for
this CL, but they would both involve substantially redoing what you have here.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:182: if
'arguments' in p:
nit: combine these two lines into

  for a in p.get('arguments', []):

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:185: to_add
= [('--' + f) for f in a['stringArray']]
nit: ['--%s' % f for f in a['stringArray']]

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:187:
to_remove = [('--' + f) for f in a['stringArray']]
nit: same

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:450: return
self._InflateTests(self._FilterTests(tests))
re below: add the separate function call here.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:568:
parameters = ParseCommandLineFlagParameters(a)
Do this in a separate function.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:117:
def _GetFullTestName(self, test):
nit: I'd rather call this the display name or something like that since it's
only for user-visible output (as compared to the test name, which is used for
filtering).

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:167:
self._flag_changers[str(device)].PushFlags(
Note that this flag changer may not have been created here.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:177:
self._flag_changers[str(device)].Restore()
This should be done in a finally block s.t. it gets called even if
StartInstrumentation fails. (or we could add a flag_changer method that acts as
a context manager)

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:189:
if flags:
This shouldn't have to be done, but I need to do some restructuring to make that
possible.

[mnaganov (inactive), 2015-11-02 19:03:54]

> Both of those are pretty tricky, though, and I'm not sure they're necessary
for this CL, but they would both involve substantially redoing what you have
here.

Well, my goal is to start running these parameterized tests ASAP, so I would
like to have at least running them locally implemented. I can help redoing the
code for running remotely -- I guess, the code that parses annotations and
converts them into flags will stay the same anyway, and the changes you are
talking about is mostly about plumbing that code properly.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:182: if
'arguments' in p:
On 2015/11/02 14:49:46, jbudorick wrote:
> nit: combine these two lines into
> 
>   for a in p.get('arguments', []):

Done.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:185: to_add
= [('--' + f) for f in a['stringArray']]
On 2015/11/02 14:49:46, jbudorick wrote:
> nit: ['--%s' % f for f in a['stringArray']]

Done.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:187:
to_remove = [('--' + f) for f in a['stringArray']]
On 2015/11/02 14:49:46, jbudorick wrote:
> nit: same

Done.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:450: return
self._InflateTests(self._FilterTests(tests))
On 2015/11/02 14:49:45, jbudorick wrote:
> re below: add the separate function call here.

Done.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:568:
parameters = ParseCommandLineFlagParameters(a)
On 2015/11/02 14:49:46, jbudorick wrote:
> Do this in a separate function.

Done.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
File build/android/pylib/local/device/local_device_instrumentation_test_run.py
(right):

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:117:
def _GetFullTestName(self, test):
On 2015/11/02 14:49:46, jbudorick wrote:
> nit: I'd rather call this the display name or something like that since it's
> only for user-visible output (as compared to the test name, which is used for
> filtering).

Done.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:167:
self._flag_changers[str(device)].PushFlags(
On 2015/11/02 14:49:46, jbudorick wrote:
> Note that this flag changer may not have been created here.

Good catch, fixed!

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:177:
self._flag_changers[str(device)].Restore()
On 2015/11/02 14:49:46, jbudorick wrote:
> This should be done in a finally block s.t. it gets called even if
> StartInstrumentation fails. (or we could add a flag_changer method that acts
as
> a context manager)

Done.

https://codereview.chromium.org/1414403002/diff/140001/build/android/pylib/lo...
build/android/pylib/local/device/local_device_instrumentation_test_run.py:189:
if flags:
On 2015/11/02 14:49:46, jbudorick wrote:
> This shouldn't have to be done, but I need to do some restructuring to make
that
> possible.

Acknowledged.

[mnaganov (inactive), 2015-11-04 16:19:11]

Friendly ping!

[jbudorick, 2015-11-04 16:34:34]

lgtm w/ nits

Thanks for the work (and patience) on this! :)

+sean fyi

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:11: from
collections import namedtuple
nit: import the module rather than the function (so here, import collections,
then use collections.namedtuple below)

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:46:
_PARAMETERIZED_TEST_ANNOTATION = 'ParameterizedTest'
nit: alphabetize these

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:451:
self._InflateTests(self._FilterTests(tests)))
nit: 4 space indent

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
build/android/pylib/instrumentation/test_runner.py:12: from collections import
namedtuple
nit: same module import nit

[sean, 2015-11-04 16:53:13]

It's nice to see this being used. Thanks for adding me, John. :)

[mnaganov (inactive), 2015-11-04 17:51:04]

Thanks, John!

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
File build/android/pylib/instrumentation/instrumentation_test_instance.py
(right):

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:11: from
collections import namedtuple
On 2015/11/04 16:34:34, jbudorick wrote:
> nit: import the module rather than the function (so here, import collections,
> then use collections.namedtuple below)
> 

Done.

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:46:
_PARAMETERIZED_TEST_ANNOTATION = 'ParameterizedTest'
On 2015/11/04 16:34:34, jbudorick wrote:
> nit: alphabetize these

Done.

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
build/android/pylib/instrumentation/instrumentation_test_instance.py:451:
self._InflateTests(self._FilterTests(tests)))
On 2015/11/04 16:34:34, jbudorick wrote:
> nit: 4 space indent

Done.

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
File build/android/pylib/instrumentation/test_runner.py (right):

https://codereview.chromium.org/1414403002/diff/160001/build/android/pylib/in...
build/android/pylib/instrumentation/test_runner.py:12: from collections import
namedtuple
On 2015/11/04 16:34:34, jbudorick wrote:
> nit: same module import nit

Done.

[mnaganov (inactive), 2015-11-04 17:53:09]

mnaganov@chromium.org changed reviewers:
+ yfriedman@chromium.org

[mnaganov (inactive), 2015-11-04 17:53:10]

yfriedman@: Please review changes in base/test/android/

[mnaganov (inactive), 2015-11-04 22:43:53]

mnaganov@chromium.org changed reviewers:
+ nyquist@chromium.org
- yfriedman@chromium.org

[mnaganov (inactive), 2015-11-04 22:43:54]

Yaron seems to be on some week-long event, so switching to Tommy.

nyquist@: PTAL base/test/android/

[nyquist, 2015-11-05 23:52:52]

lgtm

https://codereview.chromium.org/1414403002/diff/180001/base/test/android/java...
File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java
(right):

https://codereview.chromium.org/1414403002/diff/180001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:160:
// Remove all @ParameterizedTests that contain CommandLineArgumentParameter --
those
Nit: {@link CommandLineFlags.Parameter}?

[mnaganov (inactive), 2015-11-06 00:30:34]

Thanks, Tommy!

https://codereview.chromium.org/1414403002/diff/180001/base/test/android/java...
File base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java
(right):

https://codereview.chromium.org/1414403002/diff/180001/base/test/android/java...
base/test/android/javatests/src/org/chromium/base/test/BaseTestResult.java:160:
// Remove all @ParameterizedTests that contain CommandLineArgumentParameter --
those
On 2015/11/05 23:52:52, nyquist wrote:
> Nit: {@link CommandLineFlags.Parameter}?

Fixed. Although, this isn't a JavaDoc, so I didn't make it a link.

[mnaganov (inactive), 2015-11-06 00:31:11]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-11-06 00:31:11]

The patchset sent to the CQ was uploaded after l-g-t-m from
jbudorick@chromium.org, nyquist@chromium.org
Link to the patchset: https://codereview.chromium.org/1414403002/#ps200001
(title: "Fixed comment")

[commit-bot: I haz the power, 2015-11-06 00:32:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414403002/200001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414403002/200001

[commit-bot: I haz the power, 2015-11-06 01:10:10]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-06 01:10:10]

Try jobs failed on following builders:
  android_clang_dbg_recipe on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/android_clang_d...)

[mnaganov (inactive), 2015-11-06 01:31:24]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-11-06 01:31:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1414403002/#ps220001
(title: "Make CommandLineFlags.Parameter static")

[commit-bot: I haz the power, 2015-11-06 01:33:17]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414403002/220001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414403002/220001

[commit-bot: I haz the power, 2015-11-06 04:25:46]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-11-06 04:25:46]

Try jobs failed on following builders:
  linux_android_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_android_r...)

[mnaganov (inactive), 2015-11-06 17:43:54]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-11-06 17:43:55]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1414403002/#ps50026
(title: "Update tests exclusions")

[commit-bot: I haz the power, 2015-11-06 17:44:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414403002/50026
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414403002/50026

[mnaganov (inactive), 2015-11-06 21:28:32]

The CQ bit was unchecked by mnaganov@chromium.org

[mnaganov (inactive), 2015-11-06 21:30:45]

The CQ bit was checked by mnaganov@chromium.org

[mnaganov (inactive), 2015-11-06 21:30:45]

The patchset sent to the CQ was uploaded after l-g-t-m from
nyquist@chromium.org, jbudorick@chromium.org
Link to the patchset: https://codereview.chromium.org/1414403002/#ps250001
(title: "One more test needs to be excluded")

[commit-bot: I haz the power, 2015-11-06 21:32:19]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1414403002/250001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1414403002/250001

[commit-bot: I haz the power, 2015-11-07 00:11:51]

Committed patchset #14 (id:250001)

[commit-bot: I haz the power, 2015-11-07 00:13:03]

Patchset 14 (id:??) landed as
https://crrev.com/0d1904576d3765b0eb9ad618ec58c0fd8338bbea
Cr-Commit-Position: refs/heads/master@{#358459}

[jbudorick, 2015-11-07 03:47:38]

On 2015/11/07 00:13:03, commit-bot: I haz the power wrote:
> Patchset 14 (id:??) landed as
> https://crrev.com/0d1904576d3765b0eb9ad618ec58c0fd8338bbea
> Cr-Commit-Position: refs/heads/master@{#358459}

This appears to have added multiple minutes to CQ cycle time. I'm going to look
at our metrics on Monday and may wind up (at least partially) reverting this for
the time being.

[mnaganov (inactive), 2015-11-07 05:23:02]

On 2015/11/07 03:47:38, jbudorick wrote:
> On 2015/11/07 00:13:03, commit-bot: I haz the power wrote:
> > Patchset 14 (id:??) landed as
> > https://crrev.com/0d1904576d3765b0eb9ad618ec58c0fd8338bbea
> > Cr-Commit-Position: refs/heads/master@{#358459}
> 
> This appears to have added multiple minutes to CQ cycle time. I'm going to
look
> at our metrics on Monday and may wind up (at least partially) reverting this
for
> the time being.

Well, it used to take about 4 minutes for WebView tests:

Instrumentation test AndroidWebViewTest (with patch) Instrumentation test
AndroidWebViewTest (with patch) ( 3 mins, 50 secs )

Now it takes a couple of minutes more:

Instrumentation test AndroidWebViewTest (with patch) Instrumentation test
AndroidWebViewTest (with patch) ( 6 mins, 31 secs )

That's what I would expect as we basically doubled the amount of tests we run.

Running the tests is in the new mode is very important for us. And we can't turn
off the old mode yet.

If this time increase bothers you very much, let's think what else we can do in
order to address this issue.

[boliu, 2015-11-07 05:37:20]

On 2015/11/07 05:23:02, mnaganov wrote:
> On 2015/11/07 03:47:38, jbudorick wrote:
> > On 2015/11/07 00:13:03, commit-bot: I haz the power wrote:
> > > Patchset 14 (id:??) landed as
> > > https://crrev.com/0d1904576d3765b0eb9ad618ec58c0fd8338bbea
> > > Cr-Commit-Position: refs/heads/master@{#358459}
> > 
> > This appears to have added multiple minutes to CQ cycle time. I'm going to
> look
> > at our metrics on Monday and may wind up (at least partially) reverting this
> for
> > the time being.
> 
> Well, it used to take about 4 minutes for WebView tests:
> 
> Instrumentation test AndroidWebViewTest (with patch) Instrumentation test
> AndroidWebViewTest (with patch) ( 3 mins, 50 secs )
> 
> Now it takes a couple of minutes more:
> 
> Instrumentation test AndroidWebViewTest (with patch) Instrumentation test
> AndroidWebViewTest (with patch) ( 6 mins, 31 secs )
> 
> That's what I would expect as we basically doubled the amount of tests we run.
> 
> Running the tests is in the new mode is very important for us. And we can't
turn
> off the old mode yet.
> 
> If this time increase bothers you very much, let's think what else we can do
in
> order to address this issue.

Just curious, what percentage of tests can run and pass with multiprocess today?

For most tests, if there is only capacity to running in one of multiprocess or
in-process, the right trade off is probably multiprocess?

Probably worth looking into things like crbug.com/528037 first though.

[mnaganov (inactive), 2015-11-09 18:13:18]

On 2015/11/07 05:37:20, boliu wrote:
> On 2015/11/07 05:23:02, mnaganov wrote:
> > On 2015/11/07 03:47:38, jbudorick wrote:
> > > On 2015/11/07 00:13:03, commit-bot: I haz the power wrote:
> > > > Patchset 14 (id:??) landed as
> > > > https://crrev.com/0d1904576d3765b0eb9ad618ec58c0fd8338bbea
> > > > Cr-Commit-Position: refs/heads/master@{#358459}
> > > 
> > > This appears to have added multiple minutes to CQ cycle time. I'm going to
> > look
> > > at our metrics on Monday and may wind up (at least partially) reverting
this
> > for
> > > the time being.
> > 
> > Well, it used to take about 4 minutes for WebView tests:
> > 
> > Instrumentation test AndroidWebViewTest (with patch) Instrumentation test
> > AndroidWebViewTest (with patch) ( 3 mins, 50 secs )
> > 
> > Now it takes a couple of minutes more:
> > 
> > Instrumentation test AndroidWebViewTest (with patch) Instrumentation test
> > AndroidWebViewTest (with patch) ( 6 mins, 31 secs )
> > 
> > That's what I would expect as we basically doubled the amount of tests we
run.
> > 
> > Running the tests is in the new mode is very important for us. And we can't
> turn
> > off the old mode yet.
> > 
> > If this time increase bothers you very much, let's think what else we can do
> in
> > order to address this issue.
> 
> Just curious, what percentage of tests can run and pass with multiprocess
today?
>

Didn't count, but I'm hoping that with the new synchronous compositor,
only the tests that read back the screen contents, or test inline video
will be excluded. I will verify that soon.
 
> For most tests, if there is only capacity to running in one of multiprocess or
> in-process, the right trade off is probably multiprocess?
>

That will be scary, because single process is not an "official" mode for
anyone except WebView, so if even us would not be testing it, then all
the issues specific to it will be discovered in production only.
 
> Probably worth looking into things like crbug.com/528037 first though.