GTTF: Add a basic new unit test launcher.

GTTF: Add a basic new unit test launcher.

It launches batches of unit tests in a subprocess for better stability.

BUG=236893
R=maruel@chromium.org, satorux@chromium.org

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

[Paweł Hajdan Jr., 2013-08-13 22:43:27]

Scott, what do you think? This is a bit experimental and behind a flag. I'm
mostly re-implementing some run_test_cases.py features here (and eventually the
launcher will replace run_test_cases.py).

[M-A Ruel, 2013-08-14 18:42:41]

https://codereview.chromium.org/23132002/diff/4001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/23132002/diff/4001/base/base.gyp#newcode829
base/base.gyp:829: 'target_name': 'test_support_base',
It's interesting that test_support_base is in base and not in a (potentially
new) base_test.gyp. Just saying, not really important.

https://codereview.chromium.org/23132002/diff/4001/base/test/gtest_xml_util.h
File base/test/gtest_xml_util.h (right):

https://codereview.chromium.org/23132002/diff/4001/base/test/gtest_xml_util.h...
base/test/gtest_xml_util.h:18: // Returns true iff the output file exists and
has been successfully parsed.
One thing;
you may want to batches multiple test cases in a single process invocation.
That's what run_test_cases.py does. The reason is that for fast test cases, ones
< 10ms, the process creation and tear down will be well over the actual test
case execution time.

But on the other hand, using aggregation means that if you process the result
synchronously, the driver doesn't know the first test case result until all the
test cases passed, which may be significant for slower test cases.

So this means you want an active pipe, not really using an on-disk file.

[Paweł Hajdan Jr., 2013-08-14 18:53:40]

https://codereview.chromium.org/23132002/diff/4001/base/base.gyp
File base/base.gyp (right):

https://codereview.chromium.org/23132002/diff/4001/base/base.gyp#newcode829
base/base.gyp:829: 'target_name': 'test_support_base',
On 2013/08/14 18:42:41, M-A Ruel wrote:
> It's interesting that test_support_base is in base and not in a (potentially
> new) base_test.gyp. Just saying, not really important.

Agreed - I'll think about extracting test-specific gyp out of this after asking
base OWNERS.

https://codereview.chromium.org/23132002/diff/4001/base/test/gtest_xml_util.h
File base/test/gtest_xml_util.h (right):

https://codereview.chromium.org/23132002/diff/4001/base/test/gtest_xml_util.h...
base/test/gtest_xml_util.h:18: // Returns true iff the output file exists and
has been successfully parsed.
On 2013/08/14 18:42:41, M-A Ruel wrote:
> One thing;
> you may want to batches multiple test cases in a single process invocation.
> That's what run_test_cases.py does. The reason is that for fast test cases,
ones
> < 10ms, the process creation and tear down will be well over the actual test
> case execution time.

Totally agreed. This is what this launcher does (see kTestBatchLimit in
unit_test_launcher.cc).

> But on the other hand, using aggregation means that if you process the result
> synchronously, the driver doesn't know the first test case result until all
the
> test cases passed, which may be significant for slower test cases.

Right. Yet, we still need to wait for the child process to finish (with a
timeout) before the launcher has finished.

> So this means you want an active pipe, not really using an on-disk file.

Well, gtest has built-in file support bot not cross-platform pipe (it has a
pipe-sending listener but only for Linux). For simplicity I'm starting with a
solution that works - it can be optimized and changed later if this turns out to
be a serious problem. I'll be encouraging developer to test the new launcher and
report what they think.

One design decision I'm very consciously making in this launcher is to strongly
favor reliability and simplicity. IMHO it's a fair tradeoff here since we're
waiting for the child process anyway.

[M-A Ruel, 2013-08-15 01:59:14]

lgtm with optional nits

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
File base/test/unit_test_launcher.cc (right):

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:31: const char kSingleProcessTestsFlag[] =
"single-process-tests";
I feel this flag could be easily confused with --single-process but I don't have
a better idea so it's fine.

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:50:
new_cmd_line.AppendSwitchNative((*iter).first, (*iter).second);
why not iter->first, iter->second?

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:97: // Create a dedicated temporary directory
for each run to ensure clean
// Create a dedicated temporary directory to store the xml result data per run
to ensure clean state and ...

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:101: // TODO(phajdan.jr): Handle the failure
gracefully.
I don't think it's worth, at that point it could blow up anytime. I'd remove the
comment.

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:113: // (note that e.g. the last batch of tests
will be smaller).
// TODO(phajdan.jr): Use timeout adaptative based on the stream of test case
result.

I found in practice using that is a severe, a batch of 100 test cases will
result in a ridiculously high timeout. It's fine to fix it later, just noting a
TODO could be appropriate.

https://codereview.chromium.org/23132002/diff/4001/content/public/test/test_l...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/23132002/diff/4001/content/public/test/test_l...
content/public/test/test_launcher.cc:98:
new_cmd_line.AppendSwitchNative((*iter).first, (*iter).second);
same interogation

[Paweł Hajdan Jr., 2013-08-15 21:54:45]

Darin, what do you think about changes to third_party/libxml/chromium (just
adding a method to test for a closing element)?

Feel free to also look at simple base/base.gyp changes (I'm adding dependency on
libxml.gyp to a test support target).

I'll be doing further work to avoid a circular gyp file reference induced by
that, but I'd also like to make sure remaining parts of the CL would be OK.

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
File base/test/unit_test_launcher.cc (right):

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:31: const char kSingleProcessTestsFlag[] =
"single-process-tests";
On 2013/08/15 01:59:14, M-A Ruel wrote:
> I feel this flag could be easily confused with --single-process but I don't
have
> a better idea so it's fine.

Right, I'm trying to at least improve things over --single_process and
--single-process.

Furthermore, for most unit tests the idea of --single-process doesn't apply
anyway. Tests where these matters are mostly browser tests which don't use this
launcher.

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:50:
new_cmd_line.AppendSwitchNative((*iter).first, (*iter).second);
On 2013/08/15 01:59:14, M-A Ruel wrote:
> why not iter->first, iter->second?

Done.

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:97: // Create a dedicated temporary directory
for each run to ensure clean
On 2013/08/15 01:59:14, M-A Ruel wrote:
> // Create a dedicated temporary directory to store the xml result data per run
> to ensure clean state and ...

Done.

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:101: // TODO(phajdan.jr): Handle the failure
gracefully.
On 2013/08/15 01:59:14, M-A Ruel wrote:
> I don't think it's worth, at that point it could blow up anytime. I'd remove
the
> comment.

Done.

https://codereview.chromium.org/23132002/diff/4001/base/test/unit_test_launch...
base/test/unit_test_launcher.cc:113: // (note that e.g. the last batch of tests
will be smaller).
On 2013/08/15 01:59:14, M-A Ruel wrote:
> // TODO(phajdan.jr): Use timeout adaptative based on the stream of test case
> result.
> 
> I found in practice using that is a severe, a batch of 100 test cases will
> result in a ridiculously high timeout. It's fine to fix it later, just noting
a
> TODO could be appropriate.

Done.

https://codereview.chromium.org/23132002/diff/4001/content/public/test/test_l...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/23132002/diff/4001/content/public/test/test_l...
content/public/test/test_launcher.cc:98:
new_cmd_line.AppendSwitchNative((*iter).first, (*iter).second);
On 2013/08/15 01:59:14, M-A Ruel wrote:
> same interogation

Done.

[sky, 2013-08-19 17:21:33]

Pawel, do you still need me to review this?

[Paweł Hajdan Jr., 2013-08-21 20:18:43]

Satoru, could you review changes to third_party/zlib/google? The goal is to
avoid a cyclic dependency between .gyp files, so I'm extracting a piece out of
zlib.gyp.

[satorux1, 2013-08-22 01:03:14]

third_party/zlib/google LGTM

[Paweł Hajdan Jr., 2013-08-22 19:14:40]

Committed patchset #5 manually as r219065.