diff --git a/pylintrc b/pylintrc index ce28a4b7..fb1d2f90 100644 --- a/pylintrc +++ b/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 diff --git a/pyproject.toml b/pyproject.toml index c3b31481..3486e637 100644 --- a/pyproject.toml +++ b/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"] diff --git a/tests/data/lspci b/tests/data/lspci new file mode 100755 index 00000000..5470e2c3 --- /dev/null +++ b/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 diff --git a/tests/test_pci.py b/tests/test_pci.py index 1895e88a..9764887c 100644 --- a/tests/test_pci.py +++ b/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) diff --git a/xcp/pci.py b/xcp/pci.py index ffc52a33..9bb64aa9 100644 --- a/xcp/pci.py +++ b/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