From d51a816bfcdf6afa1fd66f59e96553beaabf7180 Mon Sep 17 00:00:00 2001 From: Bernhard Kaindl Date: Mon, 15 May 2023 12:07:04 +0200 Subject: [PATCH 1/2] tests/test_pci.py: Split tests_videoclass() into 3 methods Extracts assert_videoclass_devices() for reuse by a new testcase. Also extract mock_lspci_using_open_testfile() from tests_videoclass: mock_lspci_using_open_testfile() does one thing and extracting helps to make the use of the new function assert_videoclass_devices() more obvious and comprehensible from simply looking at the code quickly. And also fix the mypy type warning by adding missing type comments. Signed-off-by: Bernhard Kaindl --- pyproject.toml | 4 ---- tests/test_pci.py | 15 ++++++++++++--- xcp/pci.py | 1 + 3 files changed, 13 insertions(+), 7 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index c3b31481..08c43ba1 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -122,10 +122,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/test_pci.py b/tests/test_pci.py index 1895e88a..53fa5abf 100644 --- a/tests/test_pci.py +++ b/tests/test_pci.py @@ -66,7 +66,7 @@ def tests_nodb(self): PCIIds.read() exists_mock.assert_called_once_with("/usr/share/hwdata/pci.ids") - def tests_videoclass(self): + 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,8 +75,11 @@ 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)""" with patch("xcp.pci.subprocess.Popen") as popen_mock, \ open("tests/data/lspci-mn") as fake_data: @@ -84,6 +87,12 @@ def tests_videoclass(self): devs = PCIDevices() popen_mock.assert_called_once_with(['lspci', '-mn'], bufsize = 1, stdout = subprocess.PIPE) + 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..f2bddda1 100644 --- a/xcp/pci.py +++ b/xcp/pci.py @@ -271,6 +271,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 From 6bdd72b78e8c5d63a275813e09502c6e7232280b Mon Sep 17 00:00:00 2001 From: Bernhard Kaindl Date: Mon, 15 May 2023 13:12:27 +0200 Subject: [PATCH 2/2] 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 --- pylintrc | 1 + pyproject.toml | 1 + tests/data/lspci | 6 ++++++ tests/test_pci.py | 27 ++++++++++++++++++++++++--- xcp/pci.py | 8 ++++++-- 5 files changed, 38 insertions(+), 5 deletions(-) create mode 100755 tests/data/lspci 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 08c43ba1..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'", 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 53fa5abf..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,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 diff --git a/xcp/pci.py b/xcp/pci.py index f2bddda1..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('-')]