Add "run_testserver --sync-test" for easy chromiumsync_test.py launching.

Add "run_testserver --sync-test" for easy chromiumsync_test.py launching.

BUG=none
TEST=type that command. see tests run.

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=126847

[akalin, 2012-03-03 01:18:32]

LGTM, although i have mixed feelings about how this class is growing

we probably want to divorce the test sync server from the plain net server at
some point

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

http://codereview.chromium.org/9545019/diff/1/net/test/test_server.cc#newcode312
net/test/test_server.cc:312: if (!base::LaunchProcess(python_command,
base::LaunchOptions(), NULL)) {
can you add a VLOG of the final command line somewhere?  even better if you can
do it for the testserver

hmm i wonder if launchprocess has something like that already?

[tim (not reviewing), 2012-03-03 03:47:35]

On 2012/03/03 01:18:32, akalin wrote:
> LGTM, although i have mixed feelings about how this class is growing
> 
> we probably want to divorce the test sync server from the plain net server at
> some point
> 
> http://codereview.chromium.org/9545019/diff/1/net/test/test_server.cc
> File net/test/test_server.cc (right):
> 
>
http://codereview.chromium.org/9545019/diff/1/net/test/test_server.cc#newcode312
> net/test/test_server.cc:312: if (!base::LaunchProcess(python_command,
> base::LaunchOptions(), NULL)) {
> can you add a VLOG of the final command line somewhere?  even better if you
can
> do it for the testserver
> 
> hmm i wonder if launchprocess has something like that already?

Done, although I think what you wanted was the PYTHONPATH, right?
I added a vlog to LaunchPython to spit that out on posix.

Also, turns out there was already a routine in python_utils to get the python
runtime command, so I removed mine and use that.

[commit-bot: I haz the power, 2012-03-03 03:49:22]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/9545019/6001

[commit-bot: I haz the power, 2012-03-03 03:49:25]

Presubmit check for 9545019-6001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
net/test/test_server.h,net/test/test_server_posix.cc,net/test/test_server.cc

[Paweł Hajdan Jr., 2012-03-05 17:40:18]

Please do the split akalin suggested. I have the same concern.

[tim (not reviewing), 2012-03-05 17:43:42]

On 2012/03/05 17:40:18, Paweł Hajdan Jr. wrote:
> Please do the split akalin suggested. I have the same concern.

Sorry, I can't sign up to do that right now. This was merely for convenience, so
that people could actually run these tests. Otherwise, it's near impossible to
run them, so nobody does, and they were hence broken on ToT for a while (causing
me a lot of confusion when I tried to run them with my changes).

If you don't want to add this convenience switch here, I can instead move the
code from TestServer into a global method in run_testserver.cc.

[Paweł Hajdan Jr., 2012-03-06 20:24:28]

On 2012/03/05 17:43:42, timsteele wrote:
> On 2012/03/05 17:40:18, Paweł Hajdan Jr. wrote:
> > Please do the split akalin suggested. I have the same concern.
> 
> Sorry, I can't sign up to do that right now.

Guess what's going to happen later, when the incentive disappears completely
("it works now!"). I've seen stories like that just so many times.

> This was merely for convenience, so
> that people could actually run these tests. Otherwise, it's near impossible to
> run them, so nobody does, and they were hence broken on ToT for a while
(causing
> me a lot of confusion when I tried to run them with my changes).

I guess that's a pretty good incentive to do things the right way. No individual
patch is very messy, but those things do add up.

> If you don't want to add this convenience switch here, I can instead move the
> code from TestServer into a global method in run_testserver.cc.

Feel free to move the code to run_testserver.cc, I think I'm not OWNER there.

[tim (not reviewing), 2012-03-09 18:31:26]

On 2012/03/06 20:24:28, Paweł Hajdan Jr. wrote:
> On 2012/03/05 17:43:42, timsteele wrote:
> > On 2012/03/05 17:40:18, Paweł Hajdan Jr. wrote:
> > > Please do the split akalin suggested. I have the same concern.
> > 
> > Sorry, I can't sign up to do that right now.
> 
> Guess what's going to happen later, when the incentive disappears completely
> ("it works now!"). I've seen stories like that just so many times.
> 
> > This was merely for convenience, so
> > that people could actually run these tests. Otherwise, it's near impossible
to
> > run them, so nobody does, and they were hence broken on ToT for a while
> (causing
> > me a lot of confusion when I tried to run them with my changes).
> 
> I guess that's a pretty good incentive to do things the right way. No
individual
> patch is very messy, but those things do add up.
> 
> > If you don't want to add this convenience switch here, I can instead move
the
> > code from TestServer into a global method in run_testserver.cc.
> 
> Feel free to move the code to run_testserver.cc, I think I'm not OWNER there.

If I didn't think the most important thing was to have these tests running asap,
I would not have bothered to write this patch.  Divorcing the chromiumsync and
testserver code paths is something we've discussed before and needs careful
treatment / consideration.  I filed bug 117559 to track.

Getting tests running should not be gated on large refactorings, IMO, so I'd
like to land this.  Thanks guys.

[tim (not reviewing), 2012-03-13 00:38:42]

On 2012/03/09 18:31:26, timsteele wrote:
> On 2012/03/06 20:24:28, Paweł Hajdan Jr. wrote:
> > On 2012/03/05 17:43:42, timsteele wrote:
> > > On 2012/03/05 17:40:18, Paweł Hajdan Jr. wrote:
> > > > Please do the split akalin suggested. I have the same concern.
> > > 
> > > Sorry, I can't sign up to do that right now.
> > 
> > Guess what's going to happen later, when the incentive disappears completely
> > ("it works now!"). I've seen stories like that just so many times.
> > 
> > > This was merely for convenience, so
> > > that people could actually run these tests. Otherwise, it's near
impossible
> to
> > > run them, so nobody does, and they were hence broken on ToT for a while
> > (causing
> > > me a lot of confusion when I tried to run them with my changes).
> > 
> > I guess that's a pretty good incentive to do things the right way. No
> individual
> > patch is very messy, but those things do add up.
> > 
> > > If you don't want to add this convenience switch here, I can instead move
> the
> > > code from TestServer into a global method in run_testserver.cc.
> > 
> > Feel free to move the code to run_testserver.cc, I think I'm not OWNER
there.
> 
> If I didn't think the most important thing was to have these tests running
asap,
> I would not have bothered to write this patch.  Divorcing the chromiumsync and
> testserver code paths is something we've discussed before and needs careful
> treatment / consideration.  I filed bug 117559 to track.
> 
> Getting tests running should not be gated on large refactorings, IMO, so I'd
> like to land this.  Thanks guys.

Hey, Pawel, I'm still waiting on an LGTM from you for the now much-reduced
changes to TestServer.  Hope these look OK.  Thanks.

[Paweł Hajdan Jr., 2012-03-13 17:15:23]

Just minor comments. LGTM.

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

http://codereview.chromium.org/9545019/diff/12001/net/test/test_server.cc#new...
net/test/test_server.cc:106: return FilePath();
I don't like potentially ambiguous return types. Make the function return bool
instead (+WARN_UNUSED_RESULT), and return the FilePath via out-parameter.

http://codereview.chromium.org/9545019/diff/12001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/9545019/diff/12001/net/test/test_server_posix....
net/test/test_server_posix.cc:98: VLOG(1) << "LaunchPython called with
PYTHONPATH = " <<
Is this needed?

http://codereview.chromium.org/9545019/diff/12001/net/tools/testserver/run_te...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/9545019/diff/12001/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:46: VLOG(1) << "Launching " <<
python_command.GetCommandLineString();
Is this needed?

[tim (not reviewing), 2012-03-15 00:41:39]

Thanks for the review.

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

http://codereview.chromium.org/9545019/diff/12001/net/test/test_server.cc#new...
net/test/test_server.cc:106: return FilePath();
On 2012/03/13 17:15:23, Paweł Hajdan Jr. wrote:
> I don't like potentially ambiguous return types. Make the function return bool
> instead (+WARN_UNUSED_RESULT), and return the FilePath via out-parameter.

Done.

http://codereview.chromium.org/9545019/diff/12001/net/test/test_server_posix.cc
File net/test/test_server_posix.cc (right):

http://codereview.chromium.org/9545019/diff/12001/net/test/test_server_posix....
net/test/test_server_posix.cc:98: VLOG(1) << "LaunchPython called with
PYTHONPATH = " <<
On 2012/03/13 17:15:23, Paweł Hajdan Jr. wrote:
> Is this needed?

Fred asked for this in review, and I think it's useful in the event somebody
wants to run a nearby script (e.g. a test) in the same environment as the test
server, which is the problem we had that motivated this whole patch... I added a
comment.

http://codereview.chromium.org/9545019/diff/12001/net/tools/testserver/run_te...
File net/tools/testserver/run_testserver.cc (right):

http://codereview.chromium.org/9545019/diff/12001/net/tools/testserver/run_te...
net/tools/testserver/run_testserver.cc:46: VLOG(1) << "Launching " <<
python_command.GetCommandLineString();
On 2012/03/13 17:15:23, Paweł Hajdan Jr. wrote:
> Is this needed?

Removed this one.

[commit-bot: I haz the power, 2012-03-15 00:55:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/9545019/21001

[commit-bot: I haz the power, 2012-03-15 00:55:14]

Can't apply patch for file net/test/test_server.cc.
While running patch -p1 --forward --force;
patching file net/test/test_server.cc
Hunk #1 FAILED at 98.
1 out of 1 hunk FAILED -- saving rejects to file net/test/test_server.cc.rej

[commit-bot: I haz the power, 2012-03-15 03:47:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tim@chromium.org/9545019/22002

[commit-bot: I haz the power, 2012-03-15 05:09:23]

Try job failure for 9545019-22002 (retry) on win for step "compile" (clobber
build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...