Android: support glob-style gtest filters with content browser tests

Android: support glob-style gtest filters with content browser tests

This patch makes it possible to use glob-style gtest filters with
the content browser tests. An example of such a filter is
WebGLConformanceTest.*, which should cause all the WebGL conformance
tests to be run.

Previously the gtest filter was passed directly to the test executable,
causing it to run all matching tests successively in the same process.
Normally this would be fine, but with content browsertests we must run
each test in a fresh process to avoid issues with double initialization.

The fix is to expand the filter to a flat list of tests in the test
dispatcher. After this, the tests are sharded and run in exactly the
same way as when no gtest filter is provided.

BUG=138226
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=204680

[bulach, 2013-05-13 13:19:37]

lgtm but please wait for nilesh/craig/frank since I haven't touched this in
quite a while :)

tiny suggestion below:

https://codereview.chromium.org/14882007/diff/1/build/android/pylib/browserte...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/1/build/android/pylib/browserte...
build/android/pylib/browsertests/dispatch.py:87: """Computes the result of
applying the given gtest filter to the list of
nit: one liner doctstring as title, then \n then args

[Sami, 2013-05-13 14:58:10]

Thanks Marcus. Adding Nilesh for a second opinion.

https://codereview.chromium.org/14882007/diff/1/build/android/pylib/browserte...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/1/build/android/pylib/browserte...
build/android/pylib/browsertests/dispatch.py:87: """Computes the result of
applying the given gtest filter to the list of
On 2013/05/13 13:19:37, bulach wrote:
> nit: one liner doctstring as title, then \n then args

Done.

[Yaron, 2013-05-13 16:03:47]

Nilesh is on vacation for a few weeks so you probably want someone else.

[frankf, 2013-05-16 01:15:52]

https://codereview.chromium.org/14882007/diff/4001/build/android/pylib/browse...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/4001/build/android/pylib/browse...
build/android/pylib/browsertests/dispatch.py:86: def
_ApplyGtestFilter(all_enabled_tests, gtest_filter):
Take a look at //chrome/test/pylib/common/unittest_util.py#FilterTests

Since this is generic, we can just resuse that.

https://codereview.chromium.org/14882007/diff/4001/build/android/pylib/browse...
build/android/pylib/browsertests/dispatch.py:106: result.difference_update([t
for t in all_tests
all_tests -> all_enabeld_tests

[Sami, 2013-05-16 10:41:33]

Thanks Frank. This version reuses the existing gtest filter code. Also adding
Paweł since I changed the shared code to accept a list of test name strings.
PTAL.

https://codereview.chromium.org/14882007/diff/4001/build/android/pylib/browse...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/4001/build/android/pylib/browse...
build/android/pylib/browsertests/dispatch.py:86: def
_ApplyGtestFilter(all_enabled_tests, gtest_filter):
On 2013/05/16 01:15:52, frankf wrote:
> Take a look at //chrome/test/pylib/common/unittest_util.py#FilterTests
> 
> Since this is generic, we can just resuse that.

Thanks for the tip -- code reuse ftw!

https://codereview.chromium.org/14882007/diff/4001/build/android/pylib/browse...
build/android/pylib/browsertests/dispatch.py:106: result.difference_update([t
for t in all_tests
On 2013/05/16 01:15:52, frankf wrote:
> all_tests -> all_enabeld_tests

Oops, well spotted.

[frankf, 2013-05-16 17:15:18]

lgtm w/ a question

Thanks!

https://codereview.chromium.org/14882007/diff/9001/build/android/pylib/browse...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/9001/build/android/pylib/browse...
build/android/pylib/browsertests/dispatch.py:63: all_tests =
unittest_util.FilterTests(all_enabled, options.gtest_filter)
So we don't want to filter out PRE_ and MANUAL_ in this case since gtest_filter
option is only supplied for running tests locally?

[Sami, 2013-05-16 17:38:26]

On 2013/05/16 17:15:18, frankf wrote:
> So we don't want to filter out PRE_ and MANUAL_ in this case since
gtest_filter
> option is only supplied for running tests locally?

That's right, we should run exactly the tests the user asks for. The bots will
keep using the earlier filtering logic.

[Paweł Hajdan Jr., 2013-05-16 18:26:28]

https://codereview.chromium.org/14882007/diff/9001/build/android/pylib/browse...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/9001/build/android/pylib/browse...
build/android/pylib/browsertests/dispatch.py:20: from common import
unittest_util
This suggest things should be moved down to build pylib instead of chrome
dependency being added here.

https://codereview.chromium.org/14882007/diff/9001/build/android/pylib/browse...
build/android/pylib/browsertests/dispatch.py:63: all_tests =
unittest_util.FilterTests(all_enabled, options.gtest_filter)
On 2013/05/16 17:15:18, frankf wrote:
> So we don't want to filter out PRE_ and MANUAL_ in this case since
gtest_filter
> option is only supplied for running tests locally?

Oh, why are you trying to duplicate test_launcher.cc functionality? :-/

Additionally, you'd also be duplicating run_test_cases.py functionality. Kindly
do not reinvent the wheel. :)

[Sami, 2013-05-16 18:55:26]

On 2013/05/16 18:26:28, Paweł Hajdan Jr. wrote:
> This suggest things should be moved down to build pylib instead of chrome
> dependency being added here.

That sounds like a good idea. I'll give it a try.
 
>
https://codereview.chromium.org/14882007/diff/9001/build/android/pylib/browse...
> build/android/pylib/browsertests/dispatch.py:63: all_tests =
> unittest_util.FilterTests(all_enabled, options.gtest_filter)
> On 2013/05/16 17:15:18, frankf wrote:
> > So we don't want to filter out PRE_ and MANUAL_ in this case since
> gtest_filter
> > option is only supplied for running tests locally?
> 
> Oh, why are you trying to duplicate test_launcher.cc functionality? :-/
> 
> Additionally, you'd also be duplicating run_test_cases.py functionality.
Kindly
> do not reinvent the wheel. :)

I know next to nothing about test infrastructure, but it seems that neither
test_launcher.cc or run_test_cases.py work on Android. Maybe the way to
reconcile this is to start sharing base code like in this patch so at least that
isn't duplicated.

[Sami, 2013-05-28 16:35:18]

This version moves the shared test utilities to build/pylib, which is a new
directory. Note that I've deleted chrome/test/pylib/common/chrome_paths.py since
no-one seemed to be using it.

PTAL.

[bulach, 2013-05-28 16:42:39]

suggestion below, apologies for my poor choice in the first place.. :-/

https://codereview.chromium.org/14882007/diff/17001/build/android/pylib/brows...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/17001/build/android/pylib/brows...
build/android/pylib/browsertests/dispatch.py:18:
sys.path.append(os.path.join(constants.CHROME_DIR, 'build', 'pylib'))
"pylib" was named as such because in that tree, build/android, it was the only
"python library" directory, and build/android was a top-level with executable
python.
in hindsight, it should've been "apylib", or something... :-/

having build/pylib would be very confusing... perhaps build/buildpylib would be
clearer?

[Sami, 2013-05-28 16:48:51]

On 2013/05/28 16:42:39, bulach wrote:
> having build/pylib would be very confusing... perhaps build/buildpylib would
be
> clearer?

Works for me. I've lost count of how many pylibs we have in the repo already :)

[Paweł Hajdan Jr., 2013-05-28 17:00:06]

https://codereview.chromium.org/14882007/diff/21001/build/android/pylib/brows...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/21001/build/android/pylib/brows...
build/android/pylib/browsertests/dispatch.py:18:
sys.path.append(os.path.join(constants.CHROME_DIR, 'build', 'buildpylib'))
Looks like files have been moved down to build - is this line still needed here?

https://codereview.chromium.org/14882007/diff/21001/build/buildpylib/common/u...
File build/buildpylib/common/unittest_util.py (right):

https://codereview.chromium.org/14882007/diff/21001/build/buildpylib/common/u...
build/buildpylib/common/unittest_util.py:124: test_name = test if
isinstance(test, str) else GetTestName(test)
Is this autodetection really needed and useful? I've found it much more
maintainable to have uniform and predictable types.

[frankf, 2013-05-28 17:09:21]

Not wanting to start a bike shed argument, but there's already build/util
directory, it might make sense to just move these scripts there (maybe
build/util/lib). At first glance, "buildpylib" seems unintuitive.

On 2013/05/28 16:48:51, Sami wrote:
> On 2013/05/28 16:42:39, bulach wrote:
> > having build/pylib would be very confusing... perhaps build/buildpylib would
> be
> > clearer?
> 
> Works for me. I've lost count of how many pylibs we have in the repo already
:)

[Sami, 2013-05-28 17:17:06]

https://codereview.chromium.org/14882007/diff/21001/build/android/pylib/brows...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/21001/build/android/pylib/brows...
build/android/pylib/browsertests/dispatch.py:18:
sys.path.append(os.path.join(constants.CHROME_DIR, 'build', 'buildpylib'))
On 2013/05/28 17:00:06, Paweł Hajdan Jr. wrote:
> Looks like files have been moved down to build - is this line still needed
here?

build/android/pylib is still a distinct set of modules from the newly added
build/buildpylib so unless I missed something I think this is still needed. I
didn't merge the pylib under android with buildpylib because there's now
non-android code calling into the latter.

https://codereview.chromium.org/14882007/diff/21001/build/buildpylib/common/u...
File build/buildpylib/common/unittest_util.py (right):

https://codereview.chromium.org/14882007/diff/21001/build/buildpylib/common/u...
build/buildpylib/common/unittest_util.py:124: test_name = test if
isinstance(test, str) else GetTestName(test)
On 2013/05/28 17:00:06, Paweł Hajdan Jr. wrote:
> Is this autodetection really needed and useful? I've found it much more
> maintainable to have uniform and predictable types.

Agreed, this is a little too magical. I've split this into two functions:
FilterTests and FilterTestNames.

[Sami, 2013-05-28 17:20:24]

On 2013/05/28 17:09:21, frankf wrote:
> Not wanting to start a bike shed argument, but there's already build/util
> directory, it might make sense to just move these scripts there (maybe
> build/util/lib). At first glance, "buildpylib" seems unintuitive.

Yeah, build/util/lib sgtm. Done.

[frankf, 2013-05-28 17:41:43]

lgtm w/ nit

https://codereview.chromium.org/14882007/diff/30001/chrome/test/install_test/...
File chrome/test/install_test/install_test.py (right):

https://codereview.chromium.org/14882007/diff/30001/chrome/test/install_test/...
chrome/test/install_test/install_test.py:24:
sys.path.append(os.path.join(_DIRECTORY, os.path.pardir, 'pylib'))
Delete?

[bulach, 2013-05-28 18:08:36]

lgtm, but the problem still remains :)

since "util" is not a top-level module, the import is "import lib" or "from lib
import foo", which is still ambiguous... it doesn't matter much, but perhaps
either make util a module (i..e., with __init__, so "from util import lib") or
make lib less ambiguous?

[Sami, 2013-05-28 18:13:29]

https://codereview.chromium.org/14882007/diff/30001/chrome/test/install_test/...
File chrome/test/install_test/install_test.py (right):

https://codereview.chromium.org/14882007/diff/30001/chrome/test/install_test/...
chrome/test/install_test/install_test.py:24:
sys.path.append(os.path.join(_DIRECTORY, os.path.pardir, 'pylib'))
On 2013/05/28 17:41:43, frankf wrote:
> Delete?

Well spotted, thanks.

[Sami, 2013-05-31 10:19:11]

Paweł, any thoughts on the latest version?

[Paweł Hajdan Jr., 2013-05-31 16:49:17]

https://codereview.chromium.org/14882007/diff/34001/build/android/pylib/brows...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/34001/build/android/pylib/brows...
build/android/pylib/browsertests/dispatch.py:18:
sys.path.append(os.path.join(constants.CHROME_DIR, 'build', 'util', 'lib'))
Does CHROME_DIR refer to src or src/chrome? If the latter, this is till not OK.
If the former, the constant name is really confusing - and please get it change
to e.g. DIR_SOURCE_ROOT for consistency with base/base_paths .

[Sami, 2013-05-31 16:59:23]

On 2013/05/31 16:49:17, Paweł Hajdan Jr. wrote:
> Does CHROME_DIR refer to src or src/chrome? If the latter, this is till not
OK.
> If the former, the constant name is really confusing - and please get it
change
> to e.g. DIR_SOURCE_ROOT for consistency with base/base_paths .

CHROME_DIR refers to src/, i.e., the top-level source directory. I can change it
to DIR_SOURCE_ROOT, but I'd prefer to do it in another patch since it's used in
about 80 places across several repositories. How does that sound?

[Sami, 2013-06-04 12:48:27]

Do you think we could go ahead with this patch? I'd like to land this because
since I started working on this, at least one other person has wasted time
trying to figure out why running Android content_browsertests with a gtest
filter fails in very non-obvious ways. That is, this isn't just cleanup of
duplicated code.

[Paweł Hajdan Jr., 2013-06-04 18:02:57]

On 2013/05/31 16:59:23, Sami wrote:
> CHROME_DIR refers to src/, i.e., the top-level source directory. I can change
it
> to DIR_SOURCE_ROOT, but I'd prefer to do it in another patch since it's used
in
> about 80 places across several repositories. How does that sound?

Feel free to do that in a separate CL, but before this lands. I'd just like to
ensure this happens, as otherwise CHROME_DIR is *very* confusing.

[bulach, 2013-06-04 18:11:14]

hey pawel, I understand your desire to clean this up, but I don't see why it'd
block this patch? CHROME_DIR has been named like that in build/pylib for ages,
and this patch would not "leak" that outside that.

so whilst I fully agree the cleanup is needed and the named was poorly chosen in
the first place, I fail to see why is it needed for _this_ patch.. :) would you
be ok in decoupling it as a pre-requisite?

[frankf, 2013-06-05 16:54:53]

Pawel, although your intentions are good, I think it's counter-productive in
this case. Aside from blocking test coverage for content_browsertests, it's also
discouraging code reuse in the future (You were pulled in because I asked Sami
to reuse common test utils). I strongly believe making code reuse easier has
much higher importance than renaming a variable that's been around forever.
Besides, bulach already has a CL out for it.


On 2013/06/04 18:11:14, bulach wrote:
> hey pawel, I understand your desire to clean this up, but I don't see why it'd
> block this patch? CHROME_DIR has been named like that in build/pylib for ages,
> and this patch would not "leak" that outside that.
> 
> so whilst I fully agree the cleanup is needed and the named was poorly chosen
in
> the first place, I fail to see why is it needed for _this_ patch.. :) would
you
> be ok in decoupling it as a pre-requisite?

[scherkus (not reviewing), 2013-06-06 20:22:07]

On 2013/06/05 16:54:53, frankf wrote:
> Pawel, although your intentions are good, I think it's counter-productive in
> this case. Aside from blocking test coverage for content_browsertests, it's
also
> discouraging code reuse in the future (You were pulled in because I asked Sami
> to reuse common test utils). I strongly believe making code reuse easier has
> much higher importance than renaming a variable that's been around forever.
> Besides, bulach already has a CL out for it.

phajdan.jr: Considering bulach@ has CLs in progress (e.g.,
https://codereview.chromium.org/15839006/) I don't see a compelling reason to
delay this CL for a one-line variable rename.

I can attest to frankf@'s claim that not having this landed is negatively
impacting the productivity of anyone working on Android.

Typically I advocate for annotating confusing/ugly code with a TODO() + filing a
bug + landing follow-up CLs. How about taking a similar approach with the
CHROME_DIR references in build/android/pylib/constants.py and
build/android/pylib/browsertests/dispatch.py?

[Paweł Hajdan Jr., 2013-06-06 20:50:57]

https://codereview.chromium.org/14882007/diff/34001/build/android/pylib/brows...
File build/android/pylib/browsertests/dispatch.py (right):

https://codereview.chromium.org/14882007/diff/34001/build/android/pylib/brows...
build/android/pylib/browsertests/dispatch.py:18:
sys.path.append(os.path.join(constants.CHROME_DIR, 'build', 'util', 'lib'))
This can now be DIR_SOURCE_ROOT, right?

After you make that change, LGTM.

[Sami, 2013-06-06 21:25:22]

On 2013/06/06 20:50:57, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/14882007/diff/34001/build/android/pylib/brows...
> File build/android/pylib/browsertests/dispatch.py (right):
> 
>
https://codereview.chromium.org/14882007/diff/34001/build/android/pylib/brows...
> build/android/pylib/browsertests/dispatch.py:18:
> sys.path.append(os.path.join(constants.CHROME_DIR, 'build', 'util', 'lib'))
> This can now be DIR_SOURCE_ROOT, right?
> 
> After you make that change, LGTM.

Done, thanks.

[commit-bot: I haz the power, 2013-06-06 21:26:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/skyostil@chromium.org/14882007/45001

[commit-bot: I haz the power, 2013-06-07 00:50:15]

Change committed as 204680

[nilesh, 2013-06-07 19:25:34]

This broke content_browsertests on fyi. 
@@@BUILD_STEP content_browsertests@@@
> build/android/run_browser_tests.py --verbose
Traceback (most recent call last):
  File "build/android/run_browser_tests.py", line 12, in <module>
    from pylib.browsertests import dispatch
  File
"/b/build/slave/Android_Tests__JB_Nexus7__dbg_/build/src/build/android/pylib/browsertests/dispatch.py",
line 20, in <module>
    from common import unittest_util
ImportError: cannot import name unittest_util
< build/android/run_browser_tests.py --verbose
ERROR: process exited with code 1
@@@STEP_FAILURE@@@

[frankf, 2013-06-08 00:32:18]

On 2013/06/07 19:25:34, nilesh wrote:
> This broke content_browsertests on fyi. 
> @@@BUILD_STEP content_browsertests@@@
> > build/android/run_browser_tests.py --verbose
> Traceback (most recent call last):
>   File "build/android/run_browser_tests.py", line 12, in <module>
>     from pylib.browsertests import dispatch
>   File
>
"/b/build/slave/Android_Tests__JB_Nexus7__dbg_/build/src/build/android/pylib/browsertests/dispatch.py",
> line 20, in <module>
>     from common import unittest_util
> ImportError: cannot import name unittest_util
> < build/android/run_browser_tests.py --verbose
> ERROR: process exited with code 1
> @@@STEP_FAILURE@@@

Fix here: https://codereview.chromium.org/16124010/
The issue is that there's also a common lib on the buildbot side
(build/scripts/common)