automated WiFi test framework and start of tests

automated WiFi test framework and start of tests

[Sam Leffler, 2010-03-04 23:59:25]

very early WIP; uploading for feedback; to run this stuff you need special gear
and changes not yet committed

[petkov, 2010-03-05 18:21:18]

Some quick comments.

http://codereview.chromium.org/669118/diff/1/2
File server/bsd_router.py (right):

http://codereview.chromium.org/669118/diff/1/2#newcode1
server/bsd_router.py:1: 
Add Chromium Copyright -- here and everywhere.

http://codereview.chromium.org/669118/diff/1/2#newcode1
server/bsd_router.py:1: 
Prefix this file with site_ to indicate that this is not part of the upstream
repo. Same with the other files directly under server/. Unless you're planning
to upstream?

Does this file really need to be here -- or does it belong to the test folder?

http://codereview.chromium.org/669118/diff/1/2#newcode21
server/bsd_router.py:21: def create(self, params):
Local CODING_STYLE 2 empty lines between methods -- here and everywhere.

http://codereview.chromium.org/669118/diff/1/2#newcode26
server/bsd_router.py:26: self.router.run("ifconfig wlan0 create wlandev %s
wlanmode %s" % \
I don't think you need \ here and below (I may be wrong).

http://codereview.chromium.org/669118/diff/1/2#newcode39
server/bsd_router.py:39: self.router.run("ifconfig %s destroy" % self.bridgeif,
ignore_status=True)
80 chars

http://codereview.chromium.org/669118/diff/1/2#newcode42
server/bsd_router.py:42: for (k,v) in params.items():
space after ,

http://codereview.chromium.org/669118/diff/1/2#newcode63
server/bsd_router.py:63: for (k,v) in params.items():
space after ,

http://codereview.chromium.org/669118/diff/1/2#newcode102
server/bsd_router.py:102: self.router.run("killall hostapd >/dev/null 2>&1")
killall is not on the latest image I think -- use pkill instead?

http://codereview.chromium.org/669118/diff/1/60
File server/site_tests/network_WiFiSecMat/control (right):

http://codereview.chromium.org/669118/diff/1/60#newcode5
server/site_tests/network_WiFiSecMat/control:5: TEST_CLASS = "Hardware"
Use focus area as TEST_CLASS -- network

http://codereview.chromium.org/669118/diff/1/60#newcode25
server/site_tests/network_WiFiSecMat/control:25: router =
hosts.create_host("172.22.70.12")
Add a TODO that these IPs need to be parameters?

http://codereview.chromium.org/669118/diff/1/60#newcode26
server/site_tests/network_WiFiSecMat/control:26: router.phydev = "ath0"      #
XXX
Use TODO(sleffler), in general, instead of XXX.

http://codereview.chromium.org/669118/diff/1/60#newcode32
server/site_tests/network_WiFiSecMat/control:32: print "Client %s, Server %s, AP
%s\n" \
Use logging.info instead

http://codereview.chromium.org/669118/diff/1/61
File server/wifitest.py (right):

http://codereview.chromium.org/669118/diff/1/61#newcode1
server/wifitest.py:1: import common, fnmatch, logging, os, re, string
Does this file belong to the WiFiSecMat test folder?

http://codereview.chromium.org/669118/diff/1/61#newcode30
server/wifitest.py:30: print "%s: %s" % (os.path.join(dir, file), str(e))
logging. No need to "str(e)" -- just "e"

http://codereview.chromium.org/669118/diff/1/61#newcode105
server/wifitest.py:105: print "%s: step '%s' params %s" % (self.name, method,
params)
logging

http://codereview.chromium.org/669118/diff/1/61#newcode108
server/wifitest.py:108: eval("self.%s(params)" % method)
Ugh, this looks convoluted. Use getattr instead?

Something like:

func = getattr(self, method, None)
if func is None:
    func = getattr(self.wifi, method, None)
if func is None:
    error
else:
    func(params)

http://codereview.chromium.org/669118/diff/1/61#newcode118
server/wifitest.py:118: print "%s: Step '%s' failed: %s; abort test" % \
logging

http://codereview.chromium.org/669118/diff/1/61#newcode124
server/wifitest.py:124: return '\
This looks ugly -- any way it can live in a separate file?

http://codereview.chromium.org/669118/diff/1/61#newcode263
server/wifitest.py:263: m = re.search('([0-9]*) packets transmitted,[
]*([0-9]*)[ ]received, ([0-9]*)', str)
80 chars

http://codereview.chromium.org/669118/diff/1/61#newcode273
server/wifitest.py:273: return stats
You should probably log perf keyvals for the key stats.

http://codereview.chromium.org/669118/diff/1/61#newcode277
server/wifitest.py:277: (self.name, label, stats['xmit'], stats['recv'],
stats['loss'],
logging

[Sam Leffler, 2010-03-12 19:51:01]

PTAL.  Think I addressed most of your comments.  Still need to make a sweep
through for keyvals and to cleanup the inline escaped python I emit via ssh.

This version has more stuff filled in but is still incomplete.  I think it's
time to push it into the tree so it can start to be used and so others can pitch
in to fix+expand tests.

http://codereview.chromium.org/669118/diff/1/2
File server/bsd_router.py (right):

http://codereview.chromium.org/669118/diff/1/2#newcode1
server/bsd_router.py:1: 
site_ prefix added.  I have no plan to upstream just yet; these are currently
chrome os specific.  The files are shared by the two suites of tests included in
this CL and will be shared by more suites forthcoming.

http://codereview.chromium.org/669118/diff/1/2#newcode1
server/bsd_router.py:1: 
Done.

http://codereview.chromium.org/669118/diff/1/2#newcode21
server/bsd_router.py:21: def create(self, params):
On 2010/03/05 18:21:18, petkov wrote:
> Local CODING_STYLE 2 empty lines between methods -- here and everywhere.

Done.

http://codereview.chromium.org/669118/diff/1/2#newcode39
server/bsd_router.py:39: self.router.run("ifconfig %s destroy" % self.bridgeif,
ignore_status=True)
On 2010/03/05 18:21:18, petkov wrote:
> 80 chars

Done.

http://codereview.chromium.org/669118/diff/1/2#newcode42
server/bsd_router.py:42: for (k,v) in params.items():
On 2010/03/05 18:21:18, petkov wrote:
> space after ,

Done.

http://codereview.chromium.org/669118/diff/1/2#newcode63
server/bsd_router.py:63: for (k,v) in params.items():
On 2010/03/05 18:21:18, petkov wrote:
> space after ,

Done.

http://codereview.chromium.org/669118/diff/1/2#newcode102
server/bsd_router.py:102: self.router.run("killall hostapd >/dev/null 2>&1")
This is on a bsd system.  But I changed to pkill anyway.

http://codereview.chromium.org/669118/diff/1/60
File server/site_tests/network_WiFiSecMat/control (right):

http://codereview.chromium.org/669118/diff/1/60#newcode5
server/site_tests/network_WiFiSecMat/control:5: TEST_CLASS = "Hardware"
On 2010/03/05 18:21:18, petkov wrote:
> Use focus area as TEST_CLASS -- network

Done.

http://codereview.chromium.org/669118/diff/1/60#newcode25
server/site_tests/network_WiFiSecMat/control:25: router =
hosts.create_host("172.22.70.12")
Most of this file needs proper hookup to autotest; what's here is just to allow
testing.

http://codereview.chromium.org/669118/diff/1/60#newcode32
server/site_tests/network_WiFiSecMat/control:32: print "Client %s, Server %s, AP
%s\n" \
On 2010/03/05 18:21:18, petkov wrote:
> Use logging.info instead

Done.

http://codereview.chromium.org/669118/diff/1/61
File server/wifitest.py (right):

http://codereview.chromium.org/669118/diff/1/61#newcode1
server/wifitest.py:1: import common, fnmatch, logging, os, re, string
No, it's shared by all wifi tests.

http://codereview.chromium.org/669118/diff/1/61#newcode105
server/wifitest.py:105: print "%s: step '%s' params %s" % (self.name, method,
params)
On 2010/03/05 18:21:18, petkov wrote:
> logging

Done.

http://codereview.chromium.org/669118/diff/1/61#newcode105
server/wifitest.py:105: print "%s: step '%s' params %s" % (self.name, method,
params)
On 2010/03/05 18:21:18, petkov wrote:
> logging

Done.

http://codereview.chromium.org/669118/diff/1/61#newcode108
server/wifitest.py:108: eval("self.%s(params)" % method)
Rewritten to use getattr; thank you!

http://codereview.chromium.org/669118/diff/1/61#newcode118
server/wifitest.py:118: print "%s: Step '%s' failed: %s; abort test" % \
On 2010/03/05 18:21:18, petkov wrote:
> logging

Done.

http://codereview.chromium.org/669118/diff/1/61#newcode124
server/wifitest.py:124: return '\
Don't know.  I was going to make it a separate client test and invoke with
job.run but couldn't get it to work.  I'm happy to take guidance on how to
better structure this (just learning python)

http://codereview.chromium.org/669118/diff/1/61#newcode263
server/wifitest.py:263: m = re.search('([0-9]*) packets transmitted,[
]*([0-9]*)[ ]received, ([0-9]*)', str)
On 2010/03/05 18:21:18, petkov wrote:
> 80 chars

Done.

http://codereview.chromium.org/669118/diff/1/61#newcode273
server/wifitest.py:273: return stats
Yes, there's a bunch keyvals I need to record and also threshold values based on
qualification requirements.  This is/was an early WIP so looking more for
guidance on general approach and/or structure of the code (e.g. class
relationships)

http://codereview.chromium.org/669118/diff/1/61#newcode277
server/wifitest.py:277: (self.name, label, stats['xmit'], stats['recv'],
stats['loss'],
On 2010/03/05 18:21:18, petkov wrote:
> logging

Done.

[petkov, 2010-03-15 17:38:08]

Adding ericli to review the global_config.ini change.

Mostly style nits, otherwise.

I'm still not sure that the right place for the wifitest and the router classes
are but I don't have any better suggestions. Adding mbligh -- it seems that we
should have something like server/deps? I guess the CL can go in as is and be
cleaned up later.

http://codereview.chromium.org/669118/diff/5001/6001
File client/config/wifi_router_list (right):

http://codereview.chromium.org/669118/diff/5001/6001#newcode1
client/config/wifi_router_list:1: #
Copyright?

http://codereview.chromium.org/669118/diff/5001/6002
File client/config/wifi_testbed_config (right):

http://codereview.chromium.org/669118/diff/5001/6002#newcode1
client/config/wifi_testbed_config:1: #
Copyright?

http://codereview.chromium.org/669118/diff/5001/6003
File global_config.ini (right):

http://codereview.chromium.org/669118/diff/5001/6003#newcode94
global_config.ini:94: client_autodir_paths: /usr/local/autotest,/home/autotest
Why do you need this change? I'm not sure what the implications are. I think the
original change was put in by ericli, so adding him to the reviewers...

http://codereview.chromium.org/669118/diff/5001/6004
File server/site_bsd_router.py (right):

http://codereview.chromium.org/669118/diff/5001/6004#newcode29
server/site_bsd_router.py:29: # TODO(sleffler) default to 1st available wireless
nic
indent for the comment seems off -- same  below

http://codereview.chromium.org/669118/diff/5001/6004#newcode50
server/site_bsd_router.py:50: #        self.router.run("ifconfig wlan0 destroy
>/dev/null 2>&1",
Do you need the commented out code? Here and below...

http://codereview.chromium.org/669118/diff/5001/6005
File server/site_linux_router.py (right):

http://codereview.chromium.org/669118/diff/5001/6005#newcode19
server/site_linux_router.py:19: # TODO(sleffler) default to 1st available
wireless nic
indent seems off

http://codereview.chromium.org/669118/diff/5001/6005#newcode123
server/site_linux_router.py:123: # XXX no support                elif k ==
'rifs':
Maybe make this explicit in the code by raising an exception?

http://codereview.chromium.org/669118/diff/5001/6005#newcode141
server/site_linux_router.py:141: #        else:
No need for this comment -- i.e., fold the else into the comment describing what
you would do in the else?

http://codereview.chromium.org/669118/diff/5001/6062
File server/site_tests/network_WiFiSecMat/control (right):

http://codereview.chromium.org/669118/diff/5001/6062#newcode47
server/site_tests/network_WiFiSecMat/control:47: #tests =
site_wifitest.read_tests(dir, '018CheckWPA_CounterMeasures')
Do you intend to check these comments in?

http://codereview.chromium.org/669118/diff/5001/6063
File server/site_wifitest.py (right):

http://codereview.chromium.org/669118/diff/5001/6063#newcode11
server/site_wifitest.py:11: class PingThread(threading.Thread):
It seems there's nothing ping-related in the PingThread class :-)

http://codereview.chromium.org/669118/diff/5001/6063#newcode136
server/site_wifitest.py:136: return '\
Yes, it would be good if you could clean this up...

http://codereview.chromium.org/669118/diff/5001/6063#newcode252
server/site_wifitest.py:252: #        script =
self.__make_disconnect_script(params)
Remove comments?

[ericli, 2010-03-15 18:39:27]

http://codereview.chromium.org/669118/diff/5001/6003
File global_config.ini (right):

http://codereview.chromium.org/669118/diff/5001/6003#newcode94
global_config.ini:94: client_autodir_paths: /usr/local/autotest,/home/autotest
I had an impression /usr/local is on readonly partition so you could not write
anything there.

/home/autotest is on stateful partition which is fully writable.


On 2010/03/15 17:38:08, petkov wrote:
> Why do you need this change? I'm not sure what the implications are. I think
the
> original change was put in by ericli, so adding him to the reviewers...

[Sam Leffler, 2010-03-15 19:09:28]

http://codereview.chromium.org/669118/diff/5001/6001
File client/config/wifi_router_list (right):

http://codereview.chromium.org/669118/diff/5001/6001#newcode1
client/config/wifi_router_list:1: #
On 2010/03/15 17:38:08, petkov wrote:
> Copyright?

Done.

http://codereview.chromium.org/669118/diff/5001/6002
File client/config/wifi_testbed_config (right):

http://codereview.chromium.org/669118/diff/5001/6002#newcode1
client/config/wifi_testbed_config:1: #
On 2010/03/15 17:38:08, petkov wrote:
> Copyright?

Done.

http://codereview.chromium.org/669118/diff/5001/6003
File global_config.ini (right):

http://codereview.chromium.org/669118/diff/5001/6003#newcode94
global_config.ini:94: client_autodir_paths: /usr/local/autotest,/home/autotest
The issue is on non-chromeos machines /home is typically nfs-mounted and
unwritable.  I've reversed the order so we try /home/autotest then fallback to
/usr/local.

http://codereview.chromium.org/669118/diff/5001/6004
File server/site_bsd_router.py (right):

http://codereview.chromium.org/669118/diff/5001/6004#newcode29
server/site_bsd_router.py:29: # TODO(sleffler) default to 1st available wireless
nic
On 2010/03/15 17:38:08, petkov wrote:
> indent for the comment seems off -- same  below

Done.

http://codereview.chromium.org/669118/diff/5001/6004#newcode50
server/site_bsd_router.py:50: #        self.router.run("ifconfig wlan0 destroy
>/dev/null 2>&1",
On 2010/03/15 17:38:08, petkov wrote:
> Do you need the commented out code? Here and below...

Done.

http://codereview.chromium.org/669118/diff/5001/6005
File server/site_linux_router.py (right):

http://codereview.chromium.org/669118/diff/5001/6005#newcode19
server/site_linux_router.py:19: # TODO(sleffler) default to 1st available
wireless nic
On 2010/03/15 17:38:08, petkov wrote:
> indent seems off

Done.

http://codereview.chromium.org/669118/diff/5001/6005#newcode123
server/site_linux_router.py:123: # XXX no support                elif k ==
'rifs':
This file is actually just a placeholder for working code.  I'm waiting on a
linux wifi router I can debug with.

[mbligh, 2010-03-15 19:33:12]

http://codereview.chromium.org/669118/diff/19001/20063
File server/site_wifitest.py (right):

http://codereview.chromium.org/669118/diff/19001/20063#newcode143
server/site_wifitest.py:143: def __get_connect_script(self, params):
Wouldn't this be much cleaner as a triple-quoted string?
(or separate data file).

[petkov, 2010-03-15 19:46:36]

LGTM, assuming you're planning to cleanup the escaped stuff in a separate CL.

Nice!

[ericli, 2010-03-15 19:50:39]

server/site_wifitest.py should be put under serve/site_test/wifitest/wifitest.py


For global_config.ini, I guess the non-chromeos machine you referred is the
server host from your control file. Is it possible to use another chromeos host
as server? so both server and client are chromeosos netbook.

client/config/wifi_router_list
client/config/wifi_testbed_config

I am not sure checkin hard coded ip address here is a good idea. At least should
provide a way for people to override. I am think:
1. I will run the same set of tests on the same network as you.
2. we need to tell external users to run this test, and most likely they need to
adjust the ips.

But that might be out of the scope of this CL.






On 2010/03/15 19:33:12, mbligh wrote:
> http://codereview.chromium.org/669118/diff/19001/20063
> File server/site_wifitest.py (right):
> 
> http://codereview.chromium.org/669118/diff/19001/20063#newcode143
> server/site_wifitest.py:143: def __get_connect_script(self, params):
> Wouldn't this be much cleaner as a triple-quoted string?
> (or separate data file).

[Sam Leffler, 2010-03-15 19:57:17]

I need at least 3 devices to run these tests: client (chrome os),
router, and server.  The existing scripts/infrastructure either do not
support this (run_remote_tests) or make it painful to configure.
Hence the hardcoded config file that at least extracts the data from
the scripts.  I'm trying to get this stuff into the tree so others can
work on it.  If you see ways to make this better let's get it
committed and then you can change it in a separate CL.

-Sam

On Mon, Mar 15, 2010 at 11:50 AM,  <ericli@chromium.org> wrote:
> server/site_wifitest.py should be put under
> serve/site_test/wifitest/wifitest.py
>
>
> For global_config.ini, I guess the non-chromeos machine you referred is the
> server host from your control file. Is it possible to use another chromeos
> host
> as server? so both server and client are chromeosos netbook.
>
> client/config/wifi_router_list
> client/config/wifi_testbed_config
>
> I am not sure checkin hard coded ip address here is a good idea. At least
> should
> provide a way for people to override. I am think:
> 1. I will run the same set of tests on the same network as you.
> 2. we need to tell external users to run this test, and most likely they
> need to
> adjust the ips.
>
> But that might be out of the scope of this CL.
>
>
>
>
>
>
> On 2010/03/15 19:33:12, mbligh wrote:
>>
>> http://codereview.chromium.org/669118/diff/19001/20063
>> File server/site_wifitest.py (right):
>
>> http://codereview.chromium.org/669118/diff/19001/20063#newcode143
>> server/site_wifitest.py:143: def __get_connect_script(self, params):
>> Wouldn't this be much cleaner as a triple-quoted string?
>> (or separate data file).
>
>
>
> http://codereview.chromium.org/669118
>

[Sam Leffler, 2010-03-15 19:59:39]

I will do more cleanups in a separate CL.  Every change of this sorts
pushes back commit which will enable others to work on the code.

-Sam

On Mon, Mar 15, 2010 at 11:33 AM,  <mbligh@chromium.org> wrote:
>
> http://codereview.chromium.org/669118/diff/19001/20063
> File server/site_wifitest.py (right):
>
> http://codereview.chromium.org/669118/diff/19001/20063#newcode143
> server/site_wifitest.py:143: def __get_connect_script(self, params):
> Wouldn't this be much cleaner as a triple-quoted string?
> (or separate data file).
>
> http://codereview.chromium.org/669118
>

[ericli, 2010-03-15 20:00:53]

agree.

On Mon, Mar 15, 2010 at 12:57 PM, Sam Leffler <sleffler@chromium.org> wrote:

> I need at least 3 devices to run these tests: client (chrome os),
> router, and server.  The existing scripts/infrastructure either do not
> support this (run_remote_tests)


We are aware of this. And in discussing to improve it.


> or make it painful to configure.
> Hence the hardcoded config file that at least extracts the data from
> the scripts.  I'm trying to get this stuff into the tree so others can
> work on it.  If you see ways to make this better let's get it
> committed and then you can change it in a separate CL.
>
> -Sam
>
> On Mon, Mar 15, 2010 at 11:50 AM,  <ericli@chromium.org> wrote:
> > server/site_wifitest.py should be put under
> > serve/site_test/wifitest/wifitest.py
> >
> >
> > For global_config.ini, I guess the non-chromeos machine you referred is
> the
> > server host from your control file. Is it possible to use another
> chromeos
> > host
> > as server? so both server and client are chromeosos netbook.
> >
> > client/config/wifi_router_list
> > client/config/wifi_testbed_config
> >
> > I am not sure checkin hard coded ip address here is a good idea. At least
> > should
> > provide a way for people to override. I am think:
> > 1. I will run the same set of tests on the same network as you.
> > 2. we need to tell external users to run this test, and most likely they
> > need to
> > adjust the ips.
> >
> > But that might be out of the scope of this CL.
> >
> >
> >
> >
> >
> >
> > On 2010/03/15 19:33:12, mbligh wrote:
> >>
> >> http://codereview.chromium.org/669118/diff/19001/20063
> >> File server/site_wifitest.py (right):
> >
> >> http://codereview.chromium.org/669118/diff/19001/20063#newcode143
> >> server/site_wifitest.py:143: def __get_connect_script(self, params):
> >> Wouldn't this be much cleaner as a triple-quoted string?
> >> (or separate data file).
> >
> >
> >
> > http://codereview.chromium.org/669118
> >
>



-- 
Eric Li
李咏竹
Google Kirkland

[Sam Leffler, 2010-03-15 23:49:29]

I ended up pushing the CL w/o the change to global_config.ini.  We may
change the setup to eliminate the "server" machine in which case that
change may not be needed.

I'll follow up w/ additional CL's for the other cleanups requested.

-Sam

On Mon, Mar 15, 2010 at 8:00 PM, Eric Li(李咏竹) <ericli@chromium.org> wrote:
> agree.
>
> On Mon, Mar 15, 2010 at 12:57 PM, Sam Leffler <sleffler@chromium.org> wrote:
>>
>> I need at least 3 devices to run these tests: client (chrome os),
>> router, and server.  The existing scripts/infrastructure either do not
>> support this (run_remote_tests)
>
> We are aware of this. And in discussing to improve it.
>
>>
>> or make it painful to configure.
>> Hence the hardcoded config file that at least extracts the data from
>> the scripts.  I'm trying to get this stuff into the tree so others can
>> work on it.  If you see ways to make this better let's get it
>> committed and then you can change it in a separate CL.
>>
>> -Sam
>>
>> On Mon, Mar 15, 2010 at 11:50 AM,  <ericli@chromium.org> wrote:
>> > server/site_wifitest.py should be put under
>> > serve/site_test/wifitest/wifitest.py
>> >
>> >
>> > For global_config.ini, I guess the non-chromeos machine you referred is
>> > the
>> > server host from your control file. Is it possible to use another
>> > chromeos
>> > host
>> > as server? so both server and client are chromeosos netbook.
>> >
>> > client/config/wifi_router_list
>> > client/config/wifi_testbed_config
>> >
>> > I am not sure checkin hard coded ip address here is a good idea. At
>> > least
>> > should
>> > provide a way for people to override. I am think:
>> > 1. I will run the same set of tests on the same network as you.
>> > 2. we need to tell external users to run this test, and most likely they
>> > need to
>> > adjust the ips.
>> >
>> > But that might be out of the scope of this CL.
>> >
>> >
>> >
>> >
>> >
>> >
>> > On 2010/03/15 19:33:12, mbligh wrote:
>> >>
>> >> http://codereview.chromium.org/669118/diff/19001/20063
>> >> File server/site_wifitest.py (right):
>> >
>> >> http://codereview.chromium.org/669118/diff/19001/20063#newcode143
>> >> server/site_wifitest.py:143: def __get_connect_script(self, params):
>> >> Wouldn't this be much cleaner as a triple-quoted string?
>> >> (or separate data file).
>> >
>> >
>> >
>> > http://codereview.chromium.org/669118
>> >
>
>
>
> --
> Eric Li
> 李咏竹
> Google Kirkland
>
>
>