Process Module Preperation #6229
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR expands and clarifies docstrings across avocado/utils/process.py, updates has_capability to accept an optional pid, makes binary_from_shell_cmd raise ValueError when no binary is found, clarifies pid_exists behavior (ESRCH → False) and safe_kill semantics (owner checks and sudo path), and documents other function behaviors. It adds extensive unit and functional tests for process utilities, updates test counts in selftests/check.py, appends entries to spell.ignore, and reorders the .pylintrc_utils blacklist. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6229 +/- ##
==========================================
+ Coverage 73.50% 73.59% +0.08%
==========================================
Files 206 206
Lines 22497 22497
==========================================
+ Hits 16536 16556 +20
+ Misses 5961 5941 -20 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
clebergnu
left a comment
clebergnu
left a comment
There was a problem hiding this comment.
Hi @harvey0100 ,
Thanks for this, it's a great improvement. Please take a look at a few comments I left.
richtja
left a comment
richtja
left a comment
There was a problem hiding this comment.
Hi @harvey0100, thank you for preparing the process module for the migration. I have few comments to the tests, and I am also missing the update of pylint checks, which would reveal any issues with the docstring changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
avocado/utils/process.py (2)
65-101: Fix bytes/str comparison in can_sudo() for non‑root checkIn the
cmd is Nonebranch you do:if system_output("id -u", ignore_status=True, sudo=True).strip() == "0": return True
system_output()returnsbytes, so this comparison against the string"0"will always be False. That meanscan_sudo()will incorrectly returnFalsefor non‑root users with a workingsudoconfiguration when nocmdis passed.A minimal fix is to compare against a bytes literal:
- if system_output("id -u", ignore_status=True, sudo=True).strip() == "0": + if system_output("id -u", ignore_status=True, sudo=True).strip() == b"0": return TrueOptionally, you may also want a unit test that covers
can_sudo()(nocmd) for non‑root with functional sudo to guard this behavior.
379-404: Fix bytes/str mismatch in process_in_ptree_is_defunct()Here:
proc_name = system_output(cmd, ignore_status=True, verbose=False) if "<defunct>" in proc_name: defunct = True break
system_output()returnsbytes, so checking for a text substring raises aTypeErroron Python 3 ('in <bytes>' requires a bytes-like object). This makes the function unreliable whenever it actually inspectsproc_name.A minimal fix is to compare using a bytes literal:
- proc_name = system_output(cmd, ignore_status=True, verbose=False) - if "<defunct>" in proc_name: + proc_name = system_output(cmd, ignore_status=True, verbose=False) + if b"<defunct>" in proc_name: defunct = True breakAlternatively, you could decode the output once and stay in the text domain, but the bytes‑literal change is smallest and consistent with
system_output()’s API.
🧹 Nitpick comments (1)
selftests/unit/utils/process.py (1)
545-578: can_sudo tests are good, but one important scenario is untestedThe new tests nicely cover root, missing sudo, specific command success, and
OSErrorcases. What’s missing is a test for the common pathcan_sudo()as a non‑root user with a working sudo configuration (nocmdargument), which would exercise thesystem_output("id -u", ...)branch. That missing test would also have caught the current bytes/str comparison bug incan_sudo()(see comment onavocado/utils/process.pyaround Line 65).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.pylintrc_utils(1 hunks)avocado/utils/process.py(43 hunks)selftests/check.py(1 hunks)selftests/functional/utils/process.py(3 hunks)selftests/unit/utils/process.py(2 hunks)spell.ignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spell.ignore
- selftests/check.py
🧰 Additional context used
🧬 Code graph analysis (1)
selftests/functional/utils/process.py (1)
avocado/utils/process.py (5)
system_output(1216-1284)getstatusoutput(1350-1413)SubProcess(653-1071)stdout_text(505-516)binary_from_shell_cmd(407-435)
🪛 Ruff (0.14.7)
selftests/unit/utils/process.py
644-644: Function call with shell=True parameter identified, security issue
(S604)
🔇 Additional comments (33)
.pylintrc_utils (1)
10-10: LGTM: Cosmetic reordering with no functional impact.This reordering of the ignore list has no effect on pylint's behavior, as the list is treated as a set of files to skip.
selftests/unit/utils/process.py (5)
359-365: binary_from_shell_cmd invalid‑input coverage looks correctThe new test cases exercise both the “env‑only” command and empty string paths, matching the updated behavior of raising
ValueErrorwhen no binary can be parsed.
579-597: has_capability unit tests match the API, including pid usageThese tests validate both positive/negative capability membership and confirm that passing
pidpropagates correctly toget_capabilities(pid). They align well with the updated docstring and implementation.
598-620: pid_exists tests correctly encode ESRCH vs EPERM semanticsThe three tests cover success, non‑existent PID (
errno.ESRCH→ False) and permission‑denied (errno.EPERM→ True), which matches the intended behavior ofpid_exists(). This is a solid regression‑proofing of the subtle errno handling.
621-634: kill_process_by_pattern tests validate pkill wiring and failure handlingThe success test asserts the exact
run("pkill -f pattern", ignore_status=True)invocation, and the failure test ensures that a non‑zero exit status doesn’t raise. That matches the implementation, which only logs on failure.
635-685: system/system_output/getoutput/getstatusoutput integration tests look appropriateThese tests exercise:
system()success and non‑zero exit behavior,system_output()return type (bytes) and newline‑stripping behavior,getoutput()string output,getstatusoutput()returning(status, output)and handling failures.They align with the documented and implemented behavior of the wrappers. The
shell=Trueusage in the failure tests is acceptable here because the commands are fixed literals and this is a controlled test environment.avocado/utils/process.py (19)
17-17: Module‑level docstring is a clear improvementThe new module docstring succinctly describes the purpose of this module (“find and run external commands”) and improves discoverability without changing behavior.
104-134: get_capabilities/has_capability doc updates and pid support look consistentThe
get_capabilities()andhas_capability()docstrings now clearly explain pid handling, capability naming, and return types, and thehas_capability(capability, pid=None)implementation simply delegates toget_capabilities(pid). The behavior matches both the docs and the new unit tests.Also applies to: 136-161
164-187: pid_exists semantics and documentation align with testsThe expanded docstring accurately documents the
os.kill(pid, 0)technique and the ESRCH handling. The logic (False onerrno.ESRCH, True otherwise, including EPERM) is consistent with both standard practice and the new unit tests.
190-221: safe_kill owner‑based logic matches tests and intentUsing
get_owner_id(int(pid))to decide between a sudo‑basedkill(owner is root or unknown) and directos.kill()(non‑root) matches the new unit tests and the goal of handling privilege requirements transparently. The boolean return based on success/failure is also clearly documented now.
223-244: get_parent_pid Linux‑specific docstring is accurateThe new docstring (including the Linux‑specific note) correctly describes the
/proc/<pid>/stat‑based implementation and the return type. Behavior is unchanged and still matches existing tests.
253-291: get_children_pids docs now reflect actual behavior, including recursionThe docstring now clearly states that this is
/proc‑based, Linux‑specific, and explains therecursiveflag semantics, return type, and example usage. This is in line with the implementation and the existing Linux‑only unit test.
294-353: kill_process_tree documentation is clearer and consistent with implementationThe updated docstring accurately documents
sig,send_sigcont,timeoutsemantics, and theRuntimeErrorbehavior when processes don’t die within the timeout. The implementation is unchanged and already well covered by unit tests.
355-377: kill_process_by_pattern docstring matches pkill‑based implementationThe docstring now makes explicit that
pkill -fis used, that matching is against the full command line, and that the operation logs success/failure but doesn’t raise. This lines up with the implementation and the new unit tests.
407-435: binary_from_shell_cmd behavior and docs are now well‑specifiedThe enhanced docstring (including examples and the ValueError contract) aligns with the implementation that skips env assignments via
_RE_BASH_SET_VARIABLEand returns the first non‑assignment token. The new unit and functional tests cover both valid and invalid inputs, including env‑only commands and empty strings.
444-463: CmdResult documentation and text helpers are clear and match usageThe expanded class docstring and the
stdout_text/stderr_textproperty docs correctly describe the bytes→text decoding behavior and theTypeErrorraised when decoding is impossible. This matches how the tests exerciseCmdResultand how higher‑level helpers likegetstatusoutput()rely onstdout_text.Also applies to: 504-531
533-577: FDDrainer and its helpers are now better documentedThe FDDrainer class and its methods (
__init__,_drainer,start,flush) are now clearly documented, including the roles oflogger,stream_logger,ignore_bg_processes, andverbose. The implementation remains unchanged and is well covered by the FDDrainer tests.Also applies to: 596-625
653-702: SubProcess API docstrings are now comprehensive and aligned with behaviorThe updated docstrings for
SubProcessand its methods (__init__,_fill_streams,start,get_stdout,get_stderr,terminate,kill,send_signal,poll,wait,stop,get_pid,get_user_id,is_sudo_enabled,run) clearly document parameters, return types, and error conditions (including the zombieAssertionError). They line up with the existing implementation and the unit/functional tests that exercise timeouts, sudo, and signal handling.Also applies to: 828-872, 885-901, 903-949, 1002-1017, 1023-1049, 1050-1071
1075-1145: run(): error contracts and return type are now explicitThe docstring now documents
CmdInputErrorfor empty commands andCmdErrorwhenignore_status=Falseand the subprocess fails, plus theCmdResultreturn type. This matches the implementation (if not cmd: raise CmdInputError(...)) and the newtest_empty_commandunit test.
1149-1213: system() wrapper docstring correctly describes the APIThe updated docstring explains that
system()forwards torun()and returns only the exit code while propagatingCmdErrorwhenignore_status=False. This behavior is consistent with both the implementation and the unit tests forsystem().
1216-1285: system_output(): bytes return type and behavior are clearly specifiedThe docstring now makes explicit that
system_output()returnsbytes, supportsstrip_trail_nl, and can raiseCmdError. This matches the implementation and the new unit tests that assert on raw bytes and newline behavior.
1288-1335: getoutput() documentation matches its getstatusoutput‑based implementationThe docstring correctly states that it returns a
str, mirrorscommands.getoutput()semantics, and forwards togetstatusoutput()withshell=Trueandignore_status=Trueby default. That matches the actual implementation and new integration tests.
1350-1397: getstatusoutput() docs align with wrapper behaviorThe function is now documented as returning
(status, output)whereoutputis the decoded stdout text with a trailing newline removed. This matches the current implementation and the new tests that assert on both status and string output.
1416-1439: get_owner_id(): Linux note and None‑on‑failure behavior are clearThe docstring now documents that this is
/proc‑based, Linux‑specific, and returnsNonewhen the process cannot be found or read. That matches the implementation (os.stat(...).st_uidwithOSError→None) and the new tests that cover both success and failure paths.
1442-1461: get_command_output_matching docstring matches substring‑per‑line implementationThe clarified docs (line‑by‑line substring matching, list of matching lines) exactly reflect the comprehension
[...] if pattern in lineapplied torun(command).stdout_text.splitlines(). This is simple and consistent with expectations.selftests/functional/utils/process.py (8)
125-143: Script templates centralize test scripts and improve readabilityDefining
ENV_TEST_SCRIPT,SUCCESS_SCRIPT,LARGE_OUTPUT_SCRIPT, andUNICODE_TEST_SCRIPTas format‑based templates (with{interpreter}) removes duplication and makes the functional tests easier to read and maintain. The use of{{i}}inLARGE_OUTPUT_SCRIPTis correct for later.format()substitution.
158-169: _create_script() helper nicely factors out script creation logicThis helper cleanly encapsulates script creation with interpreter substitution, UTF‑8 encoding, and executable permissions under
tmpdir. It directly addresses previous feedback about repeating script‑creation boilerplate.
215-229: Environment propagation tests for run() and system_output() are solid
test_run_and_system_output_with_environmentverifies that bothprocess.run(..., env=...)andprocess.system_output(..., env=...)honor custom environment variables. The assertions on stdout content and exit status are strict enough to catch regressions.
230-237: getstatusoutput integration test validates real‑world usage
test_getstatusoutput_integrationexercisesprocess.getstatusoutput()against an actual script, confirming both status==0 and string output"success". This nicely complements the unit tests that useecho.
238-253: Multiple concurrent SubProcess instances test is appropriate
test_multiple_subprocess_instancesstarts threeSubProcessinstances that sleep for 1s and print their index, then waits on each and assertsexit_status == 0and that the corresponding index appears in stdout. This provides a good smoke test of concurrent subprocess management without relying on ordering.
254-271: Large output test now strictly validates completeness and integrity
test_process_with_large_outputchecks that:
- the command exits with 0,
- exactly 1000 lines are captured, and
- the first and last lines have the expected repeated pattern.
This directly addresses concerns about partial capture of large stdout and ensures detection of truncation/corruption.
272-279: CmdResult encoding behavior is functionally validated
test_cmdresult_encodingconfirms thatencoding="utf-8"is preserved on the result and thatstdout_textcorrectly decodes Unicode output ("Avokádo"). This is a good end‑to‑end check of the encoding plumbing.
280-287: binary_from_shell_cmd_realworld test covers realistic command formsThe “real‑world” patterns (env assignments before
/usr/bin/python, andsudo -u user /bin/bash -c ...) validate thatbinary_from_shell_cmd()returns the expected first executable (/usr/bin/pythonandsudorespectively), complementing the more synthetic unit tests.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (6)
avocado/utils/process.py (3)
136-162:has_capability()PID support and man‑page note are clearThe added
pidparameter and the note about UPPERCASE names incapabilities(7)versus lowercase Python strings address the earlier review concern and make the API easier to use. Implementation simply delegates toget_capabilities(pid), which is appropriate.
223-244:get_parent_pid()doc now correctly marks Linux‑specific behaviorThe added docstring (including the Linux‑specific note) matches the implementation that reads
/proc/<pid>/statand extracts the PPID. Looks good.
1416-1439:get_owner_id()doc now calls out Linux/procdependencyThe new docstring (including the Linux note and
Noneon error) matches the implementation usingos.stat("/proc/<pid>/"). This also complements the safe_kill / SubProcess sudo logic.selftests/functional/utils/process.py (3)
125-142: Script templates parameterized by{interpreter}look correctThe new
ENV_TEST_SCRIPT,SUCCESS_SCRIPT,LARGE_OUTPUT_SCRIPT, andUNICODE_TEST_SCRIPTtemplates centralize the script contents and make it easy to inject the right interpreter path via.format(interpreter=sys.executable). This addresses the earlier suggestion about using global constants for script bodies.
158-169:_create_script()nicely de‑duplicates script setupThe helper correctly:
- Writes the formatted contents with the right interpreter.
- Places scripts under
self.tmpdir.- Applies
DEFAULT_MODEso they are executable.This reduces duplication across tests and matches the prior review suggestion to factor script creation into a shared method.
255-271: Large‑output test precisely validates capture completeness
test_process_with_large_outputverifies:
- Exit status 0.
- Exactly 1000 lines captured.
- Correct content of the first and last lines.
This is a strong regression guard for buffering/draining issues and addresses the earlier concern about being more strict on output size.
🧹 Nitpick comments (4)
avocado/utils/process.py (4)
190-221: Clarify ownership logic insafe_kill()The behavior is reasonable, but the ownership check is slightly opaque:
if not get_owner_id(int(pid)):will treat bothuid == 0anduid is Noneas “needs sudo / elevated path”, leading torun("kill -…", sudo=True).- For non‑root owners (
uid != 0),os.killis used directly.Functionally this works (root callers end up running plain
killbecause_prepend_sudo()is a no‑op for uid 0, and missing/procentries are handled viaCmdError), but readability would improve with an explicit branch such as:- if not get_owner_id(int(pid)): + owner_id = get_owner_id(int(pid)) + if owner_id is None or owner_id == 0:That also makes the “unknown owner vs. root‑owned” distinction clearer for future readers.
355-377:kill_process_by_pattern()logging fine, consider minor polishBehavior is unchanged except for clearer docs and logging:
- Uses
pkill -fwithignore_status=True.- Logs an error on non‑zero exit status and info on success.
Two very minor nits you can take or leave:
- The info message could read “Succeeded to run '%s'.” or “Successfully ran '%s'.” instead of “Succeed to run '%s'.” for grammar.
- If you ever expect patterns with whitespace or shell metacharacters,
cmd = f"pkill -f {pattern!r}"(and corresponding adjustment in tests) would make the pattern handling more robust.
504-517:stdout_text/stderr_textdoc vs actual exceptionsThe implementations correctly:
- Decode
bytesusingself.encoding.- Return strings as‑is.
- Raise
TypeErrorwhen the stored value is neitherbytesnorstr.Note that for undecodable
bytesthe underlyingdecode()will raiseUnicodeDecodeError(aValueErrorsubclass), notTypeError. The docstrings currently only mentionTypeError, which is accurate for the “wrong type” case but incomplete for the “invalid byte sequence” case.If you want the docs to be fully precise, you could mention both
UnicodeDecodeErrorandTypeErroras possible failures here.
654-702:SubProcess.__init__docs align with current behavior, with a minor nitThe constructor docs now clearly describe
cmd, environment merging,sudobehavior, and logging. Implementation matches the description.One small mismatch: the doc advertises
:raises ValueError: If incorrect values are given to parameters., but the body does not currently raise aValueErrorfor any parameter combinations. If no such validation is planned, you might want to drop that:raisesline to avoid misleading readers.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.pylintrc_utils(1 hunks)avocado/utils/process.py(43 hunks)selftests/check.py(1 hunks)selftests/functional/utils/process.py(3 hunks)selftests/unit/utils/process.py(2 hunks)spell.ignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- spell.ignore
- .pylintrc_utils
🧰 Additional context used
🧬 Code graph analysis (2)
selftests/functional/utils/process.py (2)
avocado/utils/process.py (5)
system_output(1216-1284)getstatusoutput(1350-1413)SubProcess(653-1071)stdout_text(505-516)binary_from_shell_cmd(407-435)selftests/utils.py (1)
skipOnLevelsInferiorThan(133-138)
selftests/unit/utils/process.py (1)
avocado/utils/process.py (9)
binary_from_shell_cmd(407-435)can_sudo(65-101)has_capability(136-161)pid_exists(164-187)kill_process_by_pattern(355-376)system(1149-1212)system_output(1216-1284)getoutput(1288-1346)getstatusoutput(1350-1413)
🪛 Ruff (0.14.7)
selftests/unit/utils/process.py
644-644: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (24)
- GitHub Check: Linux with Python 3.12
- GitHub Check: Linux with Python 3.10
- GitHub Check: Linux with Python 3.9
- GitHub Check: Linux with Python 3.11
- GitHub Check: Linux with Python 3.13
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: Fedora selftests
- GitHub Check: Egg task ubi:8.8
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Windows with Python 3.13
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: macOS with Python 3.11
- GitHub Check: Static checks
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (36)
selftests/check.py (1)
32-32: Confirm updatedfunctional-parallelTEST_SIZE matches new suite sizeThe bump to
TEST_SIZE["functional-parallel"] = 348looks consistent with the added functional tests, but this mapping is brittle. Please confirm you re-ranselftests/check.py(or equivalent listing) and verified that 348 matches the current functional suite size on a clean tree.avocado/utils/process.py (24)
65-102:can_sudo()behavior with optionalcmdlooks soundThe new
cmdparameter and logic are consistent with the docstring:
- Root (uid 0) still short‑circuits to
True.- Non‑root: presence of
sudois checked viapath.find_command, and either a concretecmd(viasystem(..., sudo=True)) or a simpleid -ucheck is used.- Broken sudo (OSError) is safely treated as
False.Nothing blocking here; good balance between functionality and safety.
104-134: Capabilities parsing and Linux‑specific semantics look correct
get_capabilities(pid=None)now clearly documents PID handling and the twogetpcapsformats, and the implementation matches the description. Returning an empty list onFileNotFoundErroror non‑zero exit preserves the “no capabilities / no getpcaps” semantics.
165-187:pid_exists()ESRCH/EPERM semantics are correct but worth calling outThe new behavior of returning
Falseonly forerrno.ESRCHand treating other errors (notablyEPERM) as “PID exists” matches the commonkill(pid, 0)pattern and makes this helper safer under restricted permissions. This is a sensible improvement and aligns with the docstring.
253-292:get_children_pids()recursive behavior matches docsDocstring now clearly describes recursive vs non‑recursive modes and the Linux
/procdependency. Implementation (glob over/proc/*/stat, parse PPID, recurse on matching children) aligns with the description; no issues spotted.
294-353:kill_process_tree()timeout semantics remain consistentThe expanded docstring reflects the current behavior:
- Default
sigisSIGKILLwhen none is provided.- A preliminary
SIGSTOPis sent viasafe_kill.- Descendants are gathered via
get_children_pidsand killed recursively.timeout > 0useswait_forpluspid_exists, and negative timeouts loop indefinitely until all PIDs disappear.Given the updated
pid_exists()semantics, this is internally consistent. No functional issues noticed.
379-405:process_in_ptree_is_defunct()docs now align with implementationThe updated docstring clarifies that this relies on GNU
psand may not work on macOS, which matches theps --no-headers -o cmdinvocation. ReturningTrueonCmdError(missing parent) continues to treat missing processes as “defunct from the caller's perspective”, which is conservative but acceptable.
444-463:CmdResultdocstring is clear and matches constructor usageThe parameter documentation (including
encoding) lines up with howCmdResultis constructed throughout the module. Centralizing the meaning ofencodinghere is helpful given the broader process API.
533-577: FDDrainer docs and behavior match its usageThe refreshed
FDDrainerdocstring and parameter docs match howSubProcesswires it up. The_drainer()logic withignore_bg_processes, buffering, and logging is unchanged and looks correct. No issues from a concurrency or resource‑handling standpoint.
844-853:_fill_streams()cleanup and result population are consistentThe docstring now explains that stdout/stderr pipes are closed and results populated. The call order (flush drainers, then read from their buffers into
CmdResult) is correct and matches how the rest ofSubProcessexpects to consume data.
855-862:start()doc makes background‑process usage clearerThe added explanation that
start()is useful for background processes and returns the PID is accurate and matches the behavior. No changes needed.
867-883:get_stdout()/get_stderr()docs consistent with bytes return typeThe new docstrings emphasize that these methods return raw
bytes, which is consistent with howCmdResult.stdout/stderrare set. Good clarification, especially given the existence ofstdout_text/stderr_text.
885-901:terminate()andkill()docs encourage saferstop()usageThe expanded docstrings recommending
stop()over directterminate()/kill()are helpful and match howwait()handles timeouts and tree termination. Behavior remains unchanged and looks correct.
902-918:send_signal()sudo‑aware logic remains sensibleThe new doc clarifies semantics; implementation:
- Resolves the full tree via
get_children_pids(self.get_pid())when running as root/sudo and sendskillviarun(..., sudo=True), suppressing errors.- Falls back to
Popen.send_signalotherwise.This is consistent with the higher‑level process‑tree management functions and with the new
is_sudo_enabled()behavior.
920-947:poll()andwait()docs now reflect timeout/zombie handlingDocstrings correctly describe:
poll()returningNonewhile the process is running and filling results once it finishes.wait()’s behavior withtimeout=None,timeout>0, and implicit 1‑second grace followed by tree nuking and potentialAssertionErrorfor zombies.The underlying logic is complex but appears internally consistent and is well tested by the existing “refuse_to_die” tests.
1003-1021:stop()doc correctly describes graceful shutdown flow
stop()’s docstring now mirrorswait()’s timeout and signal logic and clarifies that it terminates and waits when the process is still running. Behavior is unchanged and appropriate.
1023-1049:get_pid()/get_user_id()/is_sudo_enabled()docs align with implementation
get_pid()returns the actualPopen.pid.get_user_id()delegates toget_owner_id.is_sudo_enabled()essentially checks for uid 0 (viaget_user_id()returning falsy).Docs match that behavior and make the sudo detection clearer to callers.
1051-1071:SubProcess.run()doc is consistent withwait()and result usageThe method correctly starts (if needed), waits with the given timeout/signal semantics, and returns the populated
CmdResult. The type hints and description are accurate.
1075-1145:run()wrapper: empty‑command guard and docs look goodThe updated
run():
- Validates
cmdand raisesCmdInputErroron empty input (good defensive behavior).- Clearly documents all parameters, including environment, sudo, and
ignore_bg_processes.- Returns a
CmdResultand raisesCmdErrorwhenignore_status=Falseand the command fails.The implementation matches the docstring; the new empty‑command check is also covered by unit tests.
1149-1199:system()wrapper docs and implementation are aligned
system()now explicitly documents its reliance onrun()and that it returns just the exit code, raisingCmdErrorwhenignore_status=False. The code simply forwards parameters torun()and returnsexit_status, which is correct.
1216-1269:system_output()docs clarify bytes return and newline handlingThe docstring correctly states that:
- The output is returned as
bytes.- Trailing newlines are stripped when
strip_trail_nl=True.The implementation matches this behavior and simply wraps
run(). No issues spotted.
1288-1335:getoutput()wrapper behavior matches legacycommands.getoutputThe docstring makes clear that stderr is redirected into stdout (via
getstatusoutput), and the implementation simply indexes[1]fromgetstatusoutput. This is consistent with the Python 2commandsmodule semantics.
1350-1397:getstatusoutput()integration withrun()looks correctThe function wraps
run()withshell=Trueby default, then convertsstdoutto text and strips a final newline. That matches the documented return type ((status, output)) and compatibility goal with Python’s oldcommandsmodule. No concerns here.
1442-1455:get_command_output_matching()doc accurately reflects behaviorThe docstring now clearly states that matching is substring‑based on each output line, and that a list of matching lines is returned. Implementation using
run(command).stdout_text.splitlines()is consistent with that.
407-436:binary_from_shell_cmd()raisingValueErroris a behavior changeThe new behavior of raising
ValueError(instead of silently returningNone) when no non‑assignment token is found is a good defensive choice and is well covered by unit tests. It will, however, break any external callers that previously relied on a falsey "no binary" result rather than handling an exception.If this function is part of the public API (used by plugins or external consumers), consider:
- Documenting the change in release notes / deprecation notes, or
- Keeping a transitional behavior (e.g., optional
raise_on_error=Trueflag) if you expect third‑party usage.Inside Avocado itself this looks safe given the updated tests.
selftests/unit/utils/process.py (6)
359-365:test_binary_from_shell_cmd_invalidcorrectly asserts new error behaviorThese tests explicitly validate that purely “VAR=VALUE …” inputs and the empty string now raise
ValueError, which matches the newbinary_from_shell_cmd()contract and helps catch regressions.
545-578:can_sudo()unit tests cover key branchesThe added tests exercise:
- Root short‑circuit (
uid == 0).- No
sudoinstalled (CmdNotFoundError).- Successful specific command via
system(..., sudo=True).- Broken sudo path raising
OSError.This nicely covers the major control‑flow paths in
can_sudo()and aligns with the implementation.
579-597:has_capability()tests validate both presence and PID forwardingThe tests confirm:
- Positive and negative capability membership against mocked
get_capabilities.- That the optional
pidargument is forwarded correctly.This accurately locks in the intended semantics of the wrapper.
598-620:pid_exists()tests nail ESRCH vs EPERM behaviorThe unit tests distinguish between:
- Success (
os.killreturns normally).- Non‑existent PID (
errno.ESRCH->False).- Permission denied (
errno.EPERM->True).This matches the new implementation and ensures the subtle semantics stay correct.
621-634:kill_process_by_patterntests verify both success and failure pathsThe tests check:
- That
run("pkill -f <pattern>", ignore_status=True)is invoked with the expected command.- That a non‑zero exit status does not raise.
This aligns with
kill_process_by_pattern()’s implementation and logging behavior.
635-685:system/system_output/getoutput/getstatusoutputtests look good; S604 is a false positive hereThese tests validate:
system()returning the exit code (0 for a simple echo, non‑zero forfalsewithshell=True).system_output()returning bytes and respectingstrip_trail_nl.getoutput()returning decoded text.getstatusoutput()returning(status, text)for both success and failure.The
shell=Trueuse intest_system_with_failureis over a constant string"false", so the Ruff S604 warning (“shell=True” security issue) is not a practical risk in this context. Keepingshell=Truehere is appropriate to exercise that specific code path.selftests/functional/utils/process.py (5)
215-229: Env propagation tests forrun()andsystem_output()are solid
test_run_and_system_output_with_environmentverifies that bothprocess.run()andprocess.system_output()respect theenvdict, which is important given theenvmerging logic inSubProcess. Using the small helper script keeps this focused and robust.
230-237:test_getstatusoutput_integrationexercises real command pathThis test ensures that
getstatusoutput()works end‑to‑end with an actual executable script and that the(status, output)contract holds. Good integration coverage on top of the unit tests.
238-253:test_multiple_subprocess_instancesis a reasonable concurrency sanity checkSpawning three
SubProcessinstances that each sleep and print a unique index, then asserting exit status and output, is a good basic concurrency test. The 1‑second sleep is a pragmatic choice to avoid flakiness while still overlapping process lifetimes.
272-279:test_cmdresult_encodingconfirms encoding propagation and decodingBy forcing
encoding="utf-8"onrun()and then asserting on bothresult.encodingand decodedstdout_text, this test gives practical coverage of the encoding behavior clarified inCmdResult. Looks good.
280-287: Real‑worldbinary_from_shell_cmdpatterns are well coveredThe tests for:
"FOO=bar BAR=baz /usr/bin/python script.py --arg value"→/usr/bin/python"sudo -u user /bin/bash -c 'echo test'"→sudoalign with the intended behavior of
binary_from_shell_cmd()and complement the more synthetic unit tests.
|
Hi @richtja i have amended the commits towards your comments :) |
|
/gemini review |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the avocado/utils/process.py module by adding comprehensive docstrings, type hints, and examples to most functions and methods. It also enhances correctness by fixing subtle bugs in process handling, such as in pid_exists and binary_from_shell_cmd. The test coverage is substantially increased with new unit and functional tests that cover environment variable propagation, large outputs, and concurrent subprocesses. Overall, this is an excellent contribution that greatly improves the maintainability and reliability of the process utility module. I have a couple of minor suggestions regarding import placement in the new tests.
richtja
left a comment
richtja
left a comment
There was a problem hiding this comment.
Hi @harvey0100, beside the Gemini comments it LGTM. Please fix those and resolve the conflicts so we could get the CI results. Thank you.
Process Module Improved RST Docstrings Module Reference: avocado-framework/aautils#52 Assisted-By: Cursor-4-Sonnet Signed-off-by: Harvey Lynden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
spell.ignore (1)
820-820:substringallowlist seems fine, but consider whether you want to allow such a generic term.
If it’s only needed for a specific doc/test phrase, this is still acceptable; just be aware it may hide real typos elsewhere.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.pylintrc_utils(1 hunks)avocado/utils/process.py(43 hunks)selftests/check.py(1 hunks)selftests/functional/utils/process.py(3 hunks)selftests/unit/utils/process.py(3 hunks)spell.ignore(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .pylintrc_utils
- selftests/functional/utils/process.py
🧰 Additional context used
🧬 Code graph analysis (1)
selftests/unit/utils/process.py (1)
avocado/utils/process.py (10)
binary_from_shell_cmd(407-435)can_sudo(65-101)has_capability(136-161)pid_exists(164-187)CmdResult(444-530)kill_process_by_pattern(355-376)system(1149-1212)system_output(1216-1284)getoutput(1288-1346)getstatusoutput(1350-1413)
🪛 Ruff (0.14.8)
selftests/unit/utils/process.py
641-641: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (44)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: Linux with Python 3.13
- GitHub Check: Linux with Python 3.11
- GitHub Check: Linux with Python 3.10
- GitHub Check: Linux with Python 3.9
- GitHub Check: Linux with Python 3.12
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: Podman spawner with 3rd party runner plugin
- GitHub Check: Fedora selftests
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: Version task ubi:8.8
- GitHub Check: Windows with Python 3.11
- GitHub Check: Version task debian:12.4
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Build Package (wheel/tarball) for Python 3.11
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Code Coverage (3.11)
- GitHub Check: Static checks
- GitHub Check: macOS with Python 3.11
🔇 Additional comments (12)
spell.ignore (1)
817-818: Allowlistingpkill/pkill'sis consistent with existing spell-ignore conventions.
Possessive forms are already present elsewhere in this file, so this should behave as intended.selftests/check.py (1)
30-32: LGTM! Test count updates reflect expanded coverage.The updated test counts accurately reflect the new unit and functional tests added for process utilities in this PR.
avocado/utils/process.py (5)
143-147: Past review comment addressed successfully.The docstring now clearly notes that capability names are UPPERCASE in capabilities(7) but must be lowercase in Python. This addresses the previous review feedback.
422-429: Past review comment addressed successfully.The Linux-specific note has been added as requested in the previous review.
136-161: Well-executed API enhancement.The addition of the optional
pidparameter tohas_capabilityis properly documented and backward compatible. The docstring clearly explains the new functionality with helpful examples.
407-435: Good improvement: ValueError now raised for invalid input.The change to raise
ValueErrorwhen no binary can be extracted is properly documented and provides better error handling than the previous implicit behavior. The docstring clearly describes when this exception is raised.
505-516: Enhanced error handling documentation.The expanded docstring for
stdout_textproperty clearly documents theTypeErrorexception case, improving the API's usability.selftests/unit/utils/process.py (5)
1-1: Past review comment addressed: import moved to top.The
import errnostatement has been moved to the top of the file as requested in previous review feedback, following PEP 8 guidelines.
360-366: Good test coverage for ValueError behavior.This test properly validates the new
ValueErrorexception raised bybinary_from_shell_cmdwhen given invalid input, ensuring the API change is working as documented.
546-598: Comprehensive test coverage for can_sudo and has_capability.These tests thoroughly exercise the various code paths including:
- Root user behavior
- Missing sudo binary
- OSError handling
- PID-specific capability checking
The use of mocking appropriately isolates the tests from system state.
599-631: Thorough testing of PID and process pattern operations.The tests properly cover:
- PID existence checks with different errno values (ESRCH, EPERM)
- kill_process_by_pattern success and failure cases
This provides good coverage for the edge cases in process management.
632-682: Excellent coverage of system output functions.These tests validate all the system/system_output/getoutput/getstatusoutput functions with various options including:
- Exit code handling
- Output capture
- Newline stripping behavior
Note: The static analysis warning about
shell=Trueon line 641 is a false positive—using shell=True is appropriate here for testing thefalsecommand behavior.
Process Module Preperation Improvement of Unit/Functional Tests Reference: avocado-framework/aautils#52 Assisted-By: Cursor-4-Sonnet Signed-off-by: Harvey Lynden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
selftests/unit/utils/process.py (1)
1-1:errnoimport moved to the top (PEP8 E402) — resolves prior feedback.
🧹 Nitpick comments (3)
selftests/unit/utils/process.py (2)
546-579:can_sudo()tests look solid; consider asserting key calls to reduce false positives.
Right now the “happy path” cases mostly assert only the boolean; addingassert_called_*onavocado.utils.path.find_command/avocado.utils.process.systemwould make failures easier to diagnose.
599-617:pid_exists()ESRCH/EPERM semantics are correctly exercised.
Minor: you may want to assertos.kill(pid, 0)was called in the ESRCH/EPERM tests too (like the “exists” test does) for consistency.selftests/functional/utils/process.py (1)
238-253: MultipleSubProcessinstances test is good basic coverage for parallel usage.
If you want to be stricter about “concurrently”, you could assert total wall time is closer to ~1s than ~3s, but current coverage is still useful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
selftests/check.py(1 hunks)selftests/functional/utils/process.py(3 hunks)selftests/unit/utils/process.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- selftests/check.py
🧰 Additional context used
🧬 Code graph analysis (1)
selftests/unit/utils/process.py (1)
avocado/utils/process.py (10)
binary_from_shell_cmd(407-435)can_sudo(65-101)has_capability(136-161)pid_exists(164-187)CmdResult(444-530)kill_process_by_pattern(355-376)system(1149-1212)system_output(1216-1284)getoutput(1288-1346)getstatusoutput(1350-1413)
🪛 Ruff (0.14.8)
selftests/unit/utils/process.py
641-641: Function call with shell=True parameter identified, security issue
(S604)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (47)
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: Linux with Python 3.11
- GitHub Check: Linux with Python 3.10
- GitHub Check: Linux with Python 3.13
- GitHub Check: Linux with Python 3.9
- GitHub Check: Linux with Python 3.12
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-41-s390x
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-aarch64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-ppc64le
- GitHub Check: Version task debian:11.0
- GitHub Check: Egg task fedora:40
- GitHub Check: Build Package (wheel/tarball) for Python 3.11
- GitHub Check: Windows with Python 3.13
- GitHub Check: Build Package (wheel/tarball) for Python 3.13
- GitHub Check: Build Package (wheel/tarball) for Python 3.12
- GitHub Check: Build Package (wheel/tarball) for Python 3.10
- GitHub Check: Build Package (wheel/tarball) for Python 3.9
- GitHub Check: Version task ubuntu:22.04
- GitHub Check: Static checks
- GitHub Check: macOS with Python 3.11
- GitHub Check: Version task debian:12.4
- GitHub Check: Version task ubi:8.8
- GitHub Check: Fedora selftests
- GitHub Check: Code Coverage (3.11)
🔇 Additional comments (10)
selftests/unit/utils/process.py (3)
360-366: Good negative coverage forbinary_from_shell_cmd()(empty + only var-assignments).
580-598:has_capability(..., pid=...)behavior is covered and assertsget_capabilities(pid)usage.
618-631:kill_process_by_pattern()tests cover success + failure without raising.selftests/functional/utils/process.py (7)
125-142: Script templates are clear, and{{i}}escaping inLARGE_OUTPUT_SCRIPTis correct for.format().
158-169:_create_script()is a nice consolidation point (permissions + interpreter substitution).
215-229: Env propagation is validated for bothprocess.run()andprocess.system_output().
Potential portability note: direct execution via shebang may not work on non-POSIX; if that’s in-scope, consider a platform skip.
230-237:getstatusoutput()integration test is straightforward and asserts exact output.
254-271: Large output capture test is appropriately strict (exact line count + boundary checks).
272-279: Encoding handling is verified viastdout_textand explicitencoding="utf-8".
280-286: Real-world-ishbinary_from_shell_cmd()patterns are covered (env-prefix + sudo prefix).
| @unittest.skipUnless(ECHO_CMD, "Echo command not available in system") | ||
| def test_system_function(self): | ||
| """Test system() function returns exit code""" | ||
| exit_code = process.system(f"{ECHO_CMD} test", ignore_status=True) | ||
| self.assertEqual(exit_code, 0) | ||
|
|
||
| @unittest.skipUnless(ECHO_CMD, "Echo command not available in system") | ||
| def test_system_with_failure(self): | ||
| """Test system() with failing command""" | ||
| exit_code = process.system("false", ignore_status=True, shell=True) | ||
| self.assertNotEqual(exit_code, 0) | ||
|
|
||
| @unittest.skipUnless(ECHO_CMD, "Echo command not available in system") | ||
| def test_system_output_function(self): | ||
| """Test system_output() function returns output""" | ||
| output = process.system_output(f"{ECHO_CMD} -n hello", ignore_status=True) | ||
| self.assertEqual(output, b"hello") | ||
|
|
||
| @unittest.skipUnless(ECHO_CMD, "Echo command not available in system") | ||
| def test_system_output_strip_newline(self): | ||
| """Test system_output() strips trailing newline""" | ||
| output = process.system_output(f"{ECHO_CMD} hello", ignore_status=True) | ||
| self.assertEqual(output, b"hello") | ||
|
|
||
| @unittest.skipUnless(ECHO_CMD, "Echo command not available in system") | ||
| def test_system_output_no_strip(self): | ||
| """Test system_output() without stripping newline""" | ||
| output = process.system_output( | ||
| f"{ECHO_CMD} hello", ignore_status=True, strip_trail_nl=False | ||
| ) | ||
| self.assertEqual(output, b"hello\n") | ||
|
|
||
| @unittest.skipUnless(ECHO_CMD, "Echo command not available in system") | ||
| def test_getoutput_function(self): | ||
| """Test getoutput() function""" | ||
| output = process.getoutput(f"{ECHO_CMD} -n test") | ||
| self.assertEqual(output, "test") | ||
|
|
||
| @unittest.skipUnless(ECHO_CMD, "Echo command not available in system") | ||
| def test_getstatusoutput_function(self): | ||
| """Test getstatusoutput() function""" | ||
| status, output = process.getstatusoutput(f"{ECHO_CMD} hello") | ||
| self.assertEqual(status, 0) | ||
| self.assertEqual(output, "hello") | ||
|
|
||
| @unittest.skipUnless(ECHO_CMD, "Echo command not available in system") | ||
| def test_getstatusoutput_with_failure(self): | ||
| """Test getstatusoutput() with failing command""" | ||
| status, _output = process.getstatusoutput("false") | ||
| self.assertNotEqual(status, 0) | ||
|
|
There was a problem hiding this comment.
Fix Ruff S604 (shell=True) in test_system_with_failure to avoid lint/security gate failures.
If Ruff is enforced on tests, Line 641 will fail the check; false should work fine without shell=True.
- exit_code = process.system("false", ignore_status=True, shell=True)
+ exit_code = process.system("false", ignore_status=True)🧰 Tools
🪛 Ruff (0.14.8)
641-641: Function call with shell=True parameter identified, security issue
(S604)
🤖 Prompt for AI Agents
In selftests/unit/utils/process.py around lines 632 to 682, the
test_system_with_failure uses process.system("false", ignore_status=True,
shell=True) which triggers Ruff S604 (avoid shell=True); remove the shell=True
argument so the call is process.system("false", ignore_status=True) and adjust
the call signature if needed to run the command without a shell, keeping the
ignore_status=True and the assertNotEqual(status, 0) check unchanged.
clebergnu
left a comment
clebergnu
left a comment
There was a problem hiding this comment.
All review points (including Gemini's) have been addressed, and it LGTM. Thanks!
github-project-automation
bot
moved this from Review Requested
to Done 113
in Default project
3 participants
Summary by CodeRabbit
Bug Fixes
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.