Test NaCl version of ppapi_tests via ui_tests

Test NaCl version of ppapi_tests via ui_tests

Copy server pieces to build output dir.
Update ppapi_uitests.cc to run server from output dir.
Add test to ppapi_uitests, update dependecies.
Update test_sever to support a fully qualified path.

BUG= http://code.google.com/p/chromium/issues/detail?id=96782
BUG= http://code.google.com/p/chromium/issues/detail?id=103690
TEST= try (ui_tests --gtest_filter="PPAPI*.*")
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=110221

[bradn, 2011-11-11 19:43:20]

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

http://codereview.chromium.org/8539019/diff/1/ppapi/tests/test_url_loader.cc#...
ppapi/tests/test_url_loader.cc:90: // 
RUN_TEST_FORCEASYNC_AND_NOT(SameOriginRestriction);
Why are these disabled?

[dmichael (off chromium), 2011-11-11 23:20:18]

some nits, LGTM (Assuming you're going to work on diagnosing/re-enabling those
tests you had to disable)

Adding phajdan.jr for net/test

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc#newcode281
net/test/test_server.cc:281: }
seems fine to my ignorant eye, but you'll need phajdan.jr for OWNERS check.

http://codereview.chromium.org/8539019/diff/9/ppapi/ppapi_tests.gypi
File ppapi/ppapi_tests.gypi (right):

http://codereview.chromium.org/8539019/diff/9/ppapi/ppapi_tests.gypi#newcode326
ppapi/ppapi_tests.gypi:326: # Does not compile under nacl (uses private
interface ExecuteScript).
nit: 80 characters in gyp files matter? Easy to fix since it's a comment, so I
think you might as well.

http://codereview.chromium.org/8539019/diff/9/ppapi/tests/test_url_loader.cc
File ppapi/tests/test_url_loader.cc (right):

http://codereview.chromium.org/8539019/diff/9/ppapi/tests/test_url_loader.cc#...
ppapi/tests/test_url_loader.cc:90: // Disable portion of test which failes when
the HTTP server's
nit: failes->fails

[Paweł Hajdan Jr., 2011-11-14 12:31:05]

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

http://codereview.chromium.org/8539019/diff/9/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:30: const char nacl_library_name[] =
"libppGoogleNaClPluginChrome.so";
Isn't this duplicating kInternalNaClPluginFileName from chrome_paths? Please
re-use the existing constant.

http://codereview.chromium.org/8539019/diff/9/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:124: PathService::Get(base::DIR_EXE,
&plugin_dir);
nit: Check the return value (EXPECT_TRUE).

http://codereview.chromium.org/8539019/diff/9/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:134: void AppendQuery(std::string* query, const
std::string& test_case) {
I don't like this design. How about BuildQuery(const std::string& base_path,
const std::string& test_case) ? StringPrintf might be useful.

http://codereview.chromium.org/8539019/diff/9/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:156: PathService::Get(base::DIR_EXE,
&plugin_dir);
nit: Please check return value here too.

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc#newcode281
net/test/test_server.cc:281: }
On 2011/11/11 23:20:18, dmichael wrote:
> seems fine to my ignorant eye, but you'll need phajdan.jr for OWNERS check.

Thanks. I'm not a fan of magic like this. Could you just make sure the path is
always absolute?

Bonus points for adding a check here to make sure the document root is indeed an
absolute path (feel free to do in a separate CL).

[noelallen1, 2011-11-14 22:28:17]

Also, fix manifest for 32 Bit.

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

http://codereview.chromium.org/8539019/diff/9/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:30: const char nacl_library_name[] =
"libppGoogleNaClPluginChrome.so";
On 2011/11/14 12:31:06, Paweł Hajdan Jr. wrote:
> Isn't this duplicating kInternalNaClPluginFileName from chrome_paths? Please
> re-use the existing constant.

Done.

http://codereview.chromium.org/8539019/diff/9/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:124: PathService::Get(base::DIR_EXE,
&plugin_dir);
On 2011/11/14 12:31:06, Paweł Hajdan Jr. wrote:
> nit: Check the return value (EXPECT_TRUE).

Done.

http://codereview.chromium.org/8539019/diff/9/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:134: void AppendQuery(std::string* query, const
std::string& test_case) {
On 2011/11/14 12:31:06, Paweł Hajdan Jr. wrote:
> I don't like this design. How about BuildQuery(const std::string& base_path,
> const std::string& test_case) ? StringPrintf might be useful.

Done.

http://codereview.chromium.org/8539019/diff/9/chrome/test/ui/ppapi_uitest.cc#...
chrome/test/ui/ppapi_uitest.cc:156: PathService::Get(base::DIR_EXE,
&plugin_dir);
On 2011/11/14 12:31:06, Paweł Hajdan Jr. wrote:
> nit: Please check return value here too.

Done.

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc#newcode281
net/test/test_server.cc:281: }
On 2011/11/14 12:31:06, Paweł Hajdan Jr. wrote:
> On 2011/11/11 23:20:18, dmichael wrote:
> > seems fine to my ignorant eye, but you'll need phajdan.jr for OWNERS check.
> 
> Thanks. I'm not a fan of magic like this. Could you just make sure the path is
> always absolute?
> 
> Bonus points for adding a check here to make sure the document root is indeed
an
> absolute path (feel free to do in a separate CL).

I don't agree.  It makes sense that "Append" throws and error, since it implies
a non-empty path, so we can't make allowances there.  It also makes sense that a
tool (in this case a cmd-line tool) support both absolute and relative paths.

So it seems to me we are doing the correct thing here.  Either that or FilePath
should have a 'SetOrAppend' (a.k.a 'cd') function.

[Paweł Hajdan Jr., 2011-11-15 20:02:14]

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc#newcode281
net/test/test_server.cc:281: }
On 2011/11/14 22:28:17, noelallen1 wrote:
> I don't agree.  It makes sense that "Append" throws and error, since it
implies
> a non-empty path, so we can't make allowances there.

Sorry, I don't know what error this refers to. I think there is some check in
Append that asserts the path being appended is not absolute. It's even from my
2009 patch http://src.chromium.org/viewvc/chrome?view=rev&revision=6511 ;-D

> It also makes sense that a
> tool (in this case a cmd-line tool) support both absolute and relative paths.

What I don't like here is the kind of auto-detection. I think it can be quite
error prone, and should either never be absolute, or always be absolute.
Otherwise it's very easy to create confusion what is going to be the base of the
relative path (caller might think it's cwd or something like that).

The current semantics of TestServer are very clear: it takes a path relative to
the source directory. If we start passing absolute paths to it, we should either
switch everyone to absolute paths, or don't pass absolute paths in this CL. The
rationale here is that people misuse those testing utilities "all the time". I
try to make their interface as hard to get wrong as possible.

[noelallen1, 2011-11-15 20:57:57]

I still disagree.  I appreciate what you are trying to do, but having me go
around and change all the other tests without understanding them seems more
dangerous.  We already have a mysterious failure because we have affected the
source location, you suggestion could introduce more.  See:
  http://code.google.com/p/chromium/issues/detail?id=103690


The real issue here, IMO, is that with a function init: private:
  void Init(const FilePath& document_root);

With this signature and a variable named document_ROOT, and no comment, I don't
think we can expect someone to use a relative path.  In fact, there really
should be no constraint other than document_root to be valid which it is.

If you feel this is incorrect, then I think the correct thing to do is file a
bug against TestServer instead of blocking the CL.

[rvargas (doing something else), 2011-11-15 23:26:18]

LGTM for net.

http://codereview.chromium.org/8539019/diff/5002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8539019/diff/5002/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:804: '../ppapi/tests//test_url_loader_data/hello.txt',
nit: double '/'

[rvargas (doing something else), 2011-11-15 23:50:21]

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc#newcode281
net/test/test_server.cc:281: }
I'm sorry, I only read these comments after my previous reply, so my answer was
not intended as an override for this discussion.

Personally I find it a little annoying that this tool is picky about the type of
argument it accepts, so my first reaction was "great, we are adding more
flexibility here". That said, I have not dealt personally with some of the
issues related to the test server being stuck and the random test failures that
we've had in the past 'cause of it, so I don't know for sure if adding
restrictions is the way to reduce problems in the future.

Pawel, could you expand a little about you think may happen if we do this part
of the change? Isn't the case that providing the "wrong" kind of path will just
cause a new test to fail all the time?

[noelallen1, 2011-11-16 00:18:22]

http://codereview.chromium.org/8539019/diff/5002/chrome/chrome_tests.gypi
File chrome/chrome_tests.gypi (right):

http://codereview.chromium.org/8539019/diff/5002/chrome/chrome_tests.gypi#new...
chrome/chrome_tests.gypi:804: '../ppapi/tests//test_url_loader_data/hello.txt',
On 2011/11/15 23:26:18, rvargas wrote:
> nit: double '/'

Done.

[James Hawkins, 2011-11-16 06:14:33]

I believe this commit caused regression across the board on the memory
waterfall.  See
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20Tests%20%28v...
for an example.  There's a lot of red, so I'm inclined to revert.

[Paweł Hajdan Jr., 2011-11-16 08:57:17]

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc
File net/test/test_server.cc (right):

http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc#newcode281
net/test/test_server.cc:281: }
On 2011/11/15 23:50:21, rvargas wrote:
> I'm sorry, I only read these comments after my previous reply, so my answer
was
> not intended as an override for this discussion.

Thanks. By the way, I'm annoyed this change was committed with my objections
still in place. I'm really _tired_ of keeping things sane while people break
them.

> Personally I find it a little annoying that this tool is picky about the type
of
> argument it accepts, so my first reaction was "great, we are adding more
> flexibility here".

I'm fine with absolute paths. My point here is about consistency and least
surprise. More on that below.

> That said, I have not dealt personally with some of the
> issues related to the test server being stuck and the random test failures
that
> we've had in the past 'cause of it, so I don't know for sure if adding
> restrictions is the way to reduce problems in the future.

Right, a lot of problems here have been gone, thanks to great work of David
Benjamin and other people. I mostly focused on making the public interface
simple, predictable, and hard to misuse (the previous one was just an
_invitation_ for crashes, and tests were indeed crashing).

So the "quality" of the interface translates indirectly to stability of the
tests using it.

> Pawel, could you expand a little about you think may happen if we do this part
> of the change? Isn't the case that providing the "wrong" kind of path will
just
> cause a new test to fail all the time?

Yeah, any failures resulting from this would be deterministic. That's not my
concern here at all. I think now the code using the new interface with absolute
paths mixed with paths relative to something that is not necessarily current
working directory is confusing and ugly.

I used to revert changes that were committed without approval. This time all I
can say is "if you really want to break it, I can't stop you". It's a small
thing, but those small things accumulate.

[dmichael (off chromium), 2011-11-16 16:30:44]

Noel,
w.r.t. the tests failing on the valgrind bots, many (most?) of the
corresponding PPAPI tests are disabled for the valgrind bots. We should
eventually look at fixing them, but for now, I think it would be best to
disable the NaCl versions of any PPAPI tests that are disabled (and maybe
more, if others are failing).

See tools/valgrind/gtest_exclude/ui_tests.gtest*

w.r.t. absolute paths, I admittedly don't really know the ins and outs of
the problem. But after reading this debate, my intuition is that allowing
absolute paths is an invitation for flakiness. It seems like any tests that
use absolute paths will be more fragile in the face of things like changing
bot configurations, etc. It happens that your CL makes sure the path is
still right, but the next person who comes along and uses an absolute path
might not do it right.

I think given the issue with the tests and the still controversial path,
it's probably best to revert. I'll go ahead and do that (sorry, Noel).
-Dave

On Wed, Nov 16, 2011 at 1:57 AM, <phajdan.jr@chromium.org> wrote:

>
>
http://codereview.chromium.**org/8539019/diff/9/net/test/**test_server.cc<htt...
> File net/test/test_server.cc (right):
>
> http://codereview.chromium.**org/8539019/diff/9/net/test/**
>
test_server.cc#newcode281<http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc#newcode281>
> net/test/test_server.cc:281: }
> On 2011/11/15 23:50:21, rvargas wrote:
>
>> I'm sorry, I only read these comments after my previous reply, so my
>>
> answer was
>
>> not intended as an override for this discussion.
>>
>
> Thanks. By the way, I'm annoyed this change was committed with my
> objections still in place. I'm really _tired_ of keeping things sane
> while people break them.
>
>
>  Personally I find it a little annoying that this tool is picky about
>>
> the type of
>
>> argument it accepts, so my first reaction was "great, we are adding
>>
> more
>
>> flexibility here".
>>
>
> I'm fine with absolute paths. My point here is about consistency and
> least surprise. More on that below.
>
>
>  That said, I have not dealt personally with some of the
>> issues related to the test server being stuck and the random test
>>
> failures that
>
>> we've had in the past 'cause of it, so I don't know for sure if adding
>> restrictions is the way to reduce problems in the future.
>>
>
> Right, a lot of problems here have been gone, thanks to great work of
> David Benjamin and other people. I mostly focused on making the public
> interface simple, predictable, and hard to misuse (the previous one was
> just an _invitation_ for crashes, and tests were indeed crashing).
>
> So the "quality" of the interface translates indirectly to stability of
> the tests using it.
>
>
>  Pawel, could you expand a little about you think may happen if we do
>>
> this part
>
>> of the change? Isn't the case that providing the "wrong" kind of path
>>
> will just
>
>> cause a new test to fail all the time?
>>
>
> Yeah, any failures resulting from this would be deterministic. That's
> not my concern here at all. I think now the code using the new interface
> with absolute paths mixed with paths relative to something that is not
> necessarily current working directory is confusing and ugly.
>
> I used to revert changes that were committed without approval. This time
> all I can say is "if you really want to break it, I can't stop you".
> It's a small thing, but those small things accumulate.
>
>
http://codereview.chromium.**org/8539019/<http://codereview.chromium.org/8539...
>

[dmichael (off chromium), 2011-11-16 17:49:48]

FYI: This is reverted as of r110310

On Wed, Nov 16, 2011 at 9:30 AM, David Michael <dmichael@chromium.org>wrote:

> Noel,
> w.r.t. the tests failing on the valgrind bots, many (most?) of the
> corresponding PPAPI tests are disabled for the valgrind bots. We should
> eventually look at fixing them, but for now, I think it would be best to
> disable the NaCl versions of any PPAPI tests that are disabled (and maybe
> more, if others are failing).
>
> See tools/valgrind/gtest_exclude/ui_tests.gtest*
>
> w.r.t. absolute paths, I admittedly don't really know the ins and outs of
> the problem. But after reading this debate, my intuition is that allowing
> absolute paths is an invitation for flakiness. It seems like any tests that
> use absolute paths will be more fragile in the face of things like changing
> bot configurations, etc. It happens that your CL makes sure the path is
> still right, but the next person who comes along and uses an absolute path
> might not do it right.
>
> I think given the issue with the tests and the still controversial path,
> it's probably best to revert. I'll go ahead and do that (sorry, Noel).
> -Dave
>
> On Wed, Nov 16, 2011 at 1:57 AM, <phajdan.jr@chromium.org> wrote:
>
>>
>>
http://codereview.chromium.**org/8539019/diff/9/net/test/**test_server.cc<htt...
>> File net/test/test_server.cc (right):
>>
>> http://codereview.chromium.**org/8539019/diff/9/net/test/**
>>
test_server.cc#newcode281<http://codereview.chromium.org/8539019/diff/9/net/test/test_server.cc#newcode281>
>> net/test/test_server.cc:281: }
>> On 2011/11/15 23:50:21, rvargas wrote:
>>
>>> I'm sorry, I only read these comments after my previous reply, so my
>>>
>> answer was
>>
>>> not intended as an override for this discussion.
>>>
>>
>> Thanks. By the way, I'm annoyed this change was committed with my
>> objections still in place. I'm really _tired_ of keeping things sane
>> while people break them.
>>
>>
>>  Personally I find it a little annoying that this tool is picky about
>>>
>> the type of
>>
>>> argument it accepts, so my first reaction was "great, we are adding
>>>
>> more
>>
>>> flexibility here".
>>>
>>
>> I'm fine with absolute paths. My point here is about consistency and
>> least surprise. More on that below.
>>
>>
>>  That said, I have not dealt personally with some of the
>>> issues related to the test server being stuck and the random test
>>>
>> failures that
>>
>>> we've had in the past 'cause of it, so I don't know for sure if adding
>>> restrictions is the way to reduce problems in the future.
>>>
>>
>> Right, a lot of problems here have been gone, thanks to great work of
>> David Benjamin and other people. I mostly focused on making the public
>> interface simple, predictable, and hard to misuse (the previous one was
>> just an _invitation_ for crashes, and tests were indeed crashing).
>>
>> So the "quality" of the interface translates indirectly to stability of
>> the tests using it.
>>
>>
>>  Pawel, could you expand a little about you think may happen if we do
>>>
>> this part
>>
>>> of the change? Isn't the case that providing the "wrong" kind of path
>>>
>> will just
>>
>>> cause a new test to fail all the time?
>>>
>>
>> Yeah, any failures resulting from this would be deterministic. That's
>> not my concern here at all. I think now the code using the new interface
>> with absolute paths mixed with paths relative to something that is not
>> necessarily current working directory is confusing and ugly.
>>
>> I used to revert changes that were committed without approval. This time
>> all I can say is "if you really want to break it, I can't stop you".
>> It's a small thing, but those small things accumulate.
>>
>>
http://codereview.chromium.**org/8539019/<http://codereview.chromium.org/8539...
>>
>
>