cloud-init-dev team mailing list archive
-
cloud-init-dev team
-
Mailing list archive
-
Message #02104
Re: [Merge] ~smoser/cloud-init:feature/ds-identify-test into cloud-init:master
Initial comments added
Diff comments:
> diff --git a/tests/unittests/test_ds_identify.py b/tests/unittests/test_ds_identify.py
> new file mode 100644
> index 0000000..00f91f3
> --- /dev/null
> +++ b/tests/unittests/test_ds_identify.py
> @@ -0,0 +1,275 @@
> +# This file is part of cloud-init. See LICENSE file for license information.
> +
> +import copy
> +import os
> +
> +from cloudinit import safeyaml
> +from cloudinit import util
> +from .helpers import CiTestCase, dir2dict, populate_dir
> +
> +UNAME_MYSYS = ("Linux bart 4.4.0-62-generic #83-Ubuntu "
> + "SMP Wed Jan 18 14:10:15 UTC 2017 x86_64 GNU/Linux")
> +BLKID_EFI_ROOT = """
> +DEVNAME=/dev/sda1
> +UUID=8B36-5390
> +TYPE=vfat
> +PARTUUID=30d7c715-a6ae-46ee-b050-afc6467fc452
> +
> +DEVNAME=/dev/sda2
> +UUID=19ac97d5-6973-4193-9a09-2e6bbfa38262
> +TYPE=ext4
> +PARTUUID=30c65c77-e07d-4039-b2fb-88b1fb5fa1fc
> +"""
> +
> +DI_DEFAULT_POLICY = "search,found=all,maybe=all,notfound=enabled"
> +DI_DEFAULT_POLICY_NO_DMI = "search,found=all,maybe=all,notfound=disabled"
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s_out='%(out)s'
> +%(name)s_err='%(err)s'
> +%(name)s_ret='%(ret)s'
> +%(name)s() {
> + local out='%(out)s'
> + local err='%(err)s'
> + local ret='%(ret)s'
> + [ "${%(name)s_out}" = "_unset" ] || echo "${%(name)s_out}"
> + [ "${%(name)s_err}" = "_unset" ] || echo "${%(name)s_err}"
> + return ${%(name)s_ret}
> +}
> +"""
> +
> +SHELL_MOCK_TMPL = """\
> +%(name)s() {
> + local out='%(out)s' err='%(err)s' r='%(ret)s'
> + [ "$out" = "_unset" ] || echo "$out"
> + [ "$err" = "_unset" ] || echo "$err" 2>&1
> + return $r
> +}
> +"""
> +
> +UUIDS = [
Anything special about these 3 uuids or are they just samples you draw from in unit tests? If so, we should just use import uuid; uuid.uuid4() local to the specific unit tests they probably shouldn't be globals.
> + 'e7db1e51-3eff-4ce9-8bfd-e8749884034a',
> + '4167a101-a2fc-4071-a078-f9d89b3c548d',
> + '147c295d-f5f4-4d8b-8b90-14682a6ae072',
> +]
> +
> +RC_FOUND = 0
> +RC_NOT_FOUND = 1
> +
> +P_PRODUCT_NAME = "sys/class/dmi/id/product_name"
Maybe we can add a comment about the P_* section
# Paths to contiguration files
> +P_PRODUCT_SERIAL = "sys/class/dmi/id/product_serial"
> +P_PRODUCT_UUID = "sys/class/dmi/id/product_uuid"
> +P_DSID_CFG = "etc/cloud/ds-identify.cfg"
> +
> +
> +class TestDsIdentify(CiTestCase):
> + dsid_path = os.path.realpath('tools/ds-identify')
> +
> + def call(self, rootd=None, mocks=None, args=None, files=None,
> + policy_dmi=DI_DEFAULT_POLICY,
> + policy_nodmi=DI_DEFAULT_POLICY_NO_DMI):
> + if args is None:
> + args = []
> + if mocks is None:
> + mocks = []
> +
> + if files is None:
> + files = {}
> +
> + if rootd is None:
> + rootd = self.tmp_dir()
> +
> + unset = '_unset'
> + wrap = self.tmp_path(path="_shwrap", dir=rootd)
> + populate_dir(rootd, files)
> +
> + # DI_DEFAULT_POLICY* are declared always as to not rely
> + # on the default in the code. This is because SRU releases change
> + # the value in the code, and thus tests would fail there.
> + head = [
> + "DI_MAIN=noop",
nit: Do we have a quote style preference in coud-init (single or doublequotes)? I'd just like to adhere to one type in general as a general practice, this code switches between either arbitrarily as does the rest of cloud-init.
> + "DEBUG_LEVEL=2",
> + "DI_LOG=stderr",
> + "PATH_ROOT='%s'" % rootd,
> + ". " + self.dsid_path,
> + 'DI_DEFAULT_POLICY="%s"' % policy_dmi,
> + 'DI_DEFAULT_POLICY_NO_DMI="%s"' % policy_nodmi,
> + ""
> + ]
> +
> + def write_mock(data):
> + ddata = {'out': None, 'err': None, 'ret': 0}
> + ddata.update(data)
> + for k in ddata:
> + if ddata[k] is None:
> + ddata[k] = unset
> + return SHELL_MOCK_TMPL % ddata
> +
> + mocklines = []
> + defaults = [
> + {'name': 'detect_virt', 'out': None, 'ret': 1},
> + {'name': 'uname', 'out': UNAME_MYSYS},
> + {'name': 'blkid', 'out': BLKID_EFI_ROOT},
> + ]
> +
> + written = [d['name'] for d in mocks]
> + for data in mocks:
> + mocklines.append(write_mock(data))
> + for d in defaults:
> + if d['name'] not in written:
> + mocklines.append(write_mock(d))
> +
> + endlines = [
> + 'main %s' % ' '.join(['"%s"' % s for s in args])
> + ]
> +
> + with open(wrap, "w") as fp:
> + fp.write('\n'.join(head + mocklines + endlines) + "\n")
> +
> + rc = 0
> + try:
> + out, err = util.subp(['sh', '-c', '. %s' % wrap], capture=True)
> + except util.ProcessExecutionError as e:
> + rc = e.exit_code
> + out = e.stdout
> + err = e.stderr
> +
> + cfg = None
> + cfg_out = os.path.join(rootd, 'run/cloud-init/cloud.cfg')
> + if os.path.exists(cfg_out):
> + cfg = safeyaml.load(util.load_file(cfg_out))
> +
> + return rc, out, err, cfg, dir2dict(rootd)
> +
> + def _call_via_dict(self, data, rootd=None):
> + # return output of self.call with a dict input like VALID_CFG[item]
> + kwargs = {'rootd': rootd}
> + for k in ('mocks', 'args', 'policy_dmi', 'policy_nodmi', 'files'):
> + if k in data:
> + kwargs[k] = data[k]
> + return self.call(**kwargs)
> +
> + def _test_valid(self, name):
> + rc, _out, _err, cfg, _files = self._call_via_dict(
> + copy.deepcopy(VALID_CFG[name]))
> + # print({k: v for k, v in _files.items() if k != "/_shwrap"})
> + self.assertEqual(RC_FOUND, rc)
> + self.assertEqual([VALID_CFG[name]['ds'], 'None'],
> + cfg['datasource_list'])
> +
> + def test_aws_ec2_hvm(self):
> + self._test_valid('Ec2-hvm')
> +
> + def test_aws_ec2_xen(self):
> + # ec2: sys/hypervisor/uuid starts with ec2
> + self._test_valid('Ec2-xen')
> +
> + def test_brightbox_is_ec2(self):
> + # ec2: product_serial ends with brightbox.com
> + self._test_valid('Ec2-brightbox')
> +
> + def test_gce_by_product_name(self):
> + # gce identifies itself with product_name
There are docstrings not comments, so I think they should be """ """ instead of #. Ditto throughout these tests.
> + self._test_valid('GCE')
> +
> + def test_gce_by_serial(self):
> + # older gce compute instances must be identified by serial
> + self._test_valid('GCE-serial')
> +
> + def test_config_drive(self):
> + self._test_valid('ConfigDrive')
> + return
> +
> + def test_policy_disabled(self):
> + tmp_dir = self.tmp_dir()
> + rc, out, err, cfg, _ = self.call(tmp_dir, policy_dmi="disabled")
> + self.assertEqual(RC_NOT_FOUND, 1)
> +
> + def test_policy_config_disable_overrides_builtin(self):
> + mydata = copy.deepcopy(VALID_CFG['Ec2-hvm'])
> + mydata['files'][P_DSID_CFG] = '\n'.join(['policy: disabled', ''])
> + rc, out, err, cfg, files = self._call_via_dict(mydata)
> + self.assertEqual(RC_NOT_FOUND, 1)
> +
> + def test_single_entry_defines_datasource(self):
> + # if config has a single entry in datasource_list it is accepted.
> +
> + # test the valid Ec2-hvm command with the datasource defined in config.
> + mydata = copy.deepcopy(VALID_CFG['Ec2-hvm'])
> + cfgpath = 'etc/cloud/cloud.cfg.d/myds.cfg'
> + mydata['files'][cfgpath] = 'datasource_list: ["NoCloud"]\n'
> + rc, out, err, cfg, files = self._call_via_dict(mydata)
> + self.assertEqual(RC_FOUND, rc)
> + self.assertEqual(['NoCloud', 'None'], cfg['datasource_list'])
> +
> +
> +def blkdev(dev, fs=None, label=None, ptuuid=None, uuid=None):
It doesn't feel at the moment like we are getting a whole lot of value out of the blkdev/blkid_out functions as written. They are maniplating dictionaries to create labels basically.
What do you think about using namedtuple which would give us the ability to create those dictionaries as follows:
BlkDev = namedtuple("BlkDev", "DEVNAME TYPE LABEL PARTUUID UUID")
def blkid_out(disks=None):
"""Return blkid content describing a list of BlkDevs."""
if disks is None:
disks = []
lines = []
for disk in disks:
lines.append([
"{}={}".format(k, v) for k, v in disk._asdict().items() if v])
lines.append("")
return '\n'.join(lines)
Then our ConfigDrive sample valid data would look like:
'out': blkid_out([
BlkDev('/dev/vda1', 'vfat', None, UUIDS[0], None),
BlkDev(
'/dev/vda2', 'ext4', 'cloudimg-rootfs', UUIDS[1], None),
BlkDev('/dev/vdb', 'vfat', 'config-2', None, None)])
> + # a shortcut for writing input to blkid_out.
> + if not dev.startswith("/dev/"):
> + dev = "/dev/" + dev
> +
> + ret = {'DEVNAME': dev}
> + maps = (('TYPE', fs), ('LABEL', label), ('PARTUUID', ptuuid),
> + ('UUID', uuid))
> + for k, v in maps:
> + if v:
> + ret[k] = v
> +
> + return ret
> +
> +
> +def blkid_out(disks=None):
> + if disks is None:
Docstring on any function would be helpful for maintenance so the future us knows what a function/method is without haing to read the method.
"""Generate expected blkid configuration output from a list of BlkDevs"""
> + disks = []
Any objection to earlier return ""? It let's the reader know they don't need to read farther if disks is None.
> + lines = []
> + fields = ("DEVNAME", "UUID", "UUID_SUB", "TYPE", "PARTUUID", "LABEL")
> + for d in disks:
> + for key in [f for f in fields if f in d]:
> + lines.append("%s=%s" % (key, d[key]))
> + lines.append("")
> + return '\n'.join(lines)
> +
> +
> +VALID_CFG = {
This struct feels a bit strange for 2 reasons:
1. globals referenced by a number of unit tests should be declared at the top of a file
2. Unit tests lose readability/maintainability the more external references to external functions, methods or structures on which the test relies.
When I read something like the following as a test maintainer I have to lookup the definition of _test_valid -> _call_via_dict and a VALID_CFG struct.
def test_aws_ec2_hvm(self):
self._test_valid('Ec2-hvm')
Alternatively, I don't have to do as much hopping around to educatate myself if that context is mostly local in the unit test with a docstring:
def test_aws_ec2_hvm(self):
"""Amazon's Hardware VirtualMachine is recognized as EC2 datasource by DMI product serial and UUID."""
config = {
'ds': 'Ec2',
'mocks': [{'name': 'detect_virt', 'out': 'kvm', 'ret': 0}],
'files': {
P_PRODUCT_SERIAL: 'ec23aef5-54be-4843-8d24-8c819f88453e\n',
P_PRODUCT_UUID: 'EC23AEF5-54BE-4843-8D24-8C819F88453E\n',
}
rc, _, _, cfg, _ = self._call_via_dict(config)
self.assertEqual(RC_DATASOURCE_FOUND, rc)
self.assertEqual([VALID_CFG[name]['ds'], 'None'],
cfg['datasource_list'])
Here I need to only lookup _call_via_dict which is called in almost all unit tests so I can get more familiar with that common simple class method.
It also gives be quick access to manipulate a serial or uuid to validate negative match cases via quick cut-n-paste.
> + 'Ec2-hvm': {
> + 'ds': 'Ec2',
> + 'mocks': [{'name': 'detect_virt', 'out': 'kvm', 'ret': 0}],
> + 'files': {
> + P_PRODUCT_SERIAL: 'ec23aef5-54be-4843-8d24-8c819f88453e\n',
> + P_PRODUCT_UUID: 'EC23AEF5-54BE-4843-8D24-8C819F88453E\n',
> + }
> + },
> + 'Ec2-xen': {
> + 'ds': 'Ec2',
> + 'mocks': [{'name': 'detect_virt', 'out': 'xen', 'ret': 0}],
> + 'files': {
> + 'sys/hypervisor/uuid': 'ec2c6e2f-5fac-4fc7-9c82-74127ec14bbb\n'
> + },
> + },
> + 'Ec2-brightbox': {
> + 'ds': 'Ec2',
> + 'files': {P_PRODUCT_SERIAL: 'facc6e2f.brightbox.com\n'},
> + },
> + 'GCE': {
> + 'ds': 'GCE',
> + 'files': {P_PRODUCT_NAME: 'Google Compute Engine\n'},
> + },
> + 'GCE-serial': {
> + 'ds': 'GCE',
> + 'files': {P_PRODUCT_SERIAL: 'GoogleCloud-8f2e88f\n'},
> + },
> + 'ConfigDrive': {
> + 'ds': 'ConfigDrive',
> + 'mocks': [
> + {'name': 'blkid', 'ret': 0,
> + 'out': blkid_out(
> + [blkdev('vda1', fs='vfat', ptuuid=UUIDS[0]),
> + blkdev('vda2', fs='ext4', label='cloudimg-rootfs',
> + ptuuid=UUIDS[1]),
> + blkdev('vdb', fs='vfat', label='config-2')])
> + },
> + ],
> + },
> +}
> +
> +# vi: ts=4 expandtab
--
https://code.launchpad.net/~smoser/cloud-init/+git/cloud-init/+merge/323059
Your team cloud init development team is requested to review the proposed merge of ~smoser/cloud-init:feature/ds-identify-test into cloud-init:master.
References
