Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(1)

Issue 10919265: First commit of new tools/run-tests.py (Closed)

Created:
7 years ago by Jakob Kummerow
Modified:
6 years, 12 months ago
Reviewers:
Michael Starzinger
CC:
v8-dev
Visibility:
Public.

Description

First commit of new tools/run-tests.py Committed: https://code.google.com/p/v8/source/detail?r=12583

Patch Set 1 #

Total comments: 26

Patch Set 2 : addressed comments #

Patch Set 3 : s/server.py/test-server.py/ in README #

Unified diffs Side-by-side diffs Delta from patch set Stats (+4742 lines, -216 lines) Patch
M .gitignore View 1 1 chunk +9 lines, -0 lines 0 comments Download
M test/benchmarks/testcfg.py View 1 chunk +5 lines, -0 lines 0 comments Download
M test/cctest/testcfg.py View 1 chunk +59 lines, -4 lines 0 comments Download
M test/es5conform/testcfg.py View 1 chunk +5 lines, -0 lines 0 comments Download
M test/message/testcfg.py View 1 chunk +82 lines, -2 lines 0 comments Download
M test/message/try-catch-finally-no-message.out View 1 chunk +26 lines, -26 lines 0 comments Download
M test/mjsunit/testcfg.py View 1 chunk +74 lines, -3 lines 0 comments Download
M test/mozilla/testcfg.py View 1 2 chunks +121 lines, -3 lines 0 comments Download
M test/preparser/testcfg.py View 2 chunks +101 lines, -5 lines 0 comments Download
M test/sputnik/testcfg.py View 1 chunk +5 lines, -0 lines 0 comments Download
M test/test262/testcfg.py View 1 1 chunk +96 lines, -8 lines 0 comments Download
M tools/presubmit.py View 1 chunk +1 line, -0 lines 0 comments Download
A tools/run-tests.py View 1 1 chunk +356 lines, -0 lines 0 comments Download
A + tools/status-file-converter.py View 2 chunks +11 lines, -10 lines 0 comments Download
A tools/test-server.py View 1 1 chunk +217 lines, -0 lines 0 comments Download
A tools/testrunner/README View 1 2 1 chunk +174 lines, -0 lines 0 comments Download
A + tools/testrunner/__init__.py View 1 chunk +1 line, -1 line 0 comments Download
A + tools/testrunner/local/__init__.py View 1 chunk +1 line, -1 line 0 comments Download
A tools/testrunner/local/commands.py View 1 chunk +152 lines, -0 lines 0 comments Download
A tools/testrunner/local/execution.py View 1 chunk +154 lines, -0 lines 0 comments Download
A tools/testrunner/local/old_statusfile.py View 1 1 chunk +460 lines, -0 lines 0 comments Download
A tools/testrunner/local/progress.py View 1 chunk +237 lines, -0 lines 0 comments Download
A tools/testrunner/local/statusfile.py View 1 1 chunk +145 lines, -0 lines 0 comments Download
A tools/testrunner/local/testsuite.py View 1 1 chunk +181 lines, -0 lines 0 comments Download
A + tools/testrunner/local/utils.py View 2 chunks +41 lines, -31 lines 0 comments Download
A tools/testrunner/local/verbose.py View 1 chunk +99 lines, -0 lines 0 comments Download
A + tools/testrunner/network/__init__.py View 1 chunk +1 line, -1 line 0 comments Download
A tools/testrunner/network/distro.py View 1 1 chunk +90 lines, -0 lines 0 comments Download
A tools/testrunner/network/endpoint.py View 1 1 chunk +114 lines, -0 lines 0 comments Download
A tools/testrunner/network/network_execution.py View 1 1 chunk +243 lines, -0 lines 0 comments Download
A tools/testrunner/network/perfdata.py View 1 chunk +120 lines, -0 lines 0 comments Download
A + tools/testrunner/objects/__init__.py View 1 chunk +1 line, -1 line 0 comments Download
A + tools/testrunner/objects/context.py View 1 chunk +23 lines, -32 lines 0 comments Download
A + tools/testrunner/objects/output.py View 2 chunks +33 lines, -28 lines 0 comments Download
A tools/testrunner/objects/peer.py View 1 chunk +80 lines, -0 lines 0 comments Download
A tools/testrunner/objects/testcase.py View 1 chunk +83 lines, -0 lines 0 comments Download
A tools/testrunner/objects/workpacket.py View 1 chunk +90 lines, -0 lines 0 comments Download
A + tools/testrunner/server/__init__.py View 1 chunk +1 line, -1 line 0 comments Download
A tools/testrunner/server/compression.py View 1 chunk +112 lines, -0 lines 0 comments Download
A + tools/testrunner/server/constants.py View 2 chunks +20 lines, -24 lines 0 comments Download
A tools/testrunner/server/daemon.py View 1 chunk +147 lines, -0 lines 0 comments Download
A tools/testrunner/server/local_handler.py View 1 chunk +119 lines, -0 lines 0 comments Download
A tools/testrunner/server/main.py View 1 chunk +245 lines, -0 lines 0 comments Download
A tools/testrunner/server/presence_handler.py View 1 1 chunk +120 lines, -0 lines 0 comments Download
A + tools/testrunner/server/signatures.py View 2 chunks +35 lines, -35 lines 0 comments Download
A tools/testrunner/server/status_handler.py View 1 chunk +113 lines, -0 lines 0 comments Download
A tools/testrunner/server/work_handler.py View 1 1 chunk +139 lines, -0 lines 0 comments Download

Messages

Total messages: 4 (0 generated)
Jakob Kummerow
Please take a look. Reviewer's guidance: Begin with tools/testrunner/README :-) Happy to walk you through ...
7 years ago (2012-09-13 14:47:16 UTC) #1
Michael Starzinger
LGTM. We already discussed most of the comments offline, so you already know which ones ...
6 years, 12 months ago (2012-09-21 13:20:40 UTC) #2
Michael Starzinger
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 ...
6 years, 12 months ago (2012-09-21 13:24:33 UTC) #3
Jakob Kummerow
6 years, 12 months ago (2012-09-21 17:02:24 UTC) #4
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.

Powered by Google App Engine
This is Rietveld 408576698