Test NaCl version of ppapi_tests via ui_tests

net/test/test_server.cc

Index: net/test/test_server.cc
===================================================================
--- net/test/test_server.cc	(revision 109675)
+++ net/test/test_server.cc	(working copy)
@@ -274,7 +274,11 @@
   FilePath src_dir;
   PathService::Get(base::DIR_SOURCE_ROOT, &src_dir);
 
-  document_root_ = src_dir.Append(document_root);
+  if (document_root.IsAbsolute()) {
+    document_root_ = document_root;
+  } else {
+    document_root_ = src_dir.Append(document_root);
+  }

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

seems fine to my ignorant eye, but you'll need phajdan.jr for OWNERS check.

[Paweł Hajdan Jr., 2011/11/14 12:31:06]

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]

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:15]

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
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.

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

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?

[Paweł Hajdan Jr., 2011/11/16 08:57:18]

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.
I'm fine with absolute paths. My point here is about consistency and least
surprise. More on that below.
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.
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.

 
   certificates_dir_ = src_dir.Append(FILE_PATH_LITERAL("net"))
                        .Append(FILE_PATH_LITERAL("data"))