Adding a cross-task eventing server.

Adding a cross-task eventing server.

BUG=

Committed: https://crrev.com/dfe9400af8d372dcf9a8a5b42a23094f66d6eed6
Cr-Commit-Position: refs/heads/master@{#370756}

[Mike Meade, 2016-01-15 01:04:53]

mmeade@chromium.org changed reviewers:
+ maruel@chromium.org

[M-A Ruel, 2016-01-15 14:07:13]

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
File testing/legion/examples/events/controller.py (right):

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
testing/legion/examples/events/controller.py:35: parser.add_argument('--task')
I think it'd be a good idea to have a description

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
testing/legion/examples/events/controller.py:36: parser.add_argument('--os',
default='Ubuntu-14.04')
You should make it a --dimension flag and permits arbitrary dimensions. Just
'os' is not sufficient once in an heterogeneous fleet.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
File testing/legion/examples/events/task.py (right):

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
testing/legion/examples/events/task.py:33: assert Connect('GET',
'/events/event1') == 404
That test is a bit inscrutable.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
testing/legion/examples/events/task.py:34: assert Connect('POST',
'/events/event1') == 200
In the doc, you said PUT, not POST.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/lib/even...
File testing/legion/lib/event_server.py (right):

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/lib/even...
testing/legion/lib/event_server.py:17: PUT       | /events/<NAME> | Sets the
event and returns 200.
You test for POST, not PUT.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/unittest...
File testing/legion/unittests/event_server_test.py (right):

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/unittest...
testing/legion/unittests/event_server_test.py:17: def setUp(self):
super(EventServerTest, self).setUp()

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/unittest...
testing/legion/unittests/event_server_test.py:22: self.server.shutdown()
try:
  self.server.shutdown()
finally:
  super(EventServerTest, self).tearDown()

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/unittest...
testing/legion/unittests/event_server_test.py:26: conn.request(verb, path)
make lines 26-28 a single line

[Mike Meade, 2016-01-18 20:25:51]

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
File testing/legion/examples/events/controller.py (right):

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
testing/legion/examples/events/controller.py:35: parser.add_argument('--task')
On 2016/01/15 14:07:13, M-A Ruel wrote:
> I think it'd be a good idea to have a description

Changed to task-hash to be more descriptive and more inline with other examples.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
testing/legion/examples/events/controller.py:36: parser.add_argument('--os',
default='Ubuntu-14.04')
On 2016/01/15 14:07:13, M-A Ruel wrote:
> You should make it a --dimension flag and permits arbitrary dimensions. Just
> 'os' is not sufficient once in an heterogeneous fleet.

These are defined by the test case author, not the framework. I want to be
fairly simplistic here to allow easier reading for a new test case author since
this is an example case.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
File testing/legion/examples/events/task.py (right):

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
testing/legion/examples/events/task.py:33: assert Connect('GET',
'/events/event1') == 404
On 2016/01/15 14:07:13, M-A Ruel wrote:
> That test is a bit inscrutable.

Done.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/examples...
testing/legion/examples/events/task.py:34: assert Connect('POST',
'/events/event1') == 200
On 2016/01/15 14:07:13, M-A Ruel wrote:
> In the doc, you said PUT, not POST.

Done.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/lib/even...
File testing/legion/lib/event_server.py (right):

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/lib/even...
testing/legion/lib/event_server.py:17: PUT       | /events/<NAME> | Sets the
event and returns 200.
On 2016/01/15 14:07:13, M-A Ruel wrote:
> You test for POST, not PUT.

Done.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/unittest...
File testing/legion/unittests/event_server_test.py (right):

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/unittest...
testing/legion/unittests/event_server_test.py:17: def setUp(self):
On 2016/01/15 14:07:13, M-A Ruel wrote:
> super(EventServerTest, self).setUp()

The super calls really aren't needed for either method since the base class's
setUp and tearDown are no-ops, but I added them anyways.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/unittest...
testing/legion/unittests/event_server_test.py:22: self.server.shutdown()
On 2016/01/15 14:07:13, M-A Ruel wrote:
> try:
>   self.server.shutdown()
> finally:
>   super(EventServerTest, self).tearDown()

Done.

https://codereview.chromium.org/1585373003/diff/20001/testing/legion/unittest...
testing/legion/unittests/event_server_test.py:26: conn.request(verb, path)
On 2016/01/15 14:07:13, M-A Ruel wrote:
> make lines 26-28 a single line

Changed in both locations.

[M-A Ruel, 2016-01-18 21:57:37]

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
File testing/legion/examples/events/controller.py (right):

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
testing/legion/examples/events/controller.py:37: args, _ =
parser.parse_known_args()
why not parse_args()?

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
File testing/legion/examples/events/task.py (right):

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
testing/legion/examples/events/task.py:13: args = None
Make it a class member instead, e.g. around line 17

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
testing/legion/examples/events/task.py:45: args = parser.parse_args()
Actually, you should use parse_known_args() here.

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/unittest...
File testing/legion/unittests/event_server_test.py (right):

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/unittest...
testing/legion/unittests/event_server_test.py:44:
self.assertEquals(self.Connect(verb, '/foobar'), 501)
I would have expected 405. 5xx are usually for server internal errors.

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/unittest...
File testing/legion/unittests/legion_unittest.py (right):

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/unittest...
testing/legion/unittests/legion_unittest.py:31: unittest.main(verbosity=0,
argv=sys.argv[:1])
In events/task.py you use:
 sys.argv[1:] = args.unittest_args

but here you use argv arg. Please settle for one or the other.

[Mike Meade, 2016-01-19 21:53:23]

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
File testing/legion/examples/events/controller.py (right):

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
testing/legion/examples/events/controller.py:37: args, _ =
parser.parse_known_args()
On 2016/01/18 21:57:36, M-A Ruel wrote:
> why not parse_args()?

Because there can be other flags (i.e. flags for the framework) that can also be
present. If there are more args on the command line that there are flags defined
it throws a flag not recognized error. Using parse_known_args lets different
components define and parse their own flags.

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
File testing/legion/examples/events/task.py (right):

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
testing/legion/examples/events/task.py:13: args = None
On 2016/01/18 21:57:37, M-A Ruel wrote:
> Make it a class member instead, e.g. around line 17

Done.

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/examples...
testing/legion/examples/events/task.py:45: args = parser.parse_args()
On 2016/01/18 21:57:37, M-A Ruel wrote:
> Actually, you should use parse_known_args() here.

If I use parse_known_args and no other args are passed in (aside from the ones
we have defined here) then unittest_args isn't defined and we get an
AttributeError when trying to access it. Using parse_args results in an empty
list.

The big problem trying to be solved here is that unittest defines its own flags
and doesn't accept any other args on the command line. Passing additional args
on the command line results in unknown cmd line arg error. So we are parsing,
pulling out address and port, and passing all of the other args to unittest via
unittest_args and sys.arg.

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/unittest...
File testing/legion/unittests/event_server_test.py (right):

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/unittest...
testing/legion/unittests/event_server_test.py:44:
self.assertEquals(self.Connect(verb, '/foobar'), 501)
On 2016/01/18 21:57:37, M-A Ruel wrote:
> I would have expected 405. 5xx are usually for server internal errors.

That is a better error. Thanks, fixed.

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/unittest...
File testing/legion/unittests/legion_unittest.py (right):

https://codereview.chromium.org/1585373003/diff/40001/testing/legion/unittest...
testing/legion/unittests/legion_unittest.py:31: unittest.main(verbosity=0,
argv=sys.argv[:1])
On 2016/01/18 21:57:37, M-A Ruel wrote:
> In events/task.py you use:
>  sys.argv[1:] = args.unittest_args
> 
> but here you use argv arg. Please settle for one or the other.

Done.

[Mike Meade, 2016-01-21 17:49:05]

PTAL

Note that one or two responses may be a little confusing. I responded and then
tried out one or two alternatives and uploaded them. I'm not able to go back and
edit my responses. Hopefully on those the code is clear.

[M-A Ruel, 2016-01-21 17:57:05]

lgtm

[Mike Meade, 2016-01-21 18:20:24]

The CQ bit was checked by mmeade@chromium.org

[commit-bot: I haz the power, 2016-01-21 18:20:46]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1585373003/80001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1585373003/80001

[commit-bot: I haz the power, 2016-01-21 19:46:55]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2016-01-21 19:48:15]

Description was changed from

==========
Adding a cross-task eventing server.

BUG=
==========

to

==========
Adding a cross-task eventing server.

BUG=

Committed: https://crrev.com/dfe9400af8d372dcf9a8a5b42a23094f66d6eed6
Cr-Commit-Position: refs/heads/master@{#370756}
==========

[commit-bot: I haz the power, 2016-01-21 19:48:16]

Patchset 5 (id:??) landed as
https://crrev.com/dfe9400af8d372dcf9a8a5b42a23094f66d6eed6
Cr-Commit-Position: refs/heads/master@{#370756}