Upstream: Test scripts for Android (phase 2)

Upstream: Test scripts for Android (phase 2)

Currently only support run base_unittests

BUG=
TEST=

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=106953

[michaelbai, 2011-10-20 21:36:26]

Hi Nirnimesh

Please help to review this one.

Thanks

[Nirnimesh, 2011-10-21 08:16:15]

I have mostly common style comments:

1. Non-public member vars should have _ prefix
2. Non-public methods should have _ prefix
3. Do not import *
4. All non-trivial classes, methods, functions need a docstring
5. Need only one blank line after copyright license in each file. 2 blank lines
before beginning any classes/functions thereon.
6. Use either print or logging.{info/warning/debug} consistently

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
File build/android/base_test_runner.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
build/android/base_test_runner.py:5: 
nit: only one blank line needed here.

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
build/android/base_test_runner.py:26: Args:
this should go in __init__

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
build/android/base_test_runner.py:31: self.device = device
I don't see self.device being used anywhere else.

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
build/android/base_test_runner.py:32: self.adb =
android_commands.AndroidCommands(device=device)
all member vars should have _ prefix
self._adb

Same for all the other vars below

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
build/android/base_test_runner.py:103: # if IsDevicePortUsed(self.adb,
self.forwarder_device_port):
remove commented out code

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_shar...
File build/android/base_test_sharder.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_shar...
build/android/base_test_sharder.py:30: Args:
should go in __init__

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_shar...
build/android/base_test_sharder.py:37: self.attached_devices = attached_devices
non-public member vars should have _ prefix

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_shar...
build/android/base_test_sharder.py:67: logging.warning('*' * 80)
Why warning?
maybe debug or info?

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
File build/android/chrome_test_server_spawner.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
build/android/chrome_test_server_spawner.py:19: # Path that are needed to import
testserver
cr_src = os.path.join(os.path.abspath(os.path.dirname(__file__)), '..',
  21    '..')
sys.path.append(os.path.join(cr_src, 'third_party')
sys.path.append(os.path.join(cr_src, 'third_party', 'tlslite')
...
...

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
build/android/chrome_test_server_spawner.py:30: _test_servers = []
why global?

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
build/android/chrome_test_server_spawner.py:32: class
SpawningServerRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
docstring please

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
build/android/chrome_test_server_spawner.py:85: print 'Listening on %d, type %d,
data_dir %s' % (options.port,
logging.info()?

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.py
File build/android/flag_changer.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.p...
build/android/flag_changer.py:25: if flags: assert flags[0] != 'chrome'
move "assert flags[0] != 'chrome'" to next line

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.p...
build/android/flag_changer.py:29: if self.old_flags: self.old_flags =
self.old_flags[0].strip()
same here

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.p...
build/android/flag_changer.py:32: # Avoid appending flags that already present.
that -> that are

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.p...
build/android/flag_changer.py:41: def Restore(self):
I'd recommend calling Restore in the d'tor as well, so that the caller does not
have to.

http://codereview.chromium.org/8364020/diff/2001/build/android/gtest_filter/b...
File build/android/gtest_filter/base_unittests_disabled (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/gtest_filter/b...
build/android/gtest_filter/base_unittests_disabled:3: # Automatically generated
by run_tests.py
Automatically generated? Why check in then?

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests.py
File build/android/run_tests.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests.py#n...
build/android/run_tests.py:58: from base_test_sharder import *
do not import *

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests.py#n...
build/android/run_tests.py:68: ]
align under _

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests.py#n...
build/android/run_tests.py:265: 
nit: need another blank line here

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests_help...
File build/android/run_tests_helper.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests_help...
build/android/run_tests_helper.py:17: CHROME_DIR =
os.path.abspath(os.path.join(sys.path[0], '..', '..'))
suggestion: constants like these are present all over. It might be better to
move them to a common file

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests_help...
build/android/run_tests_helper.py:128: base_url = ('127.0.0.1:%d' % device_port)
remove outer parens

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
File build/android/single_test_runner.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:13: from test_result import *
do not import *

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:60: # TODO(yfriedman): Disabled because of
flakiness.
Pleawe reference the bug (or file one if necessary)

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:61: #(self.test_package.test_suite_basename
== 'unit_tests' or
need a blank space after #

Or remove unused code

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:75: filter_name = ('%s%s') %
(self.test_package.test_suite_basename,
remove parens around '%s%s'

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:130: """)
indent 4 spaces wrt previous line

[Paweł Hajdan Jr., 2011-10-21 09:12:58]

Drive-by just to let you know about _known_ problems that have _happened_ (and
are now fixed in Chromium codebase) that this CL seems to repeat.

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_shar...
File build/android/base_test_sharder.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_shar...
build/android/base_test_sharder.py:27: class BaseTestSharder(object):
We have a sharding supervisor written by Charles Lee (Gavin Peters should know
more details). Please try to avoid adding duplicate piece of infrastructure
here.

It seems to me that Charles' supervisor handles more edge cases than this one.

http://codereview.chromium.org/8364020/diff/2001/build/android/lighttpd_serve...
File build/android/lighttpd_server.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/lighttpd_serve...
build/android/lighttpd_server.py:112: def _TestServerConnection(self):
This seems super-flaky. Can you maybe use the net/test/test_server that has a
more reliable way to wait?

[michaelbai, 2011-10-21 21:08:41]

Fixed the most of the common issue I could found.

Remove the base_test_sharder from this CL, will try to upstream again when find
more information

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
File build/android/base_test_runner.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
build/android/base_test_runner.py:5: 
On 2011/10/21 08:16:15, Nirnimesh wrote:
> nit: only one blank line needed here.

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
build/android/base_test_runner.py:26: Args:
On 2011/10/21 08:16:15, Nirnimesh wrote:
> this should go in __init__

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_runn...
build/android/base_test_runner.py:31: self.device = device
On 2011/10/21 08:16:15, Nirnimesh wrote:
> I don't see self.device being used anywhere else.

It is used in subclass like single_tests_runner

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_shar...
File build/android/base_test_sharder.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/base_test_shar...
build/android/base_test_sharder.py:27: class BaseTestSharder(object):
I didn't get the reply from him yet. I removed this file from CL as it is not
necessary at this stage.

On 2011/10/21 09:12:58, Paweł Hajdan Jr. wrote:
> We have a sharding supervisor written by Charles Lee (Gavin Peters should know
> more details). Please try to avoid adding duplicate piece of infrastructure
> here.
> 
> It seems to me that Charles' supervisor handles more edge cases than this one.

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
File build/android/chrome_test_server_spawner.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
build/android/chrome_test_server_spawner.py:19: # Path that are needed to import
testserver
On 2011/10/21 08:16:15, Nirnimesh wrote:
> cr_src = os.path.join(os.path.abspath(os.path.dirname(__file__)), '..',
>   21    '..')
> sys.path.append(os.path.join(cr_src, 'third_party')
> sys.path.append(os.path.join(cr_src, 'third_party', 'tlslite')
> ...
> ...

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
build/android/chrome_test_server_spawner.py:30: _test_servers = []
On 2011/10/21 08:16:15, Nirnimesh wrote:
> why global?

It seemed that it used to grantee only one running server instance.

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
build/android/chrome_test_server_spawner.py:32: class
SpawningServerRequestHandler(BaseHTTPServer.BaseHTTPRequestHandler):
On 2011/10/21 08:16:15, Nirnimesh wrote:
> docstring please

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/chrome_test_se...
build/android/chrome_test_server_spawner.py:85: print 'Listening on %d, type %d,
data_dir %s' % (options.port,
On 2011/10/21 08:16:15, Nirnimesh wrote:
> logging.info()?

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.py
File build/android/flag_changer.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.p...
build/android/flag_changer.py:25: if flags: assert flags[0] != 'chrome'
On 2011/10/21 08:16:15, Nirnimesh wrote:
> move "assert flags[0] != 'chrome'" to next line

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.p...
build/android/flag_changer.py:29: if self.old_flags: self.old_flags =
self.old_flags[0].strip()
On 2011/10/21 08:16:15, Nirnimesh wrote:
> same here

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.p...
build/android/flag_changer.py:32: # Avoid appending flags that already present.
On 2011/10/21 08:16:15, Nirnimesh wrote:
> that -> that are

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/flag_changer.p...
build/android/flag_changer.py:41: def Restore(self):
On 2011/10/21 08:16:15, Nirnimesh wrote:
> I'd recommend calling Restore in the d'tor as well, so that the caller does
not
> have to.

I checked all the code using FlagChanger, it seemed that Restore when instance
gone is not the typical use case. I'd not change it to avoid tricks.

http://codereview.chromium.org/8364020/diff/2001/build/android/gtest_filter/b...
File build/android/gtest_filter/base_unittests_disabled (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/gtest_filter/b...
build/android/gtest_filter/base_unittests_disabled:3: # Automatically generated
by run_tests.py
On 2011/10/21 08:16:15, Nirnimesh wrote:
> Automatically generated? Why check in then?

The test script will not generate this file by default. It used to exclude the
testcases only failed in Android and can be fixed later.

http://codereview.chromium.org/8364020/diff/2001/build/android/lighttpd_serve...
File build/android/lighttpd_server.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/lighttpd_serve...
build/android/lighttpd_server.py:112: def _TestServerConnection(self):
On 2011/10/21 09:12:58, Paweł Hajdan Jr. wrote:
> This seems super-flaky. Can you maybe use the net/test/test_server that has a
> more reliable way to wait?

I am not the author,added TODO and see if it a problem in bot.

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests.py
File build/android/run_tests.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests.py#n...
build/android/run_tests.py:58: from base_test_sharder import *
On 2011/10/21 08:16:15, Nirnimesh wrote:
> do not import *

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests.py#n...
build/android/run_tests.py:68: ]
On 2011/10/21 08:16:15, Nirnimesh wrote:
> align under _

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests.py#n...
build/android/run_tests.py:265: 
On 2011/10/21 08:16:15, Nirnimesh wrote:
> nit: need another blank line here

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests_help...
File build/android/run_tests_helper.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests_help...
build/android/run_tests_helper.py:17: CHROME_DIR =
os.path.abspath(os.path.join(sys.path[0], '..', '..'))
On 2011/10/21 08:16:15, Nirnimesh wrote:
> suggestion: constants like these are present all over. It might be better to
> move them to a common file

Add a TODO

http://codereview.chromium.org/8364020/diff/2001/build/android/run_tests_help...
build/android/run_tests_helper.py:128: base_url = ('127.0.0.1:%d' % device_port)
On 2011/10/21 08:16:15, Nirnimesh wrote:
> remove outer parens

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
File build/android/single_test_runner.py (right):

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:13: from test_result import *
On 2011/10/21 08:16:15, Nirnimesh wrote:
> do not import *

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:60: # TODO(yfriedman): Disabled because of
flakiness.
On 2011/10/21 08:16:15, Nirnimesh wrote:
> Pleawe reference the bug (or file one if necessary)

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:61: #(self.test_package.test_suite_basename
== 'unit_tests' or
On 2011/10/21 08:16:15, Nirnimesh wrote:
> need a blank space after #
> 
> Or remove unused code

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:75: filter_name = ('%s%s') %
(self.test_package.test_suite_basename,
On 2011/10/21 08:16:15, Nirnimesh wrote:
> remove parens around '%s%s'

Done.

http://codereview.chromium.org/8364020/diff/2001/build/android/single_test_ru...
build/android/single_test_runner.py:130: """)
On 2011/10/21 08:16:15, Nirnimesh wrote:
> indent 4 spaces wrt previous line

Done.

[joth, 2011-10-24 10:10:27]

http://codereview.chromium.org/8364020/diff/6001/build/android/lighttpd_serve...
File build/android/lighttpd_server.py (right):

http://codereview.chromium.org/8364020/diff/6001/build/android/lighttpd_serve...
build/android/lighttpd_server.py:116: #
http://codereview.chromium.org/8364020/diff/2001/build/android/lighttpd_serve...
This server has never shown any flakiness to date - at least, when used with the
option of automatically find a free port as we always do. This test connection
method is an important part of ensuring its reliability.


test_server.py is complex to use when the server and client are running on
different host / target devices, due to the domain socket connection between
test code and test server. 
Any flakiness seen has been in the device to host port forwarding, which is
common to both approaches.

[michaelbai, 2011-10-24 17:44:50]

Hi Nirnimesh,

PTAL

[Nirnimesh, 2011-10-24 18:02:34]

LGTM

http://codereview.chromium.org/8364020/diff/6001/build/android/base_test_runn...
File build/android/base_test_runner.py (right):

http://codereview.chromium.org/8364020/diff/6001/build/android/base_test_runn...
build/android/base_test_runner.py:42: self._forwarder_base_url =
'http://localhost:%d' % self._forwarder_device_port
80+ chars

http://codereview.chromium.org/8364020/diff/6001/build/android/flag_changer.py
File build/android/flag_changer.py (right):

http://codereview.chromium.org/8364020/diff/6001/build/android/flag_changer.p...
build/android/flag_changer.py:37: self._old_flags + ' ' +
indent by 1 more space to the right

http://codereview.chromium.org/8364020/diff/6001/build/android/flag_changer.p...
build/android/flag_changer.py:38: ' '.join(new_flags))
same here

http://codereview.chromium.org/8364020/diff/6001/build/android/flag_changer.p...
build/android/flag_changer.py:41: 'chrome ' + ' '.join(flags))
and here

http://codereview.chromium.org/8364020/diff/6001/build/android/test_result.py
File build/android/test_result.py (right):

http://codereview.chromium.org/8364020/diff/6001/build/android/test_result.py...
build/android/test_result.py:88: def GetAllBroken(self):
docstring please

http://codereview.chromium.org/8364020/diff/6001/build/android/test_result.py...
build/android/test_result.py:91: def LogFull(self):
docstring please

[michaelbai, 2011-10-24 18:33:51]

http://codereview.chromium.org/8364020/diff/6001/build/android/base_test_runn...
File build/android/base_test_runner.py (right):

http://codereview.chromium.org/8364020/diff/6001/build/android/base_test_runn...
build/android/base_test_runner.py:42: self._forwarder_base_url =
'http://localhost:%d' % self._forwarder_device_port
On 2011/10/24 18:02:34, Nirnimesh wrote:
> 80+ chars

Done.

http://codereview.chromium.org/8364020/diff/6001/build/android/flag_changer.py
File build/android/flag_changer.py (right):

http://codereview.chromium.org/8364020/diff/6001/build/android/flag_changer.p...
build/android/flag_changer.py:37: self._old_flags + ' ' +
On 2011/10/24 18:02:34, Nirnimesh wrote:
> indent by 1 more space to the right

Done.

http://codereview.chromium.org/8364020/diff/6001/build/android/flag_changer.p...
build/android/flag_changer.py:38: ' '.join(new_flags))
On 2011/10/24 18:02:34, Nirnimesh wrote:
> same here

Done.

http://codereview.chromium.org/8364020/diff/6001/build/android/flag_changer.p...
build/android/flag_changer.py:41: 'chrome ' + ' '.join(flags))
On 2011/10/24 18:02:34, Nirnimesh wrote:
> and here

Done.

http://codereview.chromium.org/8364020/diff/6001/build/android/lighttpd_serve...
File build/android/lighttpd_server.py (right):

http://codereview.chromium.org/8364020/diff/6001/build/android/lighttpd_serve...
build/android/lighttpd_server.py:116: #
http://codereview.chromium.org/8364020/diff/2001/build/android/lighttpd_serve...
On 2011/10/24 10:10:27, joth wrote:
> This server has never shown any flakiness to date - at least, when used with
the
> option of automatically find a free port as we always do. This test connection
> method is an important part of ensuring its reliability.
> 
> 
> test_server.py is complex to use when the server and client are running on
> different host / target devices, due to the domain socket connection between
> test code and test server. 
> Any flakiness seen has been in the device to host port forwarding, which is
> common to both approaches.


Great! Remove the TODO.

http://codereview.chromium.org/8364020/diff/6001/build/android/test_result.py
File build/android/test_result.py (right):

http://codereview.chromium.org/8364020/diff/6001/build/android/test_result.py...
build/android/test_result.py:88: def GetAllBroken(self):
On 2011/10/24 18:02:34, Nirnimesh wrote:
> docstring please

Done.

http://codereview.chromium.org/8364020/diff/6001/build/android/test_result.py...
build/android/test_result.py:91: def LogFull(self):
On 2011/10/24 18:02:34, Nirnimesh wrote:
> docstring please

Done.