Merge pull request #38 from xenserver-next/Py3-fix-PCIDevices-Popen-universal_newlines

pci.PCIDevices(): Fix Popen("lspci -mn") for Py3 using universal_newlines=True

Author: bernhardkaindl

pylintrc

@@ -53,6 +53,7 @@ disable=W0142,W0703,C0111,R0201,W0603,W0613,W0212,W0141,
     unrecognized-option,        # Skip complaining on pylintrc options only in pylint2/pylint3
     unknown-option-value,       # Skip complaining about checkers only in pylint2/pylint3
     useless-object-inheritance, # "object" is not obsolete for supporting Python2
+    super-with-arguments,       # super() with arguments is a Python3-only feature
     consider-using-f-string,    # Python3-only feature, need to migrate everything first
     consider-using-with,        # Only for new code, move to Python3 is more important
     logging-not-lazy            # Debug-Logging is not used in "hot" code paths here

pyproject.toml

@@ -46,6 +46,7 @@ dependencies = [
 [project.optional-dependencies]
 test = [
     "mock",
+    "pyfakefs",
     "pytest",
     "pytest-cov",
     "pytest_httpserver; python_version >= '3.7'",
@@ -122,10 +123,6 @@ disable_error_code = ["var-annotated", "unreachable"]
 
 # Most of these should be easily fixable by adding type annotations as comments(PEP484):
 
-[[tool.mypy.overrides]]
-module = ["tests.test_pci"]
-disable_error_code = ["no-any-return"]
-
 [[tool.mypy.overrides]]
 module = ["tests.test_mac"]
 disable_error_code = ["var-annotated"]

tests/data/lspci

@@ -0,0 +1,6 @@
+#!/bin/sh
+# Simulate lspci -nm for tests.test_pci.test_videoclass_without_mock():
+if [ "$1" = "-mn" ]; then
+	PATH=/usr/bin:/bin
+	cat tests/data/lspci-mn
+fi

tests/test_pci.py

@@ -1,5 +1,8 @@
 import subprocess
 import unittest
+from os import environ
+
+import pyfakefs.fake_filesystem_unittest  # type: ignore[import]
 from mock import patch, Mock
 
 from xcp.pci import PCI, PCIIds, PCIDevices
@@ -66,7 +69,23 @@ def tests_nodb(self):
                 PCIIds.read()
         exists_mock.assert_called_once_with("/usr/share/hwdata/pci.ids")
 
-    def tests_videoclass(self):
+    def test_videoclass_without_mock(self):
+        """
+        Verifies that xcp.pci uses the open() and Popen() correctly across versions.
+        Tests PCIIds.read() and PCIDevices() without mock for verifying compatibility
+        with all Python versions.
+        (The old test using moc could not detect a missing step in the Py3 migration)
+        """
+        with pyfakefs.fake_filesystem_unittest.Patcher() as p:
+            assert p.fs
+            p.fs.add_real_file("tests/data/pci.ids", target_path="/usr/share/hwdata/pci.ids")
+            ids = PCIIds.read()
+        saved_PATH = environ["PATH"]
+        environ["PATH"] = "tests/data"  # Let PCIDevices() call Popen("tests/data/lspci")
+        self.assert_videoclass_devices(ids, PCIDevices())
+        environ["PATH"] = saved_PATH
+
+    def test_videoclass_by_mock_calls(self):
         with patch("xcp.pci.os.path.exists") as exists_mock, \
              patch("xcp.pci.open") as open_mock, \
              open("tests/data/pci.ids") as fake_data:
@@ -75,15 +94,26 @@ def tests_videoclass(self):
             ids = PCIIds.read()
         exists_mock.assert_called_once_with("/usr/share/hwdata/pci.ids")
         open_mock.assert_called_once_with("/usr/share/hwdata/pci.ids")
-        video_class = ids.lookupClass('Display controller')
-        self.assertEqual(video_class, ['03'])
+        self.assert_videoclass_devices(ids, self.mock_lspci_using_open_testfile())
 
+    @classmethod
+    def mock_lspci_using_open_testfile(cls):
+        """Mock xcp.pci.PCIDevices.Popen() using open(tests/data/lspci-mn)"""
+        # Note: Mocks Popen using open, which is wrong, but mocking using Popen is
+        # not supported by mock, so the utility of this test is limited - may be removed
         with patch("xcp.pci.subprocess.Popen") as popen_mock, \
              open("tests/data/lspci-mn") as fake_data:
             popen_mock.return_value.stdout.__iter__ = Mock(return_value=iter(fake_data))
             devs = PCIDevices()
-        popen_mock.assert_called_once_with(['lspci', '-mn'], bufsize = 1,
-                                           stdout = subprocess.PIPE)
+        popen_mock.assert_called_once_with(
+            ["lspci", "-mn"], bufsize=1, stdout=subprocess.PIPE, universal_newlines=True
+        )
+        return devs
+
+    def assert_videoclass_devices(self, ids, devs):  # type: (PCIIds, PCIDevices) -> None
+        """Verification function for checking the otuput of PCIDevices.findByClass()"""
+        video_class = ids.lookupClass('Display controller')
+        self.assertEqual(video_class, ["03"])
         sorted_devices = sorted(devs.findByClass(video_class),
                                 key=lambda x: x['id'])
         self.assertEqual(len(sorted_devices), 2)

xcp/pci.py

@@ -255,8 +255,12 @@ class PCIDevices(object):
     def __init__(self):
         self.devs = {}
 
-        cmd = subprocess.Popen(['lspci', '-mn'], bufsize = 1,
-                               stdout = subprocess.PIPE)
+        cmd = subprocess.Popen(
+            ["lspci", "-mn"],
+            bufsize=1,
+            stdout=subprocess.PIPE,
+            universal_newlines=True,
+        )
         for l in cmd.stdout:
             line = l.rstrip()
             el = [x for x in line.replace('"', '').split() if not x.startswith('-')]
@@ -271,6 +275,7 @@ def __init__(self):
         cmd.wait()
 
     def findByClass(self, cls, subcls = None):
+        # type: (str|list[str], str|None) -> list[dict[str, str]]
         """ return all devices that match either of:
 
         	class, subclass