Try to reduce the flankness of the PPAPI UI tests.

Try to reduce the flankness of the PPAPI UI tests.

Instead of a "started" and "completed" cookie, this now uses a separate
cookie for each test, so that the browser can monitor progress. Now it will
only time out if an individual test takes longer than the timeout, rather
than an entire test case.

TEST=this is
BUG=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98163

[Paweł Hajdan Jr., 2011-08-18 22:45:13]

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:100: while (true) {
NOOOOOOO! If you need a loop, you're doing it wrong. Please wait for an event
instead. Let me know if I can help with figuring out the right design. Feel free
to ping my by IM/irc/e-mail/at my desk.

[Paweł Hajdan Jr., 2011-08-18 22:47:20]

Sorry about my earlier comment, I was to eager to assume it's a flaky polling
loop.

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:100: while (true) {
Ooops, please ignore my earlier comment. It's not a polling loop.

[brettw, 2011-08-22 22:53:19]

Ping.

[polina, 2011-08-22 23:53:58]

Odd, for some reason overlooked this code review before. Sorry for the delay.

I am new to this code, so it took me a while to understand what was being done.
I added some comment/rename suggestions that I wish were there when I was first
reading this.

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:93: // See comment above TestingInstance in the
ppapi test plugin. Basically
a file path would be useful here

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:97: // keeps us from timing out on cookie waits
for long tests.
missing closing ")"

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:101: std::string cookie_name =
StringPrintf("PPAPI_PROGRESS_%d",
Is it not an option to share "PPAPI_PROGRESS", "...", etc constants with the
test code?

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.cc
File ppapi/tests/testing_instance.cc (right):

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.cc...
ppapi/tests/testing_instance.cc:20: static const char kProgressCookie[] = "...";
s/kProgressCookie/kProgressSignal/
Otherwise, it get confusing, what's a cookie, what's a value and what's a cookie
that's technically a PPAPI_PROGRESS cookie, but does not correspond to a
progress signal :).

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.cc...
ppapi/tests/testing_instance.cc:202: // Use streams since nacl doesn't compile
base yet (for stringprintf).
StringPrintf?

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.h
File ppapi/tests/testing_instance.h (right):

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.h#...
ppapi/tests/testing_instance.h:22: // We want to signal to the Chrome UI test
hardness that we're making progress
a file path could be useful here

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.h#...
ppapi/tests/testing_instance.h:26: // So we set a series of cookies with an
incrementing number in the name.
please add something like "per every test run within a TestCase".

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.h#...
ppapi/tests/testing_instance.h:29: // the test is progressing. It then waits for
the next numbered cookie until
TestCase is progressing

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.h#...
ppapi/tests/testing_instance.h:30: // it either times out or the value is
something other than "...". In this
Just to confirm, by sending more messages more often, we reduced the likelihood
of a time-out for a successful run. However, if one test run is misbehaving and
timing out, we still get the same fail-all behavior and don't get to skip over
to left-over tests? A clarification in the comments would be helpful.

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.h#...
ppapi/tests/testing_instance.h:31: // case, the value will be either "PASS" or
"FAIL [optional message]".
please add something like "corresponding to the outcome of the entire TestCase"

http://codereview.chromium.org/7677031/diff/1/ppapi/tests/testing_instance.h#...
ppapi/tests/testing_instance.h:110: int progress_number_;
progress_cookie_number_?

[brettw, 2011-08-24 16:42:17]

Thanks, I did the suggestions and uploaded a new snap.

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc
File chrome/test/ui/ppapi_uitest.cc (right):

http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:101: std::string cookie_name =
StringPrintf("PPAPI_PROGRESS_%d",
Unfortunately, there's no code in common between these two projects.

[polina, 2011-08-24 20:29:00]

On 2011/08/24 16:42:17, brettw wrote:
> Thanks, I did the suggestions and uploaded a new snap.
> 
> http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc
> File chrome/test/ui/ppapi_uitest.cc (right):
> 
>
http://codereview.chromium.org/7677031/diff/1/chrome/test/ui/ppapi_uitest.cc#...
> chrome/test/ui/ppapi_uitest.cc:101: std::string cookie_name =
> StringPrintf("PPAPI_PROGRESS_%d",
> Unfortunately, there's no code in common between these two projects.

Thanks.
LGTM