Split up Each Swarm Test into Two Steps

Split up Each Swarm Test into Two Steps

Each swarm step will be now be split into an upload step, where it sends the test request to swarm, and a get step, where it retrieves the output from the test. All the upload steps occur before any get steps, so the swarm master will see all the test requests right away and be able to start them as soon as possible.

BUG=98834
TEST=


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=133190

[csharp, 2012-04-17 19:20:37]

Most of the code in get_swarm_results.py and get_swarm_results_unittest.py was
just move from the ran_slavelastic stuff so there aren't as many changed as the
line count would make it seem.

I decided to not make all the test upload occur in 1 step for this change since
it isn't required to get this stuff working, but that will be my next cl.

[M-A Ruel, 2012-04-17 20:16:00]

Is the unit test run by the presubmit check, I don't think so, it'd be good to
have it run automatically.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
File scripts/master/factory/swarm_commands.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
scripts/master/factory/swarm_commands.py:18: def AddSendSwarmTestStep(self,
target_platform, swarm_server,
I'd prefer "Trigger" to "Send". It is more meaningful (to me).

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
scripts/master/factory/swarm_commands.py:19: swarm_port, min_shards, max_shards,
weird alignment.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
File scripts/master/factory/swarm_factory.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
scripts/master/factory/swarm_factory.py:34:
swarm_command_obj.AddSendSwarmTestStep(self._target_platform,
That's weird that triggering the jobs are all done at once but getting the
results are independent steps. I think it'd be better to not send a list to
AddSendSwarmTestStep() and call it in a loop instead, like it is done for
AddGetSwarmTestStep(). Or at least have both be the same.

Any opinion on that?

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
File scripts/slave/get_swarm_results.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:8: import json # pylint: disable=F0401
You shouldn't need to disable the pylint warning, it was just an issue in python
2.5 days.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:15: DESCRIPTION = """This script takes in a
test name and retrives all the output
Make that a docstring, e.g. move it around line 7. No need for "This script",
just
"""Takes a test name and ..."""

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:18: 
2 lines between file level symbols

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:89: return '\n'.join(test_run_output)
Technically, it's better to use splitlines(True) and use
''.join(test_run_output) so the ending '\n' is kept. No big deal.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:101: description=DESCRIPTION)
description=sys.modules[__name__].__doc__

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:128: test_keys = result.split()
Oh, I guess eventually it'd make sense to use a proper format like json but it's
fine for now.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:130: # Get the swarm results.
Can you move that out of main into it's own function? At least in theory it'd
make unit testing easier.

[csharp, 2012-04-18 18:20:29]

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
File scripts/master/factory/swarm_commands.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
scripts/master/factory/swarm_commands.py:18: def AddSendSwarmTestStep(self,
target_platform, swarm_server,
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> I'd prefer "Trigger" to "Send". It is more meaningful (to me).

Done.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
scripts/master/factory/swarm_commands.py:19: swarm_port, min_shards, max_shards,
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> weird alignment.

Done.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
File scripts/master/factory/swarm_factory.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/master/factory...
scripts/master/factory/swarm_factory.py:34:
swarm_command_obj.AddSendSwarmTestStep(self._target_platform,
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> That's weird that triggering the jobs are all done at once but getting the
> results are independent steps. I think it'd be better to not send a list to
> AddSendSwarmTestStep() and call it in a loop instead, like it is done for
> AddGetSwarmTestStep(). Or at least have both be the same.
> 
> Any opinion on that?

Well the test triggers will soon all be one step, so I don't want to break that
apart.

I left GetSwarmTestStep in a loop because it just seemed cleaner to me to have
each command function associated with exactly 1 step being created. If you think
it looks better to hide the loop inside the command then I have no problem
making that change.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
File scripts/slave/get_swarm_results.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:8: import json # pylint: disable=F0401
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> You shouldn't need to disable the pylint warning, it was just an issue in
python
> 2.5 days.

Done.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:15: DESCRIPTION = """This script takes in a
test name and retrives all the output
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> Make that a docstring, e.g. move it around line 7. No need for "This script",
> just
> """Takes a test name and ..."""

Done.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:18: 
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> 2 lines between file level symbols

Done.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:89: return '\n'.join(test_run_output)
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> Technically, it's better to use splitlines(True) and use
> ''.join(test_run_output) so the ending '\n' is kept. No big deal.

Done.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:101: description=DESCRIPTION)
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> description=sys.modules[__name__].__doc__

Done.

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:128: test_keys = result.split()
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> Oh, I guess eventually it'd make sense to use a proper format like json but
it's
> fine for now.

TODO added

https://chromiumcodereview.appspot.com/10035003/diff/1/scripts/slave/get_swar...
scripts/slave/get_swarm_results.py:130: # Get the swarm results.
On 2012/04/17 20:16:00, Marc-Antoine Ruel wrote:
> Can you move that out of main into it's own function? At least in theory it'd
> make unit testing easier.

Done.

[M-A Ruel, 2012-04-19 18:31:28]

Also, you should update PRESUBMIT.py to run tests found in
scripts/slave/unittests automatically, like it's doing for
scripts/master/unittests.

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/get_s...
File scripts/slave/get_swarm_results.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:28: 
2 lines between file level symbols

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:29: class TestSummary(object):
TestRunSummary would be better IMHO.

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:181: parser.add_option('-n', '--hostname',
default='localhost',
why not accept an url instead? As such, it would be able to specify https.

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/test_...
File scripts/slave/test_env.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/test_...
scripts/slave/test_env.py:1: # Copyright (c) 2012 The Chromium Authors. All
rights reserved.
Can you put this and the tests into slave/unittests/ so it is symmetric with
master?

[csharp, 2012-04-19 19:02:16]

Updated presubmit to include the slave tests now (it slipped my mind last time
and I only checked I had answered all the comments, sorry)

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/get_s...
File scripts/slave/get_swarm_results.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:28: 
On 2012/04/19 18:31:28, Marc-Antoine Ruel wrote:
> 2 lines between file level symbols

Done.

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:29: class TestSummary(object):
On 2012/04/19 18:31:28, Marc-Antoine Ruel wrote:
> TestRunSummary would be better IMHO.

Done.

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:181: parser.add_option('-n', '--hostname',
default='localhost',
On 2012/04/19 18:31:28, Marc-Antoine Ruel wrote:
> why not accept an url instead? As such, it would be able to specify https.

Good point. Changed

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/test_...
File scripts/slave/test_env.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/5001/scripts/slave/test_...
scripts/slave/test_env.py:1: # Copyright (c) 2012 The Chromium Authors. All
rights reserved.
On 2012/04/19 18:31:28, Marc-Antoine Ruel wrote:
> Can you put this and the tests into slave/unittests/ so it is symmetric with
> master?

Done.

[M-A Ruel, 2012-04-19 19:07:19]

https://chromiumcodereview.appspot.com/10035003/diff/9001/masters/master.chro...
File masters/master.chromium.swarm/master.cfg (right):

https://chromiumcodereview.appspot.com/10035003/diff/9001/masters/master.chro...
masters/master.chromium.swarm/master.cfg:92: 'swarm_server' :
'http://vm155-m1'})
'swarm_server' : 'http://vm155-m1:9001/'})
?

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
File scripts/slave/get_swarm_results.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:109: def GetSwarmResults(hostname, port,
test_keys):
def GetSwarmResults(swarm_base_url, test_keys):

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:115: result_url = '%s/get_result?r=%s' %
(swarm_base_url, test_keys[index])
you may want to use swarm_base_url.rstrip('/')

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:185: parser.add_option('-p', '--port',
type='int', default=8080,
Remove, not needed

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:200: 
2 lines between file level symbols. Yeah, I know. After a few years, you too
will be assimilated.

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/run_s...
File scripts/slave/run_slavelastic.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/run_s...
scripts/slave/run_slavelastic.py:161: parser.add_option('-p', '--port',
type='int', default=8080,
Eventually should die too.

[csharp, 2012-04-19 19:23:22]

https://chromiumcodereview.appspot.com/10035003/diff/9001/masters/master.chro...
File masters/master.chromium.swarm/master.cfg (right):

https://chromiumcodereview.appspot.com/10035003/diff/9001/masters/master.chro...
masters/master.chromium.swarm/master.cfg:92: 'swarm_server' :
'http://vm155-m1'})
On 2012/04/19 19:07:19, Marc-Antoine Ruel wrote:
> 'swarm_server' : 'http://vm155-m1:9001/'})
> ?

Done.

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
File scripts/slave/get_swarm_results.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:109: def GetSwarmResults(hostname, port,
test_keys):
On 2012/04/19 19:07:19, Marc-Antoine Ruel wrote:
> def GetSwarmResults(swarm_base_url, test_keys):

Done.

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:115: result_url = '%s/get_result?r=%s' %
(swarm_base_url, test_keys[index])
On 2012/04/19 19:07:19, Marc-Antoine Ruel wrote:
> you may want to use swarm_base_url.rstrip('/')

Done.

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:185: parser.add_option('-p', '--port',
type='int', default=8080,
On 2012/04/19 19:07:19, Marc-Antoine Ruel wrote:
> Remove, not needed

Done.

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/get_s...
scripts/slave/get_swarm_results.py:200: 
On 2012/04/19 19:07:19, Marc-Antoine Ruel wrote:
> 2 lines between file level symbols. Yeah, I know. After a few years, you too
> will be assimilated.

Done.

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/run_s...
File scripts/slave/run_slavelastic.py (right):

https://chromiumcodereview.appspot.com/10035003/diff/9001/scripts/slave/run_s...
scripts/slave/run_slavelastic.py:161: parser.add_option('-p', '--port',
type='int', default=8080,
On 2012/04/19 19:07:19, Marc-Antoine Ruel wrote:
> Eventually should die too.

Removed (might as well keep the two swarm communicators in sync)

[M-A Ruel, 2012-04-19 20:07:34]

lgtm

[commit-bot: I haz the power, 2012-04-19 20:13:23]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10035003/12001

[commit-bot: I haz the power, 2012-04-19 20:21:17]

Presubmit check for 10035003-12001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit Warnings **
Out of 4 files, found none that matched w=['.+_test\\.py$'], b=None in directory
scripts/slave/unittests

Presubmit checks took 468.1s to calculate.

[M-A Ruel, 2012-04-19 20:26:37]

On 2012/04/19 20:21:17, I haz the power (commit-bot) wrote:
> Out of 4 files, found none that matched w=['.+_test\\.py$'], b=None in
directory
> scripts/slave/unittests

Either rename the file, which I recommend, or change the regexp.

[commit-bot: I haz the power, 2012-04-19 20:26:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10035003/14001

[commit-bot: I haz the power, 2012-04-19 20:51:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10035003/17001

[commit-bot: I haz the power, 2012-04-19 20:57:21]

Presubmit check for 10035003-17001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
scripts/slave/unittests/get_swarm_results_test.py failed!
Command scripts/slave/unittests/get_swarm_results_test.py returned non-zero exit
status 1 in /mnt/data/b/commit-queue/workdir/tools/build
Traceback (most recent call last):
  File "scripts/slave/unittests/get_swarm_results_test.py", line 11, in <module>
    from testing_support.super_mox import mox
ImportError: No module named testing_support.super_mox


scripts/slave/unittests/slave_utils_test.py failed!
Command scripts/slave/unittests/slave_utils_test.py returned non-zero exit
status 1 in /mnt/data/b/commit-queue/workdir/tools/build
Traceback (most recent call last):
  File "scripts/slave/unittests/slave_utils_test.py", line 16, in <module>
    import slave.slave_utils as slave_utils
ImportError: No module named slave.slave_utils


Presubmit checks took 347.7s to calculate.

[commit-bot: I haz the power, 2012-04-19 21:05:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10035003/13013

[commit-bot: I haz the power, 2012-04-19 21:15:34]

Presubmit check for 10035003-13013 failed and returned exit status -2001.
The presubmit check was hung. It took 600.1 seconds to execute and the time
limit is 600.0 seconds.

[commit-bot: I haz the power, 2012-04-19 23:45:14]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10035003/13013

[commit-bot: I haz the power, 2012-04-19 23:51:26]

Presubmit check for 10035003-13013 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
scripts/slave/unittests/get_swarm_results_test.py failed!
Command scripts/slave/unittests/get_swarm_results_test.py returned non-zero exit
status 1 in /mnt/data/b/commit-queue/workdir/tools/build
Traceback (most recent call last):
  File "scripts/slave/unittests/get_swarm_results_test.py", line 14, in <module>
    import slave.get_swarm_results as swarm_results
ImportError: No module named slave.get_swarm_results


scripts/slave/unittests/slave_utils_test.py failed!
Command scripts/slave/unittests/slave_utils_test.py returned non-zero exit
status 1 in /mnt/data/b/commit-queue/workdir/tools/build
Traceback (most recent call last):
  File "scripts/slave/unittests/slave_utils_test.py", line 16, in <module>
    import slave.slave_utils as slave_utils
ImportError: No module named slave.slave_utils


Presubmit checks took 366.1s to calculate.

[M-A Ruel, 2012-04-19 23:53:23]

On 2012/04/19 23:51:26, I haz the power (commit-bot) wrote:
>     import slave.get_swarm_results as swarm_results
> ImportError: No module named slave.get_swarm_results

does it properly sys.insert(0,
os.path.join(os.path.dirname(os.path.abspath(__file__)), '..') ?

[commit-bot: I haz the power, 2012-04-20 13:22:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/csharp@chromium.org/10035003/25001

[commit-bot: I haz the power, 2012-04-20 13:27:22]

Change committed as 133190