Change in vdsm[master]: netinfo: Do not mix local and universal time, improve tests

osvoboda at redhat.com osvoboda at redhat.com
Fri Feb 7 09:51:18 UTC 2014


Ondřej Svoboda has posted comments on this change.

Change subject: netinfo: Do not mix local and universal time, improve tests
......................................................................


Patch Set 3:

(6 comments)

http://gerrit.ovirt.org/#/c/24192/3//COMMIT_MSG
Commit Message:

Line 5: CommitDate: 2014-02-07 03:38:44 +0100
Line 6: 
Line 7: netinfo: Do not mix local and universal time, improve tests
Line 8: 
Line 9: Also go back to using dhclient's --bind-interfaces option
> A commit message with "also" suggests that a patch should be broken into tw
Okay, let us make the el6 patch standalone.
Line 10: to stay compatible with RHEL 6.
Line 11: 
Line 12: Change-Id: I1b2a33209b45001c427d5712d57326394cbeb9d0
Line 13: Bug-Url: https://bugzilla.redhat.com/987813


http://gerrit.ovirt.org/#/c/24192/3/tests/functional/dhcp.py
File tests/functional/dhcp.py:

Line 59:         self.proc.kill()
Line 60:         logging.debug(''.join(self.proc.stderr))
Line 61: 
Line 62: 
Line 63: def runDhclient(interface, confFile, leaseFile, pidFile):
> I appreciate your extensive unit test, but I do not think that we should ex
We are still in the context of tests here :-)

I think it is useful to have functional tests simulate both date-time formats. We catch any future changes (though there should be none!) and more importantly, we mirror static tests in this regard, making sure both tests are correct now.
Line 64:     """Starts dhclient and hands the process over after a while."""
Line 65:     rc, out, err = execCmd([_DHCLIENT_BINARY.cmd, '-lf', leaseFile,
Line 66:                             '-pf', pidFile, '-timeout', str(_DHCLIENT_TIMEOUT),
Line 67:                             '-1', '-cf', confFile, interface])


Line 61: 
Line 62: 
Line 63: def runDhclient(interface, confFile, leaseFile, pidFile):
Line 64:     """Starts dhclient and hands the process over after a while."""
Line 65:     rc, out, err = execCmd([_DHCLIENT_BINARY.cmd, '-lf', leaseFile,
> Sorry, I do not understand this comment or its motivation.
The '-d' switch was completely OK when running dhclient asynchronously and killing it by our means.

By observing this test hang I found out that once dhclient succesfully obtains a lease, it does not terminate itself nowadays. (Thankfully, the output was mirrored to the Journal.)

I am not sure how this behaviour was introduced and quite confident that the test indeed ended successfully before. The debug switch is unneeded and turned rogue by a combination of other parameters or I was blind.
Line 66:                             '-pf', pidFile, '-timeout', str(_DHCLIENT_TIMEOUT),
Line 67:                             '-1', '-cf', confFile, interface])
Line 68: 
Line 69:     if rc:  # == 2


http://gerrit.ovirt.org/#/c/24192/3/tests/functional/networkTests.py
File tests/functional/networkTests.py:

Line 1689:     @permutations([['default'], ['local']])
Line 1690:     @cleanupNet
Line 1691:     @RequireVethMod
Line 1692:     @ValidateRunningAsRoot
Line 1693:     def testDhclientLeases(self, dateFormat=False):
> Why have a default value of "False", when the to valid values is "default" 
The value is indeed an 'old' one and was forgotten.

Would a default argument be needed at all? I can only issue ./run_tests.sh functional/networkTests.py:NetworkTest.testDhclientLeases\(kwargs=\'local\'\), the argument cannot be omitted as far as I can see.

I was confused by testSetupNetworksAddDelBondedNetwork having an obligatory and testSetupNetworksAddOverExistingBond having an optional argument.
Line 1694:         dhcp4 = set()
Line 1695:         with vethIf() as (server, client):
Line 1696:             with avoidAnotherDhclient(client):
Line 1697: 


http://gerrit.ovirt.org/#/c/24192/3/tests/netinfoTests.py
File tests/netinfoTests.py:

Line 296:                 rmtree(tempDir)
Line 297: 
Line 298:     def testGetDhclientIfaces(self):
Line 299:         LEASES = (
Line 300:             'lease {{\n'
> Why is an additional brace is suddenly needed?
Literal braces have to be escaped, otherwise .format() treats them as placeholders to replace.
Line 301:             '  interface "valid";\n'
Line 302:             '  expire {0:%w %Y/%m/%d %H:%M:%S};\n'
Line 303:             '}}\n'
Line 304:             'lease {{\n'


Line 310:             '  expire {2:%w %Y/%m/%d %H:%M:%S};\n'
Line 311:             '}}\n'
Line 312:             'lease {{\n'
Line 313:             '  interface "expired2";\n'
Line 314:             '  expire epoch {3:.0f};\n'
> wouldn't it be nicer to keep a comment (any comment, even a static one)?
Okay, since I caused confusion, this definitely needs a comment, though not inside the string. The comments I removed are not supposed to be parsed by a machine so generating them / leaving them unmodified would be just a different source of confusion.
Line 315:             '}}\n'
Line 316:         )
Line 317: 
Line 318:         with namedTemporaryDir() as tmpDir:


-- 
To view, visit http://gerrit.ovirt.org/24192
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1b2a33209b45001c427d5712d57326394cbeb9d0
Gerrit-PatchSet: 3
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Ondřej Svoboda <osvoboda at redhat.com>
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list