-
Notifications
You must be signed in to change notification settings - Fork 0
Removed LSBLK dependency, fixed progress bar. (Issue #178, partially #171) #2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -285,9 +285,10 @@ def test_empty_device_node_does_not_overwrite_states_dn(self, monkeypatch): | |
|
|
||
|
|
||
| class TestFindUsbHappyPath: | ||
| def test_find_usb_returns_label_from_lsblk(self, monkeypatch): | ||
| def test_find_usb_returns_label_from_udev(self, monkeypatch): | ||
| user = "testuser" | ||
| mount_path = f"/media/{user}/MY_USB" | ||
| device_node = "/dev/sdb1" | ||
|
|
||
| monkeypatch.setattr(find_usb_module.getpass, "getuser", lambda: user) | ||
| monkeypatch.setattr( | ||
|
Comment on lines
287
to
294
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (testing): New flash_usb progress and dd stderr-parsing behavior is not covered by tests; consider adding focused tests here. These changes introduce several new behaviors in
This will help prevent regressions in the new progress and parsing logic. Suggested implementation: find_usb_module.psutil,
"disk_partitions",
class TestFlashUsbProgressAndParsing:
def _install_fake_popen(self, monkeypatch, stderr_bytes, captured_popen_args):
"""
Install a fake Popen on flash_usb_module.subprocess.Popen that:
- Records args and env for assertions.
- Exposes a bytes-like stderr stream containing stderr_bytes.
- Behaves like a finished process with returncode=0.
"""
import io
class FakePopen:
def __init__(self, args, **kwargs):
# Record arguments for assertions
captured_popen_args["args"] = args
captured_popen_args["kwargs"] = kwargs
self.args = args
self.kwargs = kwargs
self.env = kwargs.get("env") or {}
# Simulated stderr stream; flash_usb should read from this
self.stderr = io.BytesIO(stderr_bytes)
self.returncode = 0
def poll(self):
return self.returncode
def wait(self, timeout=None):
return self.returncode
def communicate(self, input=None, timeout=None):
# flash_usb shouldn't rely on this, but provide it just in case
return (b"", stderr_bytes)
monkeypatch.setattr(flash_usb_module.subprocess, "Popen", FakePopen)
def test_dd_invoked_with_lc_all_c(self, monkeypatch):
"""
Ensure that flash_usb invokes dd with LC_ALL set to 'C'.
"""
captured_popen_args = {}
# stderr doesn't matter for this test
self._install_fake_popen(monkeypatch, stderr_bytes=b"", captured_popen_args=captured_popen_args)
progress_calls = []
def progress_cb(value, status=None):
progress_calls.append((value, status))
# Use a dummy ISO path and device path; adjust if your API differs
flash_usb_module.flash_usb("/tmp/test.iso", "/dev/sdz", progress_cb=progress_cb)
# Assert Popen was invoked
assert "args" in captured_popen_args
args = captured_popen_args["args"]
kwargs = captured_popen_args["kwargs"]
# dd should be part of the invoked command
assert any("dd" in str(part) for part in (args[0] if isinstance(args[0], (list, tuple)) else args)), args
# LC_ALL must be set to "C" in the environment
env = kwargs.get("env") or {}
assert env.get("LC_ALL") == "C"
def test_progress_parsing_monotonic_and_scaled(self, monkeypatch, caplog):
"""
Feed simulated dd stderr with \\r-separated/incomplete progress lines and verify:
- progress_cb is monotonic and stays within 10–95.
- status line uses raw percentage while progress_cb sees scaled value.
"""
caplog.set_level("INFO")
# Simulate dd-style progress output with carriage returns and partial lines
stderr_bytes = (
b"12345 bytes (12 MB) copied, 1.0 s, 12.3 MB/s\r"
b"12% done, 1.0 s, 12.3 MB/s\r"
b"33% done, 2.0 s, 11.0 MB/s\r"
b"95% done, 3.0 s, 10.0 MB/s\r"
)
captured_popen_args = {}
self._install_fake_popen(monkeypatch, stderr_bytes=stderr_bytes, captured_popen_args=captured_popen_args)
progress_values = []
status_values = []
def progress_cb(value, status=None):
progress_values.append(value)
status_values.append(status)
flash_usb_module.flash_usb("/tmp/test.iso", "/dev/sdz", progress_cb=progress_cb)
# Filter out any early "pre-dd" progress calls like 2/5/8
meaningful_progress = [v for v in progress_values if 10 <= v <= 95]
assert meaningful_progress, "Expected scaled progress values between 10 and 95"
# Monotonic non-decreasing within the 10–95 window
assert meaningful_progress == sorted(meaningful_progress)
# Status messages should contain the raw dd percentages (12%, 33%, 95%)
# while progress_cb receives scaled values.
# We do not assert the exact scaling function, just that raw percentages appear in status.
raw_percent_fragments = ["12%", "33%", "95%"]
status_str = " ".join(str(s) for s in status_values if s is not None)
for fragment in raw_percent_fragments:
assert fragment in status_str
# Also check log messages contain raw percentages (status line text)
log_text = " ".join(rec.getMessage() for rec in caplog.records)
for fragment in raw_percent_fragments:
assert fragment in log_text
def test_dd_common_output_not_warned_but_other_stderr_is(self, monkeypatch, caplog):
"""
Ensure 'records in', 'records out', and 'copied' lines are not logged as warnings,
but unrelated stderr lines still are.
"""
caplog.set_level("WARNING")
stderr_lines = [
b"123+0 records in\n",
b"123+0 records out\n",
b"123456 bytes (123 MB) copied, 1.23 s, 100 MB/s\n",
b"weird dd error: something unexpected\n",
]
stderr_bytes = b"".join(stderr_lines)
captured_popen_args = {}
self._install_fake_popen(monkeypatch, stderr_bytes=stderr_bytes, captured_popen_args=captured_popen_args)
def progress_cb(value, status=None):
# progress_cb is irrelevant for this test
pass
flash_usb_module.flash_usb("/tmp/test.iso", "/dev/sdz", progress_cb=progress_cb)
warning_messages = [rec.getMessage() for rec in caplog.records if rec.levelname == "WARNING"]
# Common dd noise should not be warned
assert not any("records in" in msg for msg in warning_messages)
assert not any("records out" in msg for msg in warning_messages)
assert not any("copied" in msg for msg in warning_messages)
# But unrelated stderr lines should still show up as warnings
assert any("weird dd error" in msg for msg in warning_messages)
def test_early_progress_calls_for_windows_iso(self, monkeypatch):
"""
Verify early progress_cb invocations (e.g. 2, 5, 8) occur in the correct order
for a Windows ISO path.
"""
captured_popen_args = {}
# No need for real progress; dd won't be heavily exercised here
self._install_fake_popen(monkeypatch, stderr_bytes=b"", captured_popen_args=captured_popen_args)
progress_values = []
def progress_cb(value, status=None):
progress_values.append(value)
# Heuristic: treat a path ending with '.iso' and containing backslashes as "Windows ISO"
flash_usb_module.flash_usb(r"C:\\isos\\windows-server.iso", "/dev/sdz", progress_cb=progress_cb)
# Expect specific early progress hints; adjust expected sequence to match implementation
early = [v for v in progress_values if v < 10]
assert early == sorted(early), "Early progress should be increasing for Windows ISO"
# At least contain the milestones (implementation-dependent; see additional_changes)
for milestone in (2, 5, 8):
assert milestone in early
def test_early_progress_calls_for_non_windows_iso(self, monkeypatch):
"""
Verify early progress_cb invocations occur in correct order for a non-Windows ISO path.
"""
captured_popen_args = {}
self._install_fake_popen(monkeypatch, stderr_bytes=b"", captured_popen_args=captured_popen_args)
progress_values = []
def progress_cb(value, status=None):
progress_values.append(value)
flash_usb_module.flash_usb("/isos/linux-distro.iso", "/dev/sdz", progress_cb=progress_cb)
early = [v for v in progress_values if v < 10]
assert early == sorted(early), "Early progress should be increasing for non-Windows ISO"
# Non-Windows images may skip the earliest hint; adjust as needed
assert early, "Expected at least one early progress value for non-Windows ISO"
|
||
|
|
@@ -308,14 +309,26 @@ def test_find_usb_returns_label_from_lsblk(self, monkeypatch): | |
| monkeypatch.setattr( | ||
| find_usb_module.psutil, | ||
| "disk_partitions", | ||
| lambda *args, **kwargs: [SimpleNamespace(mountpoint=mount_path, device="/dev/sdb1")], | ||
| ) | ||
| monkeypatch.setattr( | ||
| find_usb_module.subprocess, | ||
| "check_output", | ||
| lambda *a, **kw: "MY_LABEL\n", | ||
| lambda *args, **kwargs: [SimpleNamespace(mountpoint=mount_path, device=device_node)], | ||
| ) | ||
|
|
||
| import os as real_os | ||
| from unittest.mock import MagicMock | ||
| os_stat_orig = find_usb_module.os.stat | ||
| def mock_os_stat(p): | ||
| if str(p).startswith("/dev/"): | ||
| m = MagicMock() | ||
| m.st_rdev = 1234 | ||
| return m | ||
| return os_stat_orig(p) | ||
| monkeypatch.setattr(find_usb_module.os, "stat", mock_os_stat) | ||
|
|
||
| mock_context = MagicMock() | ||
| mock_device = MagicMock() | ||
| mock_device.get.return_value = "MY_LABEL" | ||
| monkeypatch.setattr(find_usb_module.pyudev, "Context", lambda: mock_context) | ||
| monkeypatch.setattr(find_usb_module.pyudev.Devices, "from_device_number", lambda ctx, type, num: mock_device) | ||
|
|
||
| result = find_usb_module.find_usb() | ||
| assert result == {mount_path: "MY_LABEL"} | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (testing): New flash_usb progress and dd stderr parsing behaviour is not exercised; add tests to cover progress_cb milestones and LC_ALL handling.
The new
flash_usblogic (earlyprogress_cbmilestones, dd progress scaling, and LC_ALL + stderr parsing) isn’t covered by tests. Please add focused tests (e.g. aTestFlashUsbProgresssuite) that:subprocess.Popenwith controlledstderrto verify:progress_cbreceives the expected early milestones and correctly scaled dd progress values.progress_cbsees the scaled percentage.subprocess.Popenis invoked with anenvthat setsLC_ALL="C"without mutating the global environment.This will guard against regressions in progress reporting and locale-dependent parsing.
Suggested implementation:
flash_usb_moduleis imported at the top oftests/test_flash_usb_and_find_usb_fixes.py, for example:import flash_usb_moduleorfrom <your_package> import flash_usb as flash_usb_module, matching howfind_usb_moduleis imported in this file.flash_usb_module.flash_usbcall signature with your implementation. If the function uses different parameter names or order (e.g.image_paththendeviceor additional options), update the tests accordingly.flash_usbemits progress milestones with specific numeric values (e.g.0.05,0.1, etc.), update theassertstatements onprogress_valuesto check for those exact milestones instead of only checking the first and last values and monotonicity.caplog.at_level(...)call to pass the correct logger name (e.g.caplog.at_level("WARNING", logger="your.module.logger")) so that the warning assertions observe the expected records.subprocess.Popenis accessed differently (e.g.from subprocess import Popeninflash_usb_module), modify themonkeypatch.setattrtarget accordingly so that the fakePopenis actually used byflash_usb.