gemini-code-assist[bot] commented on code in PR #35733:
URL: https://github.com/apache/beam/pull/35733#discussion_r2241361503
##########
sdks/python/scripts/run_pytest.sh:
##########
@@ -63,7 +63,7 @@ if [[ -n $user_marker ]]; then
fi
# Run tests in parallel.
-echo "Running parallel tests with: pytest -m \"$marker_for_parallel_tests\"
$pytest_args"
+echo "Running parallel tests with: pytest -m \"$marker_for_parallel_tests\"
$posargs"
pytest -v -rs -o junit_suite_name=${envname} \
--junitxml=pytest_${envname}.xml -m "$marker_for_parallel_tests" -n 6
--import-mode=importlib ${pytest_args} --pyargs ${posargs}
Review Comment:
That's a great question, @mohamedawnallah! Parsing shell arguments robustly
can be tricky, especially with quoting. The goal is to correctly separate the
actual test paths/package names from `pytest` options (like `-k` or `--cov`).
Here's a more robust way to parse the `$posargs` variable using `eval "set
-- $posargs"` which correctly handles quoted arguments. You can insert this
logic before line 68 in `sdks/python/scripts/run_pytest.sh`:
```bash
# Parse posargs to separate test paths/packages from other pytest options.
# This handles arguments that start with '-' as options, and others as
paths/packages.
# It correctly handles quoted arguments (e.g., -k "foo bar").
parsed_test_paths_or_packages=""
parsed_pytest_options=""
# Safely split the posargs string into individual arguments
eval "set -- $posargs"
# Iterate through the arguments
while [[ $# -gt 0 ]]; do
arg="$1"
case "$arg" in
-*) # This is an option
parsed_pytest_options+=" $arg"
shift
# Special handling for options that take an argument (e.g., -k)
# This assumes the argument to -k is the next token.
if [[ "$arg" == "-k" ]]; then
if [[ $# -gt 0 ]]; then
# Re-quote the argument to preserve it if it contained spaces
parsed_pytest_options+=" \"$1\""
shift
fi
fi
;;
*) # This is a positional argument (assumed to be a test path or package
name)
parsed_test_paths_or_packages+=" $arg"
shift
;;
esac
done
# Construct the final arguments to be passed to pytest on line 68.
# The original ${pytest_args} variable should still contain its intended
options.
# We append the newly parsed user options and the --pyargs section.
pytest_pyargs_section=""
if [[ -n "$parsed_test_paths_or_packages" ]]; then
pytest_pyargs_section="--pyargs ${parsed_test_paths_or_packages}"
fi
# This variable will replace '--pyargs ${posargs}' on line 68.
# It combines the user-provided options and the correctly formed --pyargs
section.
final_posargs_for_pytest="${parsed_pytest_options} ${pytest_pyargs_section}"
```
Then, you would modify line 68 to use `final_posargs_for_pytest`:
```bash
# Original line 68:
# --junitxml=pytest_${envname}.xml -m "$marker_for_parallel_tests" -n 6
--import-mode=importlib ${pytest_args} --pyargs ${posargs}
# Modified line 68:
--junitxml=pytest_${envname}.xml -m "$marker_for_parallel_tests" -n 6
--import-mode=importlib ${pytest_args} ${final_posargs_for_pytest}
```
This approach ensures that only actual test paths or package names are
passed to `--pyargs`, and other `pytest` options are passed as regular
arguments, resolving the issue of `pytest` misinterpreting options as package
names. Remember to adjust the `case` statement if there are other specific
options in `$posargs` that take arguments.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
