First commit of new tools/run-tests.py

First commit of new tools/run-tests.py

Committed: https://code.google.com/p/v8/source/detail?r=12583

[Jakob Kummerow, 2012-09-13 14:47:16]

Please take a look.

Reviewer's guidance:
Begin with tools/testrunner/README :-)

Happy to walk you through this offline. 
Also happy to add more comments to the source wherever you feel that would be
helpful.

[Michael Starzinger, 2012-09-21 13:20:40]

LGTM.

We already discussed most of the comments offline, so you already know which
ones to ignore. Also thanks a lot for the walk-through of the code, that was
really helpful!

To sum up our discussion of the new status file format: You convinced me that we
should replace the current format with the new format soon-ish (not now and
certainly not in this CL!) if two points are addressed:

1) The initial 'True' condition of the unconditional block should be turned into
a domain-specific keyword like "ALWAYS", "UNCONDITIONALLY" or
"FOR_ALL_ARCHITECTURES"

2) The values in the conditions should also be made domain-specific keywords. So
"arm" instead of "\"arm\"" and likewise. I think lowercase makes it more
readable here.

And now let's distribute the hell out of it, I am really looking forward to
using this! :)

https://codereview.chromium.org/10919265/diff/1/tools/run-tests.py
File tools/run-tests.py (right):

https://codereview.chromium.org/10919265/diff/1/tools/run-tests.py#newcode153
tools/run-tests.py:153: if not arch in ['ia32', 'x64', 'arm', 'mips']:
Shouldn't that be "mipsel"?

https://codereview.chromium.org/10919265/diff/1/tools/run-tests.py#newcode179
tools/run-tests.py:179: if options.arch == "arm" or options.arch == "mips":
Likewise.

https://codereview.chromium.org/10919265/diff/1/tools/server.py
File tools/server.py (right):

https://codereview.chromium.org/10919265/diff/1/tools/server.py#newcode150
tools/server.py:150: 
We probably should add a separating comment line here that indicates that only
after this point can you assume that the rest of the test runner framework is
present. This file is a standalone script before this point.

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/README
File tools/testrunner/README (right):

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/README#newco...
tools/testrunner/README:12: The interface is mostly the same as it was for
tools/test-wrapper-gypbuild.py.
Since the old script was just a temporary wrapper, I wouldn't cite it's name
here but just call it "[...] as it was for the old test wrapper/runner/harness.
[...]"

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/README#newco...
tools/testrunner/README:17: --nonetwork is the default on Mac and Windows. If
you don't specify --arch
Can we make --nonetwork the default on Linux too? I want this thing to just work
as before without any special --make-this-behave-as-before flag. If it falls
back to non-network mode silently and quickly then I am fine with how it is ATM.

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/README#newco...
tools/testrunner/README:33: 1.) Copy tools/server.py to a new empty directory
anywhere on your hard drive
Does symlinking work? If yes that would be awesome and should be mentioned here.

I just realized that it won't work with a symlink because of the self-update. We
should mention that here.

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/README#newco...
tools/testrunner/README:80: ./server.py:
I am not sure about the name of this script. How about "test-server.py". I know
that is also disambiguate because it might sound like "testing a server".

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/disco...
File tools/testrunner/server/discovery.py (right):

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/disco...
tools/testrunner/server/discovery.py:54: class
PresenceHandler(SocketServer.BaseRequestHandler):
This file should probably be called "presence_handler.py".

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/local...
File tools/testrunner/server/local_handler.py (right):

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/local...
tools/testrunner/server/local_handler.py:70: elif action ==
constants.REQUEST_PUBKEY_FINGERPRINT:
This action could be avoided if you do mapping from IP address to public key
when verifying the signature.

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/signa...
File tools/testrunner/server/signatures.py (right):

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/signa...
tools/testrunner/server/signatures.py:41: code = subprocess.call("openssl dgst
-out %s -sign %s %s" %
Interesting, you can use OpenSSL on SSH keys, didn't know that.

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/work_...
File tools/testrunner/server/work_handler.py (right):

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/work_...
tools/testrunner/server/work_handler.py:90: self._Call("rm -rf %s" %
self.ctx.shell_dir)
As discussed offline, we should make sure this doesn't kill downloaded test
data.

https://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/work_...
tools/testrunner/server/work_handler.py:126: code = self._Call("patch -p1 < %s"
% patchfilename)
As discussed offline, better use "git apply" here.

[Michael Starzinger, 2012-09-21 13:24:33]

Addendum.

https://codereview.chromium.org/10919265/diff/1/tools/server.py
File tools/server.py (right):

https://codereview.chromium.org/10919265/diff/1/tools/server.py#newcode98
tools/server.py:98: privkeyfile = os.path.expanduser("~/.ssh/v8_dtest")
I am not sure if we should put this key into the user's home dir or into the
server's working dir. But since I don't have strong arguments given that you can
only run one instance of the server anyways, I can't argue my point. :)

[Jakob Kummerow, 2012-09-21 17:02:24]

New patch set uploaded.

Also contains some small fixes for issues I found during further testing.

http://codereview.chromium.org/10919265/diff/1/tools/run-tests.py
File tools/run-tests.py (right):

http://codereview.chromium.org/10919265/diff/1/tools/run-tests.py#newcode153
tools/run-tests.py:153: if not arch in ['ia32', 'x64', 'arm', 'mips']:
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> Shouldn't that be "mipsel"?

Done.

http://codereview.chromium.org/10919265/diff/1/tools/run-tests.py#newcode179
tools/run-tests.py:179: if options.arch == "arm" or options.arch == "mips":
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> Likewise.

Done.

http://codereview.chromium.org/10919265/diff/1/tools/server.py
File tools/server.py (right):

http://codereview.chromium.org/10919265/diff/1/tools/server.py#newcode98
tools/server.py:98: privkeyfile = os.path.expanduser("~/.ssh/v8_dtest")
On 2012/09/21 13:24:33, Michael Starzinger wrote:
> I am not sure if we should put this key into the user's home dir or into the
> server's working dir. But since I don't have strong arguments given that you
can
> only run one instance of the server anyways, I can't argue my point. :)

Actually, there's a good reason to keep it in a known/accessible location like
the homedir: run-tests.py needs this too to sign binaries that are being sent
out, and it shouldn't have to care about the path to the server.

http://codereview.chromium.org/10919265/diff/1/tools/server.py#newcode150
tools/server.py:150: 
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> We probably should add a separating comment line here that indicates that only
> after this point can you assume that the rest of the test runner framework is
> present. This file is a standalone script before this point.

Done.

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/README
File tools/testrunner/README (right):

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/README#newcode12
tools/testrunner/README:12: The interface is mostly the same as it was for
tools/test-wrapper-gypbuild.py.
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> Since the old script was just a temporary wrapper, I wouldn't cite it's name
> here but just call it "[...] as it was for the old test
wrapper/runner/harness.
> [...]"

Done.

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/README#newcode17
tools/testrunner/README:17: --nonetwork is the default on Mac and Windows. If
you don't specify --arch
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> Can we make --nonetwork the default on Linux too? I want this thing to just
work
> as before without any special --make-this-behave-as-before flag. If it falls
> back to non-network mode silently and quickly then I am fine with how it is
ATM.

Yes, if the server is not running it falls back silently.

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/README#newcode33
tools/testrunner/README:33: 1.) Copy tools/server.py to a new empty directory
anywhere on your hard drive
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> Does symlinking work? If yes that would be awesome and should be mentioned
here.
> 
> I just realized that it won't work with a symlink because of the self-update.
We
> should mention that here.

Done.

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/README#newcode80
tools/testrunner/README:80: ./server.py:
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> I am not sure about the name of this script. How about "test-server.py". I
know
> that is also disambiguate because it might sound like "testing a server".

Done.

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/discov...
File tools/testrunner/server/discovery.py (right):

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/discov...
tools/testrunner/server/discovery.py:54: class
PresenceHandler(SocketServer.BaseRequestHandler):
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> This file should probably be called "presence_handler.py".

Done.

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/local_...
File tools/testrunner/server/local_handler.py (right):

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/local_...
tools/testrunner/server/local_handler.py:70: elif action ==
constants.REQUEST_PUBKEY_FINGERPRINT:
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> This action could be avoided if you do mapping from IP address to public key
> when verifying the signature.

I agree, but I don't think it's important. Let's leave it as it is for now.

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/signat...
File tools/testrunner/server/signatures.py (right):

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/signat...
tools/testrunner/server/signatures.py:41: code = subprocess.call("openssl dgst
-out %s -sign %s %s" %
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> Interesting, you can use OpenSSL on SSH keys, didn't know that.

Only the private key format is compatible. They both have their own format for
public keys.

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/work_h...
File tools/testrunner/server/work_handler.py (right):

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/work_h...
tools/testrunner/server/work_handler.py:90: self._Call("rm -rf %s" %
self.ctx.shell_dir)
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> As discussed offline, we should make sure this doesn't kill downloaded test
> data.

Done (by means of .gitignore).

http://codereview.chromium.org/10919265/diff/1/tools/testrunner/server/work_h...
tools/testrunner/server/work_handler.py:126: code = self._Call("patch -p1 < %s"
% patchfilename)
On 2012/09/21 13:20:40, Michael Starzinger wrote:
> As discussed offline, better use "git apply" here.

Done.