Skip to content

Commit 6bbad2b

Browse files
pci.PCIDevices: Use Popen(lspci -mn, universal_newlines=True)
Signed-off-by: Bernhard Kaindl <bernhard.kaindl@cloud.com>
1 parent fa471e0 commit 6bbad2b

File tree

5 files changed

+64
-4
lines changed

5 files changed

+64
-4
lines changed

pylintrc

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

pyproject.toml

+1
Original file line numberDiff line numberDiff line change
@@ -46,6 +46,7 @@ dependencies = [
4646
[project.optional-dependencies]
4747
test = [
4848
"mock",
49+
"pyfakefs",
4950
"pytest",
5051
"pytest-cov",
5152
"pytest_httpserver; python_version >= '3.7'",

tests/data/lspci

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

tests/test_pci.py

+50-2
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
import subprocess
22
import unittest
3+
from os import environ
4+
5+
import pyfakefs.fake_filesystem_unittest # type: ignore[import]
36
from mock import patch, Mock
47

58
from xcp.pci import PCI, PCIIds, PCIDevices
@@ -66,6 +69,22 @@ def tests_nodb(self):
6669
PCIIds.read()
6770
exists_mock.assert_called_once_with("/usr/share/hwdata/pci.ids")
6871

72+
def test_videoclass_without_mock(self):
73+
"""
74+
Verifies that xcp.pci uses the open() and Popen() correctly across versions.
75+
Tests PCIIds.read() and PCIDevices() without mock for verifying compatibility
76+
with all Python versions.
77+
(The old test using moc could not detect a missing step in the Py3 migration)
78+
"""
79+
with pyfakefs.fake_filesystem_unittest.Patcher() as p:
80+
assert p.fs
81+
p.fs.add_real_file("tests/data/pci.ids", target_path="/usr/share/hwdata/pci.ids")
82+
ids = PCIIds.read()
83+
saved_PATH = environ["PATH"]
84+
environ["PATH"] = "tests/data" # Let PCIDevices() call Popen("tests/data/lspci")
85+
self.assert_videoclass_devices(ids, PCIDevices())
86+
environ["PATH"] = saved_PATH
87+
6988
def test_videoclass_by_mock_calls(self):
7089
with patch("xcp.pci.os.path.exists") as exists_mock, \
7190
patch("xcp.pci.open") as open_mock, \
@@ -81,12 +100,41 @@ def test_videoclass_by_mock_calls(self):
81100
def mock_lspci_using_open_testfile(cls):
82101
"""Mock xcp.pci.PCIDevices.Popen() using open(tests/data/lspci-mn)"""
83102

103+
#
104+
# Mocking xcp.pci.subprocess.Popen.stdout using open("tests/data/lspci-mn")
105+
# does not model the actual behavior of subprocess.Popen().std* in Python3:
106+
#
107+
# From: https://docs.python.org/3.6/library/subprocess.html#popen-constructor:
108+
# "If encoding or errors are specified, the file objects stdin, stdout and stderr
109+
# are opened in text mode with the specified encoding and errors, as described
110+
# above in Frequently Used Arguments.
111+
# If universal_newlines is True, they are opened in text mode with default
112+
# encoding. >>>__Otherwise, they are opened as binary streams.__<<<"
113+
#
114+
# By it's nature, mocking Popen() does not validate it's correct use in xcp.pci
115+
# and therfore, it failed to detect the Python3 problem in `xcp/pci.py`
116+
#
117+
# The validation of xcp.pci.PCIDevices() using self.PopenWrapper(), replacing
118+
# "lspci -mn" with "cat tests/data/lspci-mn" is able to detect such usage bugs.
119+
#
120+
# Correctly modeling subprocess.Popen() without really calling it is nontrivial,
121+
# and would require something like the testfixtures library and even that would
122+
# still be a mock and use the actual subprocess class. Mocking is better suited
123+
# when the test knows how an API must be called, which isn't the case here.
124+
#
84125
with patch("xcp.pci.subprocess.Popen") as popen_mock, \
85126
open("tests/data/lspci-mn") as fake_data:
86127
popen_mock.return_value.stdout.__iter__ = Mock(return_value=iter(fake_data))
87128
devs = PCIDevices()
88-
popen_mock.assert_called_once_with(['lspci', '-mn'], bufsize = 1,
89-
stdout = subprocess.PIPE)
129+
#
130+
# Updated to accept the use of universal_newlines=True as a kind of bridge
131+
# from Python2 to Python3, because it is accepted by both versions and
132+
# on both versions it makes Popen().stdout return a iterator on strings
133+
# which is what xcp.pci.PCIDevices expects to get:
134+
#
135+
popen_mock.assert_called_once_with(
136+
["lspci", "-mn"], bufsize=1, stdout=subprocess.PIPE, universal_newlines=True
137+
)
90138
return devs
91139

92140
def assert_videoclass_devices(self, ids, devs): # type: (PCIIds, PCIDevices) -> None

xcp/pci.py

+6-2
Original file line numberDiff line numberDiff line change
@@ -255,8 +255,12 @@ class PCIDevices(object):
255255
def __init__(self):
256256
self.devs = {}
257257

258-
cmd = subprocess.Popen(['lspci', '-mn'], bufsize = 1,
259-
stdout = subprocess.PIPE)
258+
cmd = subprocess.Popen(
259+
["lspci", "-mn"],
260+
bufsize=1,
261+
stdout=subprocess.PIPE,
262+
universal_newlines=True,
263+
)
260264
for l in cmd.stdout:
261265
line = l.rstrip()
262266
el = [x for x in line.replace('"', '').split() if not x.startswith('-')]

0 commit comments

Comments
 (0)