[PATCH] create python test for samba-tool time command

[Sean Dague]

On 10/23/2011 06:12 PM, Jelmer Vernooij wrote:
> This file lacks a copyright header - does the source test pass for you?

Source test? Is there something I should be running here?

>> +# # domain stuff
>> +# "DOMAIN",
>> +# "REALM",
>> +
>> +# # domain controller stuff
>> +# "DC_SERVER",
>> +# "DC_SERVER_IP",
>> +# "DC_NETBIOSNAME",
>> +# "DC_NETBIOSALIAS",
>> +
>> +# # domain member
>> +# "MEMBER_SERVER",
>> +# "MEMBER_SERVER_IP",
>> +# "MEMBER_NETBIOSNAME",
>> +# "MEMBER_NETBIOSALIAS",
>> +
>> +# # rpc proxy controller stuff
>> +# "RPC_PROXY_SERVER",
>> +# "RPC_PROXY_SERVER_IP",
>> +# "RPC_PROXY_NETBIOSNAME",
>> +# "RPC_PROXY_NETBIOSALIAS",
>> +
>> +# # domain controller stuff for Vampired DC
>> +# "VAMPIRE_DC_SERVER",
>> +# "VAMPIRE_DC_SERVER_IP",
>> +# "VAMPIRE_DC_NETBIOSNAME",
>> +# "VAMPIRE_DC_NETBIOSALIAS",
>> +
>> +# # server stuff
>> +# "SERVER",
>> +# "SERVER_IP",
>> +# "NETBIOSNAME",
>> +# "NETBIOSALIAS",
>> +
>> +# # user stuff
>> +# "USERNAME",
>> +# "USERID",
>> +# "PASSWORD",
>> +# "DC_USERNAME",
>> +# "DC_PASSWORD",
>> +
>> +# # misc stuff
>> +# "KRB5_CONFIG",
>> +# "WINBINDD_SOCKET_DIR",
>> +# "WINBINDD_PRIV_PIPE_DIR",
>> +# "NMBD_SOCKET_DIR",
>> +# "LOCAL_PATH"
>> +
>> +# These can all be accesses via os.environ["VARIBLENAME"] when needed
> s/VARIBLENAME/VARIABLENAME/
>
> Rather than duplicating this list here, it would probably be better to
> point at the master list of the variables. The risk of duplicating it is
> that it will get out of date when the main list gets changed.

Is there a master list somewhere? I just went and found it in 
selftest.pl, and it seemed confusing to tell folks to read a perl file 
when writing a python test, especially if they weren't heavily 
experienced with both.

>> +import os
>> +import sys
>> +from cStringIO import StringIO
>> +from samba.netcmd.main import cmd_sambatool
>> +import samba.tests
>> +
>> +class SambaToolCmdTest(samba.tests.TestCase):
>> +
>> + def runcmd(self, name, *args):
>> + cmd = cmd_sambatool.subcommands[name]
>> + cmd.outf = StringIO()
>> + cmd.errf = StringIO()
>> + result = cmd._run(name, *args)
>> + return (result, cmd.outf.getvalue(), cmd.errf.getvalue())
> This duplicates code from samba.tests.netcmd - it would be nicer to
> derive from the class there instead.

There are some similarities, but the calling interface is different 
enough that I think for now it warants being on it's own. That becomes 
especially true when dealing with commands with subcommands, as you 
actually need to reach down another layer otherwise the StringIO 
redirection doesn't work. I found that out the hard way when working on 
user tests.

Also, calling by class might make sense for the iteration commands, but 
when trying to write tests for commands, doing calling those by string 
(i.e. self.runsubcmd("user", "add", .....) ends up being a bit more clear.

If you really want to make them based off the same base class, I think 
we should move netcmd into samba_tool directory.

>> +
>> + def assertCmdSuccess(self, val, msg=""):
>> + self.assertIsNone(val, msg)
>> +
>> + def assertCmdFail(self, val, msg=""):
>> + self.assertIsNotNone(val, msg)
> These methods seem pretty useless given output and stderr are never
> going to be None, they'll always be a string.

result is currently None if success, and a number (usually -1) if fail, 
though I'm not sure that will always be the case. I added them in mostly 
to make it clear in the test what was being tested for.

>> diff --git a/source4/scripting/python/samba/tests/samba_tool/timecmd.py
> b/source4/scripting/python/samba/tests/samba_tool/timecmd.py
>> new file mode 100644
>> index 0000000..2de9685
>> --- /dev/null
>> +++ b/source4/scripting/python/samba/tests/samba_tool/timecmd.py
>> +
>> +class TimeCmdTestCase(SambaToolCmdTest):
>> + """Tests for samba-tool time subcommands"""
>> +
>> + def test_timeget(self):
>> + """Run time against the server and make sure it looks accurate"""
>> + (result, out, err) = self.runcmd("time", os.environ["SERVER"])
>> + self.assertCmdSuccess(result, "Ensuring time ran successfully")
>> +
>> + timefmt = strptime(out, "%a %b %d %H:%M:%S %Y %Z\n")
>> + servertime = int(mktime(timefmt))
>> + now = int(mktime(localtime()))
>> +
>> + # because there is a race here, allow up to 5 seconds difference in times
>> + delta = 5
>> + self.assertTrue((servertime>  (now - delta) and (servertime<  (now +
> delta)), "Time is now"))
>> +
>> + def test_timefail(self):
>> + """Run time against a non-existant server, and make sure it fails"""
>> + try:
>> + (result, out, err) = self.runcmd("time", "notaserver")
>> + self.assertCmdFail(result, "Ensuring time failed as expected")
>> + except:
>> + self.assertTrue(True, "Failure caused an exception, ok and expected
> for now")
> except: (rather than e.g. "except Exception:") is evil (it can "eat"
> exceptions like KeyboardInterrupt). Rather than using this, it's nicer
> and easier to use self.assertRaises, e.g.
>
> self.assertRaises(self.runcmd, "time", "notaserver")

Agreed. I will update with that.

Do you want me to respin what's here? Or is this going to be applied and 
we'll build off of it?

	-Sean

-- 
Sean Dague
IBM Linux Technology Center
email: sldague at us.ibm.com
alt-email: sdague at linux.vnet.ibm.com