Code review comment for lp:~fcorrea/landscape-client/add-vminfo-to-computer-info-message

What Thomas said (especially about testing) plus some other comments.

Just to elaborate on test coverage thing, a rule of thumb is that any change in the code (like removing a line, an if branch, etc.) should result in a test failure. In this case replacing the entire body of ComputerInfo._get_vm_info with a simple 'return ""' doesn't make any tests fail, which means that its behavior is untested.

[1]

+ def _get_vm_info(self, root_path="/"):

Please pass root_path to the constructor instead, like:

    def __init__(self, get_fqdn=get_fqdn,
                 meminfo_file="/proc/meminfo",
                 lsb_release_filename=LSB_RELEASE_FILENAME,
                 root_path="/"):
        self._get_fqdn = get_fqdn
        self._meminfo_file = meminfo_file
        self._lsb_release_filename = lsb_release_filename
        self._root_path = root_path

and then you can have something like

    root_path = self.makeDir()
    plugin = ComputerInfo(root_path=root_path)

in your vm-info-related tests, where the test populates root_path by hand for checking the behavior of the method.

To avoid cluttering the constructor with arguments that are only meaningful for tests, I think it would be better to use class variables that one can override in the tests:

class ComputerInfo(MonitorPlugin):

    get_fqdn = get_fqdn
    meminfo_file = "/proc/meminfo"
    lsb_release_filename = LSB_RELEASE_FILENAME
    root_path = "/"

and then

    plugin = ComputerInfo()
    plugin.root_path = self.makeDir()

We use this pattern in other places, but consider that this piece of code already uses test-oriented constructor parameters, it's fine to be consistent.

[2]

+ virt_info = "OpenVZ"

Please use lowercase strings, like "openvz" "xen" and "kvm", that's one chance less to get spelling errors while writing server side code/tests that use them. If this information must be eventually shown in some UI, it will be the UI's job to map those lowercase strings to nice human-oriented strings/phrases.

[3]

- def test_resynchronize(self):
+ def test_wb_resynchronize(self):

and

+ plugin._get_vm_info = lambda: ""

Once [1] is addressed I believe you can revert these changes.

[4]

+ xen_info = [u"proc/sys/xen", u"sys/bus/xen", u"proc/xen"]
+ xen_info = map(join_root_path, xen_info)

Maybe this variable would be better named "xen_paths".