PPAPI/NaCl: Make related tests run in 1 fixture

PPAPI/NaCl: Make related tests run in 1 fixture

I've only converted WebSockets tests in this CL. The idea is that the bulk of the test time is actually from starting up the test:
- Starting HTTP and/or WebSocket and/or SSL server
- Launching the renderer
- Launching the NaCl loader
- Downloading, validating, launching the .nexe

Now this all happens once for all WebSocket sub-tests (times 4: in-process, out-of-process, NaCl Newlib, NaCl Glibc). Locally, the time goes from about 5 minutes to less than 20 seconds.

The trick is now we can still enable/disable the individual tests from within ppapi_browsertest.cc.

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

[dmichael (off chromium), 2013-02-06 00:40:43]

Not the prettiest CL, sorry :(

https://codereview.chromium.org/12193015/diff/9001/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (left):

https://codereview.chromium.org/12193015/diff/9001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:74: }
These don't actually doing anything different anymore; they should have been
deleted a while ago.

https://codereview.chromium.org/12193015/diff/9001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:77: #define
TEST_PPAPI_NACL_VIA_HTTP(test_name)
VIA_HTTP is redundant for NaCl, so I shortened the names

[Nick Bray (chromium), 2013-02-06 00:52:01]

I am semi-concerned about this CL. To point out the obvious:

1) --gtest_filter can't select between the individual tests.  Meh.

2) The log parser will lump all these tests together.  Meh.

3) It will make the consolidated web socket test take longer than any individual
test - pushing it closer to time out.  (The irony.)  You might need to monkey
with the timeout for this mega test.

4) This is nonstandard and Chrome sheriffs will get confused and do stupid
things if they suspect flake - you may seem them TBR a review that disables all
the websocket tests.


All in all, it's different and different has a cost.  Is it worth it?

[bbudge, 2013-02-06 14:51:26]

https://codereview.chromium.org/12193015/diff/9001/chrome/test/ppapi/ppapi_br...
File chrome/test/ppapi/ppapi_browsertest.cc (right):

https://codereview.chromium.org/12193015/diff/9001/chrome/test/ppapi/ppapi_br...
chrome/test/ppapi/ppapi_browsertest.cc:25: // to turn names in to DISABLED_Foo,
but still pass a string to RunTest.
smallest nit ever:  s/in to/into

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/test_case.cc
File ppapi/tests/test_case.cc (right):

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/test_case.cc#n...
ppapi/tests/test_case.cc:19: std::string StripPrefixes(const std::string&
test_name) {
s/StripPrefixes/StripPrefix

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/test_case.cc#n...
ppapi/tests/test_case.cc:38: //   "Foo"
I had trouble understanding this documentation. Maybe replace "Foo" with
"TestCase"? Also, the last line should be "", right?

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/test_case.cc#n...
ppapi/tests/test_case.cc:55: // We can't use base/string_util.h::Tokenize in
ppapi. Instead, we'll
Did this comment get cut off?

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/test_case.cc#n...
ppapi/tests/test_case.cc:68: // We'll strip the case for use as a key, because
the test name
This is a little confusing to me. Maybe say "Strip off the test case and use the
test name as the key ..."

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/test_case.cc#n...
ppapi/tests/test_case.cc:179: bool TestCase::MatchesFilter(const std::string&
test_name,
nit: Maybe this method should be named "ShouldRunTest" now?

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/test_case.h
File ppapi/tests/test_case.h (right):

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/test_case.h#ne...
ppapi/tests/test_case.h:98: 
extra line

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/testing_instan...
File ppapi/tests/testing_instance.cc (right):

https://codereview.chromium.org/12193015/diff/9001/ppapi/tests/testing_instan...
ppapi/tests/testing_instance.cc:207: "tests in the TestCase. Forgotten tests:
");
s/Forgotten/Unknown

[dmichael (off chromium), 2013-02-06 17:27:29]

Yeah, I don't love it either, but I do think it's worth it. You're right on
all counts, but I'll rebut them anyway :)

On Tue, Feb 5, 2013 at 5:52 PM, <ncbray@chromium.org> wrote:

> I am semi-concerned about this CL. To point out the obvious:
>
> 1) --gtest_filter can't select between the individual tests.  Meh.
>
Yes, this sucks. We also can't use --gtest_also_run_disabled_tests.

We can mitigate it by adding our own flags, like --ppapi_filter or
--ppapi_also_run_disabled_tests. But, that's different and therefore sucks.
I don't have a good answer to this one except I think it's worth the
tradeoff.


>
> 2) The log parser will lump all these tests together.  Meh.
>
I don't think that will be a big problem in practice.


>
> 3) It will make the consolidated web socket test take longer than any
> individual
> test - pushing it closer to time out.  (The irony.)  You might need to
> monkey
> with the timeout for this mega test.
>
Marc-Antoine already upped the timeout to 120 seconds, so I think we're
okay here. I think we can agree that if we can make the total test time
much lower, then there's nothing wrong with increasing the timeout for a
single gtest test.

I also added logging of the elapsed time for each sub-test in this CL, so
we can tell which one is the long pole.


>
> 4) This is nonstandard and Chrome sheriffs will get confused and do stupid
> things if they suspect flake - you may seem them TBR a review that
> disables all
> the websocket tests.
>
Have a look at ppapi_browsertest.cc at the new WebSocket stuff if you
haven't. I tried to make it look as similar as I can; DISABLED and the
MAYBE pattern work as you would expect. So I think it's possible Sheriffs
can do it without hand-holding. But even if they can't, I don't mind them
turning off the whole test case. We'll get notified, and we can go whittle
it back down to only disable the parts that fail.


>
>
> All in all, it's different and different has a cost.  Is it worth it?
>
I just ran:
time out/Debug/browser_tests --gtest_filter="*PPAPI*WebSocket*"
With this change:
real 0m39.950s
user 0m10.960s
sys 0m1.570s

Without this change:
real 9m38.664s
user 3m7.040s
sys 0m31.080s



>
>
https://codereview.chromium.**org/12193015/<https://codereview.chromium.org/1...
>

-- 
You received this message because you are subscribed to the Google Groups
"Native-Client-Reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to native-client-reviews+unsubscribe@googlegroups.com.
To post to this group, send email to native-client-reviews@googlegroups.com.
Visit this group at http://groups.google.com/group/native-client-reviews?hl=en.
For more options, visit https://groups.google.com/groups/opt_out.

[dmichael (off chromium), 2013-02-06 17:27:30]

Yeah, I don't love it either, but I do think it's worth it. You're right on
all counts, but I'll rebut them anyway :)

On Tue, Feb 5, 2013 at 5:52 PM, <ncbray@chromium.org> wrote:

> I am semi-concerned about this CL. To point out the obvious:
>
> 1) --gtest_filter can't select between the individual tests.  Meh.
>
Yes, this sucks. We also can't use --gtest_also_run_disabled_tests.

We can mitigate it by adding our own flags, like --ppapi_filter or
--ppapi_also_run_disabled_tests. But, that's different and therefore sucks.
I don't have a good answer to this one except I think it's worth the
tradeoff.


>
> 2) The log parser will lump all these tests together.  Meh.
>
I don't think that will be a big problem in practice.


>
> 3) It will make the consolidated web socket test take longer than any
> individual
> test - pushing it closer to time out.  (The irony.)  You might need to
> monkey
> with the timeout for this mega test.
>
Marc-Antoine already upped the timeout to 120 seconds, so I think we're
okay here. I think we can agree that if we can make the total test time
much lower, then there's nothing wrong with increasing the timeout for a
single gtest test.

I also added logging of the elapsed time for each sub-test in this CL, so
we can tell which one is the long pole.


>
> 4) This is nonstandard and Chrome sheriffs will get confused and do stupid
> things if they suspect flake - you may seem them TBR a review that
> disables all
> the websocket tests.
>
Have a look at ppapi_browsertest.cc at the new WebSocket stuff if you
haven't. I tried to make it look as similar as I can; DISABLED and the
MAYBE pattern work as you would expect. So I think it's possible Sheriffs
can do it without hand-holding. But even if they can't, I don't mind them
turning off the whole test case. We'll get notified, and we can go whittle
it back down to only disable the parts that fail.


>
>
> All in all, it's different and different has a cost.  Is it worth it?
>
I just ran:
time out/Debug/browser_tests --gtest_filter="*PPAPI*WebSocket*"
With this change:
real 0m39.950s
user 0m10.960s
sys 0m1.570s

Without this change:
real 9m38.664s
user 3m7.040s
sys 0m31.080s



>
>
https://codereview.chromium.**org/12193015/<https://codereview.chromium.org/1...
>

[Nick Bray (chromium), 2013-02-06 23:30:58]

To be explicit, I am not blocking, just asking questions.

[dmichael (off chromium), 2013-02-07 19:17:54]

On 2013/02/06 23:30:58, Nick Bray (chromium) wrote:
> To be explicit, I am not blocking, just asking questions.
No problem, I appreciate the feedback. I'm not certain it won't be worse
overall, but I suspect it's mostly win. Feel free to look through and make
suggestions if you see ways to make it suck less.

Another benefit I forgot to mention is that we can now tell whether we've
enumerated all the tests. I have that part commented out in this CL until we
transition all tests over. But once we enable it, if somebody adds a single test
and forgets to add it to ppapi_browsertest.cc, they will get an error when they
run the tests. So that's nice.

Of course the big reason to do this is to improve bot cycle time and make the
rest of Chromium hate us less :-). I'm looking forward to seeing what the
difference is for these tests on the bots.

[bbudge, 2013-02-07 19:45:20]

LGTM

[commit-bot: I haz the power, 2013-02-19 21:18:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/dmichael@chromium.org/12193015/19001

[commit-bot: I haz the power, 2013-02-19 23:36:25]

Change committed as 183319