965325 - talos unittests should support remote verification with a dummy agent

Status: RESOLVED WONTFIX

(Testing :: Talos, defect)

Description

[Joel Maher ( :jmaher ) (UTC -8)]

talos unittests which require a device (i.e. perfconfigurator) need to have a reasonable option for a remote device.

Comment 1

[Joel Maher ( :jmaher ) (UTC -8)]

Attachment: talos unittests now have a dummy agent (1.0)

These work great on my local desktop (linux64)- not perfect, but this is a bit step towards more unittests.

Comment 2

[William Lachance (:wlach)]

Comment on attachment 8368177 [details] [diff] [review]
talos unittests now have a dummy agent (1.0)

Review of attachment 8368177 [details] [diff] [review]:
-----------------------------------------------------------------

The idea seems good, but I have some issues with the implementation. Chiefly I am uncomfortable with the design of launching the agent in a seperate process as it makes many things more complicated to debug and understand than the really need to be. I would prefer a simpler implementation of a fake agent which is focused just on what we need to validate the talos code (perhaps use the one in mozdevice for inspiration: https://github.com/mozilla/mozbase/blob/master/mozdevice/tests/sut.py). 

I gave some comments in line but did not do a thorough review. I'd preview to take another look after the architectural issue above is addressed.

Other general nits:

1. We should use 4 space indents. You can do a mass reindent with the reindent package:

https://pypi.python.org/pypi/Reindent/0.1.0

2. Lots of try: except: without specifying exception names here, which can hide/mask all sorts of problems. http://wrla.ch/blog/2012/10/catching-problems-early-with-python/

3. Use mozlog (perhaps the LoggingMixin) for printing debug information. http://mozbase.readthedocs.org/en/latest/mozlog.html#examples

::: tests/test-agent.py
@@ +9,5 @@
> +import optparse
> +import sys
> +import datetime
> +
> +gTestRoot = ''

I don't see why this needs to be a global.

@@ +11,5 @@
> +import datetime
> +
> +gTestRoot = ''
> +
> +class procLauncher(threading.Thread):

First character of classnames should be capitalized. procLauncher -> ProcLauncher. Although, in fact, I don't really see the point of this class. Can you not just use a subprocess.Popen object here?

@@ +166,5 @@
> +    options = ' '.join(parts[1:1+int(num_options)])
> +
> +    values = ''
> +    for part in parts[1+int(num_options):]:
> +      if httpre.search(part) is None:

I don't understand this logic or why it's needed. In general we shouldn't be trying to do extensive validation of input here IMO. We're using mozdevice under the covers which is tested elsewhere.

@@ +332,5 @@
> +      subprocess.Popen(['shudown', '-r', 'now'])
> +    return "rebooting"
> +
> +
> +class HeartBeatHandler(SocketServer.BaseRequestHandler):

This class seems to be unused and I don't see any case where we would want to use it. Could we just take it out?

@@ +386,5 @@
> +  def handle_timeout(self):
> +    self.RequestHandlerClass.timeout = True
> +
> +
> +class AgentOptions(optparse.OptionParser):

Why do we need a new class here? Can you not just add the option you want to an existing OptionParser?

The need for this code should disappear if we don't launch the test agent in a seperate process.

::: tests/test_PerfConfigurator.py
@@ +42,5 @@
> +        self.cmthread.setDaemon(True) # don't hang on quit
> +        self.cmthread.start()
> +
> +    def launchAgent(self):
> +        self.testAgent = talosProcess(['python', 'test-agent.py'], logfile="testagent.log")

I don't see why we should launch this stuff in a seperate process.

Comment 3

[Joel Maher ( :jmaher ) (UTC -8)]

Will,  thanks for taking the time to review this patch.  test-agent.py is an old python agent written in the dark ages- it just worked pretty smoothly for a quick and dirty win here.  You had suggestions of making something simpler- do you have any general thoughts/ideas/direction on that?  

Either I can clean up the test-agent.py or go another route.  Either way the more we can test the stack, the better off we will be in the long term.

Comment 4

[William Lachance (:wlach)]

(In reply to Joel Maher (:jmaher) from comment #3)
> Will,  thanks for taking the time to review this patch.  test-agent.py is an
> old python agent written in the dark ages- it just worked pretty smoothly
> for a quick and dirty win here.  You had suggestions of making something
> simpler- do you have any general thoughts/ideas/direction on that?  

As I mentioned, something like the general pattern we have in the mozdevice tests, where you instantiate a single class to create the mock agent, would be ideal.

m = MockAgent(self, commands=commands)

I think it shouldn't be that difficult to rework your code into a single class this way. We use a slightly different design for the mozdevice tests, but the code should be useful as a model: https://github.com/mozilla/mozbase/blob/master/mozdevice/tests/sut.py 

> Either I can clean up the test-agent.py or go another route.  Either way the
> more we can test the stack, the better off we will be in the long term.

Yep, to be clear I just want to make sure the code is easy to understand and debug for the future. I think the idea itself is great!

Comment 5

[Joel Maher ( :jmaher ) (UTC -8)]

moving the remaining android talos tests to autophone this quarter, autophone is more robust in device management and retrying, most likely we will not see this issue there.