Fix latent bugs in helpers and add unit tests - #3574
Merged
Conversation
str.capitalize() lowercases the rest of the string, so a key like "rewardAccount" was translated to "spsRewardaccount" instead of "spsRewardAccount" and the lookup failed on node 10.6.0+ params. Uppercase only the first character instead. Use slicing so that empty or too-short keys keep returning None instead of raising IndexError.
The conditional `orig_path.expanduser()` call discarded its result, so paths starting with ~ were returned unexpanded. Expand the user home directly when constructing the returned path.
decode_bech32 and encode_bech32 interpolated their input into a shell command line inside single quotes. Input containing a quote would break the command or allow injection. Pass the input via stdin and invoke the bech32 tool directly without a shell. Add stdin_data parameter to run_command and document its arguments.
When code running inside the environ context manager deleted a variable that was not set before entering, the restore step raised KeyError on `del os.environ[key]`. Use pop with a default instead.
The check_dir_arg_keep error message referred to check_dir_arg, misattributing the failing argparse option in user-facing output.
Indexing the result of split(":", maxsplit=1) with [1] raises
IndexError when the error message contains no colon. Use [-1] which
falls back to the whole message, and drop the now unneeded err_str
preassignment.
When the calling file path did not contain "cardano_node_tests", str.find returned -1 and the resulting GitHub URL was silently built from the last character of the path. Raise ValueError instead.
Naive str.split() broke quoted arguments containing spaces. Use shlex.split() for shell-like tokenization. Also convert list items to str so callers can pass e.g. Path objects without crashing the " ".join() used for logging.
With a negative num2 the computed interval was inverted (_min > _max) and the check always returned False. Take the absolute value of the fraction so the interval stays valid.
Single call instead of a generator loop with an unused loop variable.
Replace the manual block-read loop with hashlib.file_digest (Python 3.11+). Drop the blocksize parameter - no caller used it.
Use json.dump instead of building the whole string with json.dumps before writing it.
The command argument is already a string.
get_current_line_str and get_vcs_link contained an identical block for obtaining the caller's frame. Extract it into a _get_calling_frame helper that raises ValueError when the frame is unavailable.
Add docstrings to get_current_commit and get_line_str_from_frame, and clarify that run_command stdout may contain merged stderr.
Use Generator return type for contextmanager functions (Iterator annotation is deprecated by type checkers), parameterize bare Iterable/Generator/dict/list hints, and reflect that run_command accepts non-str list items.
git rev-parse ran in the current working directory, so when tests changed CWD the reported commit could come from a different repo (or fail). Run it in the directory of this source file instead. Resolves the standing TODO.
start_cluster joined the script path and its arguments into one string that run_command then re-tokenized with shlex, which would raise on args containing quotes and split args containing spaces. Pass the list directly.
Parameterize the remaining bare Iterable in flatten's ltypes hint, document the actual filename#L<lineno> format, and note that an empty GIT_REVISION falls through to git.
Cover the helpers touched by the preceding fixes: get_pool_param key translation, check_dir_arg_keep ~ expansion, environ restore, run_command tokenization/stdin/error handling, bech32 helper invocation, tool_has error parsing, is_in_interval with negative reference, and the remaining pure helpers. The tests don't depend on external binaries - subprocess tests use sys.executable and the bech32/tool_has tests monkeypatch run_command.
Splitting on the first ":" selected the wrong segment when the command string itself contained a colon, biasing the check toward reporting the capability as present. Partition on the "`: " delimiter that run_command actually emits.
A missing executable raised FileNotFoundError from Popen since the shell invocation was removed, giving run_command two failure types. Re-raise as RuntimeError naming the command, document raised exceptions and string tokenization in the docstring, and log ignored failures at debug level.
Document that get_current_commit is cached and anchored to this repo, that _get_calling_frame raises ValueError when the frame is unavailable, and reword the get_current_line_str NOTE to describe the actual frame-depth mechanism instead of a vague warning.
The stop script path was passed to run_command as a string and tokenized with shlex, so a state dir path containing a space or quote would be mis-split. Pass a single-item list instead, consistent with the start_cluster fix.
Add tests for start_cluster argv pass-through, run_in_bash invocation, run_command failure with merged stderr, missing executable error, empty GIT_REVISION fallback and environ restore on exception. Clarify the module docstring on shell builtins.
Popen raises FileNotFoundError for a missing workdir too, so the "Command not found" message could blame the wrong thing, and a non-executable file escaped as PermissionError. Catch OSError and re-raise as RuntimeError with neutral wording that covers all cases.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens cardano_node_tests.utils.helpers by fixing several latent bugs (notably around subprocess execution, env handling, and key transformations) and adds a new framework_tests/ unit-test suite that pins the corrected behavior without requiring a running cluster or project-specific binaries.
Changes:
- Add unit tests for
helpersandcluster_nodesunderframework_tests/, relying on monkeypatching instead of external tools. - Refactor and harden helper utilities (safer bech32 invocation, improved
run_command, bug fixes inenviron,get_pool_param,is_in_interval, etc.). - Update cluster start/stop call sites to pass argv lists into
run_command(avoids re-tokenization issues).
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| framework_tests/test_helpers.py | Adds broad unit coverage for helper utilities, including subprocess invocation semantics via monkeypatching. |
| framework_tests/test_cluster_nodes.py | Adds a focused unit test ensuring start_cluster forwards argv lists to run_command. |
| cardano_node_tests/utils/helpers.py | Implements the bug fixes/hardening (notably run_command, bech32 helpers, environ, tool_has, and key transformations). |
| cardano_node_tests/utils/cluster_nodes.py | Updates start_cluster to call run_command with an argv list. |
| cardano_node_tests/cluster_management/manager.py | Updates stop-script execution to use argv list calling convention. |
| cardano_node_tests/cluster_management/cluster_getter.py | Updates stop-script execution to use argv list calling convention. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
tool_has caught the RuntimeError that run_command now raises for execution failures, so a missing or non-executable tool was reported as having the capability. Raise the execution failure as a CommandExecError subclass and re-raise it in tool_has, since availability cannot be determined when the tool doesn't run at all.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (2)
cardano_node_tests/utils/helpers.py:147
- For list-form commands,
cmd_stris built with' '.join(cmd), which can misrepresent argv when an argument contains spaces/quotes (e.g. it will look like multiple args). Sincecmd_stris used in logs and error messages (and parsed bytool_has), consider usingshlex.join(cmd)to produce an unambiguous, shell-escaped representation of the actual argv.
else:
cmd = [str(c) for c in command]
cmd_str = " ".join(cmd)
cardano_node_tests/utils/helpers.py:415
get_pool_paramstill uses an unparameterizeddicttype forpool_params, while other updated helpers in this file use parameterized generics (e.g.dict[str, tp.Any]). Parameterizing this improves type clarity and matches the surrounding style.
def get_pool_param(key: str, *, pool_params: dict) -> tp.Any:
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Review of
cardano_node_tests/utils/helpers.pyuncovered several latent bugs andhardening opportunities. Each issue is fixed in its own commit, and new unit tests
in
framework_tests/pin the fixed behavior. The tests don't depend onproject-specific binaries or a running cluster.
Bug fixes
get_pool_param:str.capitalize()lowercased the rest of the key, so"rewardAccount"was looked up as"spsRewardaccount"and the lookup failed onnode 10.6.0+ params. Slicing-based transform now preserves camelCase and handles
degenerate keys (
"","sps") without raising.check_dir_arg_keep: theexpanduser()result was discarded, so~pathswere returned unexpanded and
testnet_cleanup_infosilently globbed nothing(reported balance 0). Error message also misattributed
check_dir_arg.decode_bech32/encode_bech32: input was interpolated into ashell=Truecommand line; a quote in the input broke the command or allowedinjection. Input is now passed via stdin without a shell. No
shell=Truecallers remain in the repo.
tool_has:IndexErroron error message without a colon, and splitting onthe first colon anywhere could report a missing CLI capability as present when
the command string contained a colon (e.g.
--opt=a:b).environ: restoring a variable that was unset before entering (and deletedinside the block) raised
KeyErrorfrom thefinallyclause, masking the realin-flight exception.
get_vcs_link: a calling path withoutcardano_node_testssilentlyproduced a one-character URL; now raises
ValueError.run_command: string commands are tokenized withshlex(quoted argumentssurvive), list items are
str()-converted (acceptsPath), and all execfailures (missing executable, missing workdir, non-executable file) are
normalized to
RuntimeError. Newstdin_dataparameter used by the bech32helpers. Ignored failures are logged at debug level.
get_current_commit:git rev-parsenow runs in the repo containing themodule instead of CWD, so tests that change directories can't report a foreign
repo commit. Resolves the standing TODO.
is_in_interval: negative reference value produced an inverted (empty)interval.
start_cluster/ stop script call sites: script paths and args are passedto
run_commandas argv lists instead of strings that get re-tokenized.Refactors and docs
random.choicesinget_rand_str,hashlib.file_digestinchecksum(unused
blocksizeparam dropped),json.dumpinwrite_json, deduplicatedcalling-frame lookup (
_get_calling_frame).Raises/Argssections and caching semanticsdocumented, contextmanager return hints use
tp.Generator, bare generic hintsparameterized.
Tests
framework_tests/test_helpers.py(89 tests) andframework_tests/test_cluster_nodes.py. Subprocess tests usesys.executableor POSIX shell builtins; helpers wrapping external tools monkeypatch
run_command;functools.cache/lru_cachestate is cleared around tests.