GTTF: Move core parts of test_launcher down to base.

GTTF: Move core parts of test_launcher down to base.

BUG=236893
R=jam@chromium.org

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

[jam, 2013-06-13 20:43:31]

now that you have a separate delegate, https://codereview.chromium.org/15808008/
can be reverted. please do that first or as part of this cl

https://codereview.chromium.org/16820008/diff/1/base/test/test_launcher.h
File base/test/test_launcher.h (right):

https://codereview.chromium.org/16820008/diff/1/base/test/test_launcher.h#new...
base/test/test_launcher.h:24: class TestLauncherDelegate {
need to document this class, the methods, and the method below the class

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
File content/public/test/test_launcher.cc (left):

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
content/public/test/test_launcher.cc:708: bool warmup =
command_line->HasSwitch(kWarmupFlag);
where did the warmup/empty test code go?

please enumerate any other changes to the moved code

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
content/public/test/test_launcher.cc:219: class WrapperTestLauncherDelegate :
public base::TestLauncherDelegate {
need documentation

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
content/public/test/test_launcher.cc:221:
WrapperTestLauncherDelegate(content::TestLauncherDelegate* launcher_delegate)
nit: here and below, no "content::"

explicit

[Paweł Hajdan Jr., 2013-06-13 22:13:05]

John, thank you for review, your time and patience here. I appreciate your
ongoing feedback about this series of CLs (even if I don't always fully agree
with it or just ask questions).

Here is a question. :) Why would you like me to revert
https://codereview.chromium.org/15808008/ ? I actually think it's a more correct
way to handle this (one step closer to remove GetCurrentTestLauncherDelegate,
which I think is rather ugly) and would prefer that change to stay.

https://codereview.chromium.org/16820008/diff/1/base/test/test_launcher.h
File base/test/test_launcher.h (right):

https://codereview.chromium.org/16820008/diff/1/base/test/test_launcher.h#new...
base/test/test_launcher.h:24: class TestLauncherDelegate {
On 2013/06/13 20:43:31, jam wrote:
> need to document this class, the methods, and the method below the class

Done. I was not sure if you'd accept the approach. Looks like this patch has a
chance, so I'm going to give it more polish. :)

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
File content/public/test/test_launcher.cc (left):

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
content/public/test/test_launcher.cc:708: bool warmup =
command_line->HasSwitch(kWarmupFlag);
On 2013/06/13 20:43:31, jam wrote:
> where did the warmup/empty test code go?

Empty test code is still in the same place - I've used cs.chromium.org to
determine that --warmup flag is not used.

> please enumerate any other changes to the moved code

One change to note is the new version is not counting timeouts. I can restore it
if you have a strong opinion about it. I was opposing the introduction of
counting timeouts in its current form, and I think removing it is the right
thing (my future changes in this area will address this better).

There is also a slight change in behavior of the empty test handling
(ShouldShard is not checked), done for simplicity of the interface. If you have
a strong opinion, I can add it back (headers in base/test would change) - I
think it's the right thing to do though.

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
content/public/test/test_launcher.cc:219: class WrapperTestLauncherDelegate :
public base::TestLauncherDelegate {
On 2013/06/13 20:43:31, jam wrote:
> need documentation

Done.

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
content/public/test/test_launcher.cc:221:
WrapperTestLauncherDelegate(content::TestLauncherDelegate* launcher_delegate)
On 2013/06/13 20:43:31, jam wrote:
> nit: here and below, no "content::"

Here is the error I get when I do that:

../../content/public/test/test_launcher.cc: In member function ‘virtual bool
content::{anonymous}::WrapperTestLauncherDelegate::ShouldRunTest(const
testing::TestCase*, const testing::TestInfo*)’:
../../content/public/test/test_launcher.cc:246:40: error: ‘class
base::TestLauncherDelegate’ has no member named ‘GetEmptyTestName’
../../content/public/test/test_launcher.cc: In member function ‘virtual bool
content::{anonymous}::WrapperTestLauncherDelegate::RunTest(const
testing::TestCase*, const testing::TestInfo*)’:
../../content/public/test/test_launcher.cc:268:33: error: cannot convert
‘base::TestLauncherDelegate*’ to ‘content::TestLauncherDelegate*’ for argument
‘1’ to ‘int content::{anonymous}::DoRunTest(content::TestLauncherDelegate*,
const testing::TestCase*, const string&, base::TimeDelta, bool*)’
../../content/public/test/test_launcher.cc: In function ‘int
content::LaunchTests(content::TestLauncherDelegate*, int, char**)’:
../../content/public/test/test_launcher.cc:393:57: error: no matching function
for call to
‘content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(content::TestLauncherDelegate*&)’
../../content/public/test/test_launcher.cc:393:57: note: candidates are:
../../content/public/test/test_launcher.cc:236:3: note:
content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(const
content::{anonymous}::WrapperTestLauncherDelegate&)
../../content/public/test/test_launcher.cc:236:3: note:   no known conversion
for argument 1 from ‘content::TestLauncherDelegate*’ to ‘const
content::{anonymous}::WrapperTestLauncherDelegate&’
../../content/public/test/test_launcher.cc:223:3: note:
content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(base::TestLauncherDelegate*)
../../content/public/test/test_launcher.cc:223:3: note:   no known conversion
for argument 1 from ‘content::TestLauncherDelegate*’ to
‘base::TestLauncherDelegate*’

Given I'm no C++ expert, there could be something I don't see that would fix it
while not saying content:: explicitly.

Having content::TestLauncherDelegate and base::TestLauncherDelegate certainly
has potential to confuse things. I could rename them, e.g.
BaseTestLauncherDelegate or ContentTestLauncherDelegate, or something else.

> explicit

Done.

[jam, 2013-06-14 16:16:50]

On 2013/06/13 22:13:05, Paweł Hajdan Jr. wrote:
> Here is a question. :) Why would you like me to revert
> https://codereview.chromium.org/15808008/ ? I actually think it's a more
correct
> way to handle this (one step closer to remove GetCurrentTestLauncherDelegate,
> which I think is rather ugly) and would prefer that change to stay.

There are no plans (that I know of) to remove the usage of
GetCurrentTestLauncherDelegate in the chrome test suite. So until another
solution is done, it doesn't matter if it has one or two usages.

More importantly, this is a step backward because now you're allowing
AddPreRunMessageLoopHook and AddPostRunMessageLoopHook to be called by other
code. That will make removing them harder.

Finally, https://codereview.chromium.org/15808008/ was done because you wanted
to move the delegate to base. That is not happening any more. So there's no
reason to have done that change (and I had already expressed my desire to not
move the interface to base before you did that change, but you sent it to
another reviewer).

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
File content/public/test/test_launcher.cc (left):

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
content/public/test/test_launcher.cc:708: bool warmup =
command_line->HasSwitch(kWarmupFlag);
On 2013/06/13 22:13:06, Paweł Hajdan Jr. wrote:
> On 2013/06/13 20:43:31, jam wrote:
> > where did the warmup/empty test code go?
> 
> Empty test code is still in the same place - I've used http://cs.chromium.org
to
> determine that --warmup flag is not used.

--warmup is in sharding_supervisor_old.py. afaik we don't use that file anymore
because we use run_test_cases.py.

That means that we don't do the warmup path anymore? i.e. because on the bots
that we added this to, they all do sharding. So that second code path to run the
empty test isn't run.

I wonder if we suddenly don't need the empty test case optimization anymore?
what changed?

> 
> > please enumerate any other changes to the moved code
> 
> One change to note is the new version is not counting timeouts. I can restore
it
> if you have a strong opinion about it.

You should separate refactoring from behavior change. Not counting timeout is
not a trivial thing, and in the future, please call this out in the cl
description as it's hard to see this when reviewing large chunks of moved code.

> I was opposing the introduction of
> counting timeouts in its current form, and I think removing it is the right
> thing (my future changes in this area will address this better).

Until there's another way of addressing this issue, the timeout counting code
seems needed. Otherwise someone who sends a change which causes chrome to hang
will take a long time to run on all try bots.

> 
> There is also a slight change in behavior of the empty test handling
> (ShouldShard is not checked), done for simplicity of the interface. 

From the comment of the code that you deleted:
"and also on trybots when sharding as this one test runs ~200 times and wastes a
few minutes."

Making test runs a few minutes slower is a regression.

> If you have
> a strong opinion, I can add it back (headers in base/test would change) - I
> think it's the right thing to do though.

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/16820008/diff/1/content/public/test/test_laun...
content/public/test/test_launcher.cc:221:
WrapperTestLauncherDelegate(content::TestLauncherDelegate* launcher_delegate)
On 2013/06/13 22:13:06, Paweł Hajdan Jr. wrote:
> On 2013/06/13 20:43:31, jam wrote:
> > nit: here and below, no "content::"
> 
> Here is the error I get when I do that:
> 
> ../../content/public/test/test_launcher.cc: In member function ‘virtual bool
> content::{anonymous}::WrapperTestLauncherDelegate::ShouldRunTest(const
> testing::TestCase*, const testing::TestInfo*)’:
> ../../content/public/test/test_launcher.cc:246:40: error: ‘class
> base::TestLauncherDelegate’ has no member named ‘GetEmptyTestName’
> ../../content/public/test/test_launcher.cc: In member function ‘virtual bool
> content::{anonymous}::WrapperTestLauncherDelegate::RunTest(const
> testing::TestCase*, const testing::TestInfo*)’:
> ../../content/public/test/test_launcher.cc:268:33: error: cannot convert
> ‘base::TestLauncherDelegate*’ to ‘content::TestLauncherDelegate*’ for argument
> ‘1’ to ‘int content::{anonymous}::DoRunTest(content::TestLauncherDelegate*,
> const testing::TestCase*, const string&, base::TimeDelta, bool*)’
> ../../content/public/test/test_launcher.cc: In function ‘int
> content::LaunchTests(content::TestLauncherDelegate*, int, char**)’:
> ../../content/public/test/test_launcher.cc:393:57: error: no matching function
> for call to
>
‘content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(content::TestLauncherDelegate*&)’
> ../../content/public/test/test_launcher.cc:393:57: note: candidates are:
> ../../content/public/test/test_launcher.cc:236:3: note:
>
content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(const
> content::{anonymous}::WrapperTestLauncherDelegate&)
> ../../content/public/test/test_launcher.cc:236:3: note:   no known conversion
> for argument 1 from ‘content::TestLauncherDelegate*’ to ‘const
> content::{anonymous}::WrapperTestLauncherDelegate&’
> ../../content/public/test/test_launcher.cc:223:3: note:
>
content::{anonymous}::WrapperTestLauncherDelegate::WrapperTestLauncherDelegate(base::TestLauncherDelegate*)
> ../../content/public/test/test_launcher.cc:223:3: note:   no known conversion
> for argument 1 from ‘content::TestLauncherDelegate*’ to
> ‘base::TestLauncherDelegate*’
> 
> Given I'm no C++ expert, there could be something I don't see that would fix
it
> while not saying content:: explicitly.
> 
> Having content::TestLauncherDelegate and base::TestLauncherDelegate certainly
> has potential to confuse things. I could rename them, e.g.
> BaseTestLauncherDelegate or ContentTestLauncherDelegate, or something else.

ah I see, it's because it derives from a parent class that's also called that.
usually you don't need to specify the namespace of a parent class when referring
to it.

ignore my comment then. I think it would be redundant to have base::Base and
content::Content
> 
> > explicit
> 
> Done.

[Paweł Hajdan Jr., 2013-06-14 18:11:21]

This should now have no behavior changes compared to the previous version.

I reverted the change you've asked for as
https://codereview.chromium.org/17084007/ .

PTAL

[jam, 2013-06-14 23:12:19]

https://codereview.chromium.org/16820008/diff/22001/content/public/test/test_...
File content/public/test/test_launcher.cc (left):

https://codereview.chromium.org/16820008/diff/22001/content/public/test/test_...
content/public/test/test_launcher.cc:612: const char kWarmupFlag[] = "warmup";
you should update sharding_supervisor_old.py since this flag is gone now

My previous question is still unanswered:

"That means that we don't do the warmup path anymore? i.e. because on the bots
that we added this to, they all do sharding. So that second code path to run the
empty test isn't run.

I wonder if we suddenly don't need the empty test case optimization anymore?
what changed?"

if we're not actually using this, then we don't need the empty test case concept
anymore.

[Paweł Hajdan Jr., 2013-06-17 20:30:30]

https://codereview.chromium.org/16820008/diff/22001/content/public/test/test_...
File content/public/test/test_launcher.cc (left):

https://codereview.chromium.org/16820008/diff/22001/content/public/test/test_...
content/public/test/test_launcher.cc:612: const char kWarmupFlag[] = "warmup";
On 2013/06/14 23:12:20, jam wrote:
> you should update sharding_supervisor_old.py since this flag is gone now

Done: https://codereview.chromium.org/17305002

> My previous question is still unanswered:
> 
> "That means that we don't do the warmup path anymore? i.e. because on the bots
> that we added this to, they all do sharding. So that second code path to run
the
> empty test isn't run.
> 
> I wonder if we suddenly don't need the empty test case optimization anymore?
> what changed?"
> 
> if we're not actually using this, then we don't need the empty test case
concept
> anymore.

I think not all tests on the waterfall use run_test_cases.py, and thus I prefer
to keep this path.

If you really want to get it removed, I can do that.

[jam, 2013-06-17 21:41:46]

On 2013/06/17 20:30:30, Paweł Hajdan Jr. wrote:
>
https://codereview.chromium.org/16820008/diff/22001/content/public/test/test_...
> File content/public/test/test_launcher.cc (left):
> 
>
https://codereview.chromium.org/16820008/diff/22001/content/public/test/test_...
> content/public/test/test_launcher.cc:612: const char kWarmupFlag[] = "warmup";
> On 2013/06/14 23:12:20, jam wrote:
> > you should update sharding_supervisor_old.py since this flag is gone now
> 
> Done: https://codereview.chromium.org/17305002
> 
> > My previous question is still unanswered:
> > 
> > "That means that we don't do the warmup path anymore? i.e. because on the
bots
> > that we added this to, they all do sharding. So that second code path to run
> the
> > empty test isn't run.
> > 
> > I wonder if we suddenly don't need the empty test case optimization anymore?
> > what changed?"
> > 
> > if we're not actually using this, then we don't need the empty test case
> concept
> > anymore.
> 
> I think not all tests on the waterfall use run_test_cases.py, and thus I
prefer
> to keep this path.

which waterfall builders run browser tests without sharding? as I'm sure you
know, any bot that calls to sharding_supervisor.py is redirected to
run_test_cases

> 
> If you really want to get it removed, I can do that.

[Paweł Hajdan Jr., 2013-06-17 23:21:41]

On 2013/06/17 21:41:46, jam wrote:
> which waterfall builders run browser tests without sharding? as I'm sure you
> know, any bot that calls to sharding_supervisor.py is redirected to
> run_test_cases

Just one example is interactive_ui_tests on chromium.win waterfall:

http://build.chromium.org/p/chromium.win/builders/Interactive%20Tests%20%28db...
http://chromium-build-logs.appspot.com/list?buildbot_root=&builder=Interactiv...

[jam, 2013-06-18 00:11:19]

On 2013/06/17 23:21:41, Paweł Hajdan Jr. wrote:
> On 2013/06/17 21:41:46, jam wrote:
> > which waterfall builders run browser tests without sharding? as I'm sure you
> > know, any bot that calls to sharding_supervisor.py is redirected to
> > run_test_cases
> 
> Just one example is interactive_ui_tests on chromium.win waterfall:
> 
>
http://build.chromium.org/p/chromium.win/builders/Interactive%2520Tests%2520%...
>
http://chromium-build-logs.appspot.com/list?buildbot_root=&builder=Interactiv...

sure, by definition interactive_ui_tests isn't sharded.

what i was trying to get to is if browser_tests doesn't need the empty test case
anymore, there's no reason to believe that interactive would. so we can delete
this code path.

https://codereview.chromium.org/16820008/diff/28001/content/public/test/test_...
File content/public/test/test_launcher.cc (right):

https://codereview.chromium.org/16820008/diff/28001/content/public/test/test_...
content/public/test/test_launcher.cc:64: int DoRunTestInternal(const
testing::TestCase* test_case,
I just noticed that this code is still here. it seems that the bulk of this is
stuff that you'll need to move to base, right? otherwise it won't do sharding,
so there's no point in moving it to base.

can you outline how you'll move this code?

[Paweł Hajdan Jr., 2013-06-18 00:53:36]

On 2013/06/18 00:11:19, jam wrote:
> sure, by definition interactive_ui_tests isn't sharded.

That's why it was an obvious example. :) Note that interactive_ui_tests use the
code path from this CL.

> what i was trying to get to is if browser_tests doesn't need the empty test
case
> anymore, there's no reason to believe that interactive would. so we can delete
> this code path.

On GPU waterfall content_browsertests do not use sharding:

http://chromium-build-logs.appspot.com/list?buildbot_root=http%3A//build.chro...

Please let me know if you have more questions, or still prefer deleting the
code.

>
https://codereview.chromium.org/16820008/diff/28001/content/public/test/test_...
> File content/public/test/test_launcher.cc (right):
> 
>
https://codereview.chromium.org/16820008/diff/28001/content/public/test/test_...
> content/public/test/test_launcher.cc:64: int DoRunTestInternal(const
> testing::TestCase* test_case,
> I just noticed that this code is still here. it seems that the bulk of this is
> stuff that you'll need to move to base, right? otherwise it won't do sharding,
> so there's no point in moving it to base.
> 
> can you outline how you'll move this code?

First I'll need to implement running several tests in the launcher process
instead of one (too much overhead for smaller unit test targets). After that,
the core logic that launches the child test process would move to base, and the
delegate would decide which tests should run in the same process.

I'm happy to answer any of your questions - as noted before though, for many of
them answering them more precisely amounts to actually making the code do it, at
least locally. This would likely result in a huge CL, harder to review, or
dividing it into a series of dependent patches. Then there is a question of
making sure we can have different behavior for say base_unittests, and so for me
the best course of action seems to be to move the core logic to base and move
forward from there.

If you'd like to see something different please let me know.

[jam, 2013-06-18 23:29:53]

On 2013/06/18 00:53:36, Paweł Hajdan Jr. wrote:
> On 2013/06/18 00:11:19, jam wrote:
> > sure, by definition interactive_ui_tests isn't sharded.
> 
> That's why it was an obvious example. :) Note that interactive_ui_tests use
the
> code path from this CL.
> 
> > what i was trying to get to is if browser_tests doesn't need the empty test
> case
> > anymore, there's no reason to believe that interactive would. so we can
delete
> > this code path.
> 
> On GPU waterfall content_browsertests do not use sharding:

I was wondering about browser_tests. content_browsertests is much lighterweight,
both in binary size and in startup time, so it doesn't need this optimization.

> 
>
http://chromium-build-logs.appspot.com/list?buildbot_root=http%253A//build.ch...
> 
> Please let me know if you have more questions, or still prefer deleting the
> code.
> 
> >
>
https://codereview.chromium.org/16820008/diff/28001/content/public/test/test_...
> > File content/public/test/test_launcher.cc (right):
> > 
> >
>
https://codereview.chromium.org/16820008/diff/28001/content/public/test/test_...
> > content/public/test/test_launcher.cc:64: int DoRunTestInternal(const
> > testing::TestCase* test_case,
> > I just noticed that this code is still here. it seems that the bulk of this
is
> > stuff that you'll need to move to base, right? otherwise it won't do
sharding,
> > so there's no point in moving it to base.
> > 
> > can you outline how you'll move this code?
> 
> First I'll need to implement running several tests in the launcher process
> instead of one (too much overhead for smaller unit test targets).

run_test_cases.py currently does not do that, correct? Do you have an estimate
on what the time difference is, before doing this? i.e. how much of a slow down
happened when we switched to run_test_cases?


> After that,
> the core logic that launches the child test process would move to base, and
the
> delegate would decide which tests should run in the same process.

can you share more information on how this will be done? i.e. i assume this is
only for unit tests targets, and not browser tests. so how will unit test runner
pick which tests to group?

> 
> I'm happy to answer any of your questions - as noted before though, for many
of
> them answering them more precisely amounts to actually making the code do it,
at
> least locally.

I don't think one needs to fully code it, just have a plan on how this will be
done. I don't want to land refactorings of this code without understanding the
plan, because if it doesn't work out then the code will be in limbo.

> This would likely result in a huge CL, harder to review, or
> dividing it into a series of dependent patches. Then there is a question of
> making sure we can have different behavior for say base_unittests, and so for
me
> the best course of action seems to be to move the core logic to base and move
> forward from there.
> 
> If you'd like to see something different please let me know.

[Paweł Hajdan Jr., 2013-06-19 17:56:28]

On 2013/06/18 23:29:53, jam wrote:
> I was wondering about browser_tests. content_browsertests is much
lighterweight,
> both in binary size and in startup time, so it doesn't need this optimization.

Got it - actually indeed it was not using the empty test.

I've checked and removed the empty test code then.

> > First I'll need to implement running several tests in the launcher process
> > instead of one (too much overhead for smaller unit test targets).
> 
> run_test_cases.py currently does not do that, correct?

It does. I've talked to M-A about this and the C++ test launcher will have to
support this functionality (or at the very least be fast for small unit tests).

> Do you have an estimate on what the time difference is, before doing this?
i.e. how much of a slow down
> happened when we switched to run_test_cases?

I think it did not make anything slower.

Note that running say base_unittests with every test in its own process is
prohibitively slow on Windows. I've tried it.

> > After that,
> > the core logic that launches the child test process would move to base, and
> the
> > delegate would decide which tests should run in the same process.
> 
> can you share more information on how this will be done? i.e. i assume this is
> only for unit tests targets, and not browser tests. so how will unit test
runner
> pick which tests to group?

Yes, this is not for browser tests which will continue to have one process per
test.

I plan to implement this by using gtest_filter and a constant for batch size.

> I don't think one needs to fully code it, just have a plan on how this will be
> done. I don't want to land refactorings of this code without understanding the
> plan, because if it doesn't work out then the code will be in limbo.

The code will not be in limbo in any case - could you explain more where that
concern comes from?

[jam, 2013-06-20 00:38:00]

lgtm

[Paweł Hajdan Jr., 2013-06-20 23:07:37]

Committed patchset #7 manually as r207629.