pci.PCIDevices: Use Popen(lspci -mn, universal_newlines=True)

Fixes one part of #21 by calling Popen(lspci -nm, universal_newlines=True)

This makes Popen().stdout it return an __inter__: string, which the code
expects (otherwise, it opens the stdout pipe as binary stream):

From: https://docs.python.org/3.6/library/subprocess.html#popen-constructor:

"If encoding or errors are specified, the file objects stdin, stdout and stderr
are opened in text mode with the specified encoding and errors, as described
above in Frequently Used Arguments.
If universal_newlines is True, they are opened in text mode with default
encoding. >>>__Otherwise, they are opened as binary streams.__<<<"

The new test tests xcp.pci.PCIDevices() by setting PATH to a shell script which
simluates "lspci -mn" with "cat tests/data/lspci-mn", so calls the real Popen.

Final thoughts on using mock:

Mocking xcp.pci.subprocess.Popen.stdout using open("tests/data/lspci-mn")
does not model the actual behavior of subprocess.Popen().std* in Python3:

By it's nature, mocking Popen() does not validate it's correct use in xcp.pci
and therfore, it failed to detect the Python3 problem in `xcp/pci.py`

Correctly modeling subprocess.Popen() without really calling it is nontrivial,
and would require something like the testfixtures library and even that would
still be a mock and use the actual subprocess class. Mocking is better suited
when the test knows how an API must be called, which isn't the case here.

Signed-off-by: Bernhard Kaindl <[email protected]>

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'",

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,6 +69,22 @@ def tests_nodb(self):
                 PCIIds.read()
         exists_mock.assert_called_once_with("/usr/share/hwdata/pci.ids")
 
+    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, \
@@ -80,13 +99,15 @@ def test_videoclass_by_mock_calls(self):
     @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

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('-')]