Added a test updater tool #6709
Conversation
* Added usage instructions * Added basic usability (debugging + automatic adding expectedFailure)
…king erroring but not crashing tests
…n amount of times the annotation loop can run + fixed bugs
Added known limitations
📝 WalkthroughWalkthroughA new shell script, Changes
Sequence DiagramsequenceDiagram
participant User
participant Script as update_tests.sh
participant FS as File System
participant Updater as lib_updater
participant Runner as RustPython
participant Parser as Output Parser
User->>Script: invoke with CLI options
Script->>Script: parse args, discover test files, determine ownership
Script->>FS: read/backup existing tests and skip metadata
Script->>Updater: request copy/update patches (concurrent)
Updater->>FS: apply patches & write updated tests
alt Dynamic annotation enabled
Script->>Runner: run tests for target lib
Runner-->>Parser: emit test run output
Parser->>Script: list of failing tests
Script->>FS: annotate failing tests (add skip/expectedFailure)
Script->>FS: reapply or restore skip metadata as needed
end
Script-->>User: summary and exit status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In @scripts/update_tests.sh:
- Around line 197-200: The if-condition uses the literal word "attempts" instead
of the variable value; change the test from if [[ attempts -gt 10 ]]; then to if
[[ $attempts -gt 10 ]]; then (or [[ $attempts -ge 10 ]] if you prefer inclusive)
so the numeric comparison evaluates the attempts variable; ensure the variable
attempts is initialized and incremented elsewhere so this condition can break
the loop when it exceeds the threshold.
- Line 159: Replace the combined declaration+assignment "local
output=$(rustpython $lib 2>&1)" with a separate declaration and capture so the
command's exit status isn't masked: first declare "local output" (and optionally
"local rc"), then run "output=$(rustpython $lib 2>&1)" and immediately save the
exit code with "rc=$?" (or check $?) before any other commands; use the saved rc
when deciding failure handling for the rustpython invocation.
- Around line 286-297: The readarray lines use unquoted command substitution
which allows shell globbing/word-splitting to corrupt the file list; update the
two occurrences of readarray -t libraries <<< $(find ...) so the command
substitution is quoted (e.g., readarray -t libraries <<< "$(find ...)" ) and
also ensure path vars used inside find (cpython_path and rpython_path) and the
grep pattern expansion (ignored_libraries) are properly quoted/escaped; this
preserves filenames with glob chars or spaces and keeps update_tests and
check_skips receiving the exact file list.
- Around line 96-98: The script sets cpython_path and rpython_path but doesn't
validate them; add a check after those assignments that when running in update
mode (i.e., not using the --update-skipped flag) you verify cpython_path is
non-empty and points to an existing directory (use test -d) and exit with a
clear error if it fails; do the same for rpython_path if it is required by
update operations; reference the cpython_path and rpython_path variables and
ensure any subsequent use assumes the validated paths.
🧹 Nitpick comments (9)
scripts/update_tests.sh (9)
69-72: Redundantexit 1afterusagecall.The
usagefunction already callsexit 1at line 37, so the explicitexit 1here is unreachable.Suggested fix
-h|--help) usage - exit 1 ;;
132-134: Unquoted variables and fragile boolean check.
$annotateis being used as a command (relying ontrue/falsebeing executables). This works but is fragile - prefer explicit comparison.- Variables passed to
annotate_libshould be quoted to handle paths with spaces.Suggested fix
- if [[ $annotate && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then - annotate_lib $lib $rlib_path + if [[ "$annotate" == true && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then + annotate_lib "$lib" "$rlib_path" fi
147-154: Unquoted variables in function calls.Variables should be quoted to handle paths with spaces correctly.
Suggested fix
check_skip() { local lib=$1 local rlib_path="$rpython_path/$lib" - remove_skips $rlib_path + remove_skips "$rlib_path" - annotate_lib $lib $rlib_path + annotate_lib "$lib" "$rlib_path" }
175-182: Unquoted variables in loop.Variables
$rlib_pathand$testshould be quoted to handle paths/names with special characters.Suggested fix
for test in "${failed_tests[@]}" do - if already_failed $rlib_path $test; then - replace_expected_with_skip $rlib_path $test + if already_failed "$rlib_path" "$test"; then + replace_expected_with_skip "$rlib_path" "$test" else - add_above_test $rlib_path $test "@unittest.expectedFailure # TODO: RUSTPYTHON" + add_above_test "$rlib_path" "$test" "@unittest.expectedFailure # TODO: RUSTPYTHON" fi done
207-211: Unquoted variables in sed command.Variables
$fileand$test_nameshould be quoted to prevent word splitting and glob expansion issues.Suggested fix
replace_expected_with_skip() { - file=$1 - test_name=$2 - sed -E "/^\s*@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip\('TODO: RUSTPYTHON'\)/ } }" -i $file + local file=$1 + local test_name=$2 + sed -E "/^\s*@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip\('TODO: RUSTPYTHON'\)/ } }" -i "$file" }
213-217: Grep output pollutes stdout; addlocaldeclarations.The
already_failedfunction's grep output goes to stdout, cluttering the script's output. Also, variables should be declaredlocal.Suggested fix
already_failed() { - file=$1 - test_name=$2 - grep -Pz "\s*@unittest\.expectedFailure # TODO: RUSTPYTHON\n\s*def\s+${test_name}\(" $file + local file=$1 + local test_name=$2 + grep -Pz "\s*@unittest\.expectedFailure # TODO: RUSTPYTHON\n\s*def\s+${test_name}\(" "$file" > /dev/null }
233-238: Potential sed injection if$linecontains special characters.The
$linevariable is used directly in the sed replacement without escaping. Characters like/,&, or\in the line content could break the sed command or cause unexpected substitutions. Currently safe given controlled inputs from this script, but fragile.Note
Consider escaping special characters in
$lineif this function might receive arbitrary input in the future:escaped_line=$(printf '%s\n' "$line" | sed 's/[&/\]/\\&/g')
240-248: Unquoted variable at end of sed command.
$rlib_pathshould be quoted to handle paths with spaces.Suggested fix
- sed -i -E '/^[[:space:]]*@unittest\.skip.*\(["'\'']TODO\s?:\s?RUSTPYTHON.*["'\'']\)/Id' $rlib_path + sed -i -E '/^[[:space:]]*@unittest\.skip.*\(["'\'']TODO\s?:\s?RUSTPYTHON.*["'\'']\)/Id' "$rlib_path" }
250-264: Missinglocaldeclaration formessagevariable.The
messagevariable should be declaredlocalto avoid polluting the global namespace.Suggested fix
apply_skip() { local rlib_path=$1 local test_name=$2 local hanging=$3 - message="unknown" + local message="unknown"
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/update_tests.sh
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to Lib/** : In the `Lib/` directory, when modifications are necessary, add a `# TODO: RUSTPYTHON` comment or use `unittest.skip('TODO: RustPython <reason>')` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: When testing Python code, always use RustPython instead of the standard `python` command; use `cargo run -- script.py` or `cargo run` for interactive REPL
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 6176
File: .github/workflows/Check_Tests.yml:133-141
Timestamp: 2025-09-28T22:22:55.921Z
Learning: In the RustPython project's CI-5974-Test-RustPython-Integration action, the override-rustpython-path input is marked as required but has runtime fallback logic that defaults to RUSTPYTHONPATH environment variable or "Lib" if neither is provided, making explicit specification unnecessary in most cases.
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to Lib/** : In the `Lib/` directory, when modifications are necessary, add a `# TODO: RUSTPYTHON` comment or use `unittest.skip('TODO: RustPython <reason>')` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment
Applied to files:
scripts/update_tests.sh
📚 Learning: 2025-08-30T14:40:05.858Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Applied to files:
scripts/update_tests.sh
🪛 Shellcheck (0.11.0)
scripts/update_tests.sh
[warning] 159-159: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 287-287: Quote this to prevent word splitting.
(SC2046)
[warning] 294-294: Quote this to prevent word splitting.
(SC2046)
⏰ 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). (11)
- GitHub Check: Run snippets and cpython tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (windows-2025)
- GitHub Check: Run snippets and cpython tests on wasm-wasi
- GitHub Check: Run rust tests (windows-2025)
- GitHub Check: Run rust tests (ubuntu-latest)
- GitHub Check: Run snippets and cpython tests (macos-latest)
- GitHub Check: Run rust tests (macos-latest)
- GitHub Check: Check the WASM package and demo
- GitHub Check: Ensure compilation on various targets
- GitHub Check: Run tests under miri
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (3)
scripts/update_tests.sh (3)
3-38: Well-documented usage function.The usage function provides comprehensive documentation including options, examples, notes, and known limitations. This is helpful for users of the script.
227-231: Simple semaphore implementation is adequate for the use case.The busy-wait approach with
sleep 0.1is reasonable for a developer tool. The known limitation about orphaned threads causing deadlock is properly documented in usage.
266-281: Backup mechanism for skip reasons is well-designed.The function properly extracts skip messages and associates them with test names for later restoration. Using
declare -gAfor the global associative array is appropriate here since it needs to be accessed byapply_skip. Based on learnings, this aligns with the RustPython convention of usingTODO: RUSTPYTHONin skip annotations.
| annotate_lib() { | ||
| local lib=${1//\//.} | ||
| local rlib_path=$2 | ||
| local output=$(rustpython $lib 2>&1) |
There was a problem hiding this comment.
Declare and assign separately to capture exit status.
If rustpython fails (e.g., cargo run fails), the exit status is masked by the assignment. This could lead to processing invalid output.
Suggested fix
- local output=$(rustpython $lib 2>&1)
+ local output
+ output=$(rustpython "$lib" 2>&1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| local output=$(rustpython $lib 2>&1) | |
| local output | |
| output=$(rustpython "$lib" 2>&1) |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 159-159: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In @scripts/update_tests.sh at line 159, Replace the combined
declaration+assignment "local output=$(rustpython $lib 2>&1)" with a separate
declaration and capture so the command's exit status isn't masked: first declare
"local output" (and optionally "local rc"), then run "output=$(rustpython $lib
2>&1)" and immediately save the exit code with "rc=$?" (or check $?) before any
other commands; use the saved rc when deciding failure handling for the
rustpython invocation.
scripts/update_tests.sh
Outdated
| if [[ ${#libraries[@]} -eq 0 ]]; then | ||
| readarray -t libraries <<< $(find ${cpython_path} -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | ||
| fi | ||
| update_tests "${libraries[@]}" | ||
| else | ||
| echo "Checking Skips" | ||
|
|
||
| if [[ ${#libraries[@]} -eq 0 ]]; then | ||
| readarray -t libraries <<< $(find ${rpython_path} -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | ||
| fi | ||
| check_skips "${libraries[@]}" | ||
| fi |
There was a problem hiding this comment.
Unquoted command substitution can cause glob expansion issues.
The $(find ...) command substitutions should be quoted to prevent potential glob expansion of file patterns like *.
Suggested fix
if [[ ${#libraries[@]} -eq 0 ]]; then
- readarray -t libraries <<< $(find ${cpython_path} -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
+ readarray -t libraries <<< "$(find "${cpython_path}" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")"
fi
update_tests "${libraries[@]}"
else
echo "Checking Skips"
if [[ ${#libraries[@]} -eq 0 ]]; then
- readarray -t libraries <<< $(find ${rpython_path} -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
+ readarray -t libraries <<< "$(find "${rpython_path}" -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")"
fi
check_skips "${libraries[@]}"
fi🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 287-287: Quote this to prevent word splitting.
(SC2046)
[warning] 294-294: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In @scripts/update_tests.sh around lines 286 - 297, The readarray lines use
unquoted command substitution which allows shell globbing/word-splitting to
corrupt the file list; update the two occurrences of readarray -t libraries <<<
$(find ...) so the command substitution is quoted (e.g., readarray -t libraries
<<< "$(find ...)" ) and also ensure path vars used inside find (cpython_path and
rpython_path) and the grep pattern expansion (ignored_libraries) are properly
quoted/escaped; this preserves filenames with glob chars or spaces and keeps
update_tests and check_skips receiving the exact file list.
youknowone
left a comment
There was a problem hiding this comment.
Currently fix_test.py have similar categorized feature that appending expectedFailure for failing tests.
I am not sure what you meant by A yet, but B sounds good.
I wish it can be is integrated as fix_test.py --try-unskip.
Could you share examples runs of this script what it solves?
The command and git diff after the command run.
So to clarify, what I mean by A, is that it can run updates on multiple files at once. As I could tell from the documentation, In the ideal case* you could run Also for that same reason, it's different from For example, it first tries to add
|
|
For example uses cases, see below: |
|
In terms of the Like I said it's very very janky, and there are a lot of issues/limitations that it has:
But it's good enough to at least get candidates for downgrading/removal (and makes the process for downgrading much smoother) imo |
…umentation + quieted grep
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @scripts/update_tests.sh:
- Around line 202-205: The if condition is comparing the literal string
"attempts" instead of the variable; change the test to reference the variable
(e.g., replace [[ attempts -gt 10 ]] with [[ $attempts -gt 10 ]] or use
arithmetic form (( attempts > 10 ))), so the loop can exit correctly when the
numeric attempts counter exceeds 10; keep the existing echo and return behavior.
- Line 22: In the scripts/update_tests.sh example usage the command contains an
extraneous dash and space ("$0 -c ~/cpython -r - . -t 300"); update that example
to remove the spurious "-" so it reads "$0 -c ~/cpython -r . -t 300" (i.e.,
replace "-r - ." with "-r .") to correct the typo.
- Around line 97-99: The script constructs cpython_path and rpython_path without
validating that the -c and -r options were provided, causing values like
"/Lib/test"; add a guard after argument parsing to check that the variables
cpython_path and rpython_path are non-empty (and optionally that the resulting
directories exist) and if either is missing print a clear error/usage message
and exit non-zero before the lines that set
cpython_path="$cpython_path/Lib/test" and rpython_path="$rpython_path/Lib/test".
Ensure you reference and validate the same variable names (cpython_path,
rpython_path) and fail-fast so the paths are never built from empty values.
- Around line 291-293: The unquoted command substitution can cause
word-splitting and the use of find -printf is GNU-specific; fix by replacing the
find -printf pipeline with a portable command and quoting the substitution: run
a subshell that cd's into "${cpython_path}" and uses portable find (e.g. find .
-type f -print) piped to sed 's|^\./||' to strip the leading ./, then grep -vE
"$(IFS=\|; echo "${ignored_libraries[*]}")"; finally assign with readarray -t
libraries <<< "$(cd "${cpython_path}" && find . -type f -print | sed 's|^\./||'
| grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")" so variables libraries,
cpython_path and ignored_libraries are quoted and portable tools are used.
🧹 Nitpick comments (8)
scripts/update_tests.sh (8)
70-72: Redundantexit 1afterusage.The
usagefunction already callsexit 1on line 38, making this exit statement unreachable.-h|--help) usage - exit 1 ;;
129-129: Quote variables to prevent word splitting.Arguments should be quoted to handle paths containing spaces.
- ./scripts/lib_updater.py --from $clib_path --to $rlib_path -o $rlib_path + ./scripts/lib_updater.py --from "$clib_path" --to "$rlib_path" -o "$rlib_path"
133-135: Quote variables in function call and use explicit boolean comparison.The boolean check works but is implicit (relies on empty string being falsy). Using explicit comparison is clearer, and arguments should be quoted.
- if [[ $annotate && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then - annotate_lib $lib $rlib_path + if [[ "$annotate" == true && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then + annotate_lib "$lib" "$rlib_path" fi
164-164: Declare and assign separately to avoid masking return values.Per static analysis (SC2155), if the command substitution fails, the exit status is masked by the assignment.
- local output=$(rustpython $lib 2>&1) + local output + output=$(rustpython "$lib" 2>&1)
180-187: Quote variables in function calls.Variables passed to functions should be quoted to prevent word splitting issues.
for test in "${failed_tests[@]}" do - if already_failed $rlib_path $test; then - replace_expected_with_skip $rlib_path $test + if already_failed "$rlib_path" "$test"; then + replace_expected_with_skip "$rlib_path" "$test" else - add_above_test $rlib_path $test "@unittest.expectedFailure # TODO: RUSTPYTHON" + add_above_test "$rlib_path" "$test" "@unittest.expectedFailure # TODO: RUSTPYTHON" fi done
212-216: Quote variables in sed command.Unquoted
$filecould break with paths containing spaces.replace_expected_with_skip() { - file=$1 - test_name=$2 - sed -E "/^\s*@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip\('TODO: RUSTPYTHON'\)/ } }" -i $file + local file=$1 + local test_name=$2 + sed -E "/^\s*@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip\('TODO: RUSTPYTHON'\)/ } }" -i "$file" }
238-243: Sed injection risk if$linecontains metacharacters.If the
$lineargument contains sed metacharacters (e.g.,/,&,\), the sed command could break or behave unexpectedly. Since$lineis controlled internally (decorator strings), this is low risk but worth noting.Consider escaping or using a different delimiter if this will handle varied input in the future.
252-252: Quote variable in sed command.- sed -i -E '/^[[:space:]]*@unittest\.skip.*\(["'\'']TODO\s?:\s?RUSTPYTHON.*["'\'']\)/Id' $rlib_path + sed -i -E '/^[[:space:]]*@unittest\.skip.*\(["'\'']TODO\s?:\s?RUSTPYTHON.*["'\'']\)/Id' "$rlib_path"
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/update_tests.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to Lib/** : In the `Lib/` directory, when modifications are necessary, add a `# TODO: RUSTPYTHON` comment or use `unittest.skip('TODO: RustPython <reason>')` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to Lib/** : In the `Lib/` directory, when modifications are necessary, add a `# TODO: RUSTPYTHON` comment or use `unittest.skip('TODO: RustPython <reason>')` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment
Applied to files:
scripts/update_tests.sh
📚 Learning: 2025-08-30T14:40:05.858Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Applied to files:
scripts/update_tests.sh
📚 Learning: 2025-09-28T22:22:55.921Z
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 6176
File: .github/workflows/Check_Tests.yml:133-141
Timestamp: 2025-09-28T22:22:55.921Z
Learning: In the RustPython project's CI-5974-Test-RustPython-Integration action, the override-rustpython-path input is marked as required but has runtime fallback logic that defaults to RUSTPYTHONPATH environment variable or "Lib" if neither is provided, making explicit specification unnecessary in most cases.
Applied to files:
scripts/update_tests.sh
🧬 Code graph analysis (1)
scripts/update_tests.sh (4)
crates/vm/src/function/argument.rs (1)
shift(147-149)crates/vm/src/exceptions.rs (1)
basename(2148-2155)crates/vm/src/builtins/list.rs (1)
sort(314-328)crates/vm/src/stdlib/builtins.rs (1)
all(46-53)
🪛 Shellcheck (0.11.0)
scripts/update_tests.sh
[warning] 164-164: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 292-292: Quote this to prevent word splitting.
(SC2046)
[warning] 299-299: Quote this to prevent word splitting.
(SC2046)
⏰ 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). (1)
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (1)
scripts/update_tests.sh (1)
228-236: LGTM!The
rustpythonwrapper and semaphore implementation are reasonable for this scripting context. The busy-wait insemis acceptable given the 0.1s sleep.
scripts/update_tests.sh
Outdated
| cpython_path="$cpython_path/Lib/test" | ||
| rpython_path="$rpython_path/Lib/test" | ||
|
|
There was a problem hiding this comment.
Missing validation for required paths.
If -c or -r are not provided, cpython_path or rpython_path remain empty, resulting in paths like /Lib/test which will silently cause incorrect behavior. Add validation before constructing these paths.
Proposed fix
+if [[ -z "$rpython_path" ]]; then
+ echo "Error: -r/--rpython-path is required" >&2
+ usage
+fi
+
+if [[ "$check_skip_flag" == false && -z "$cpython_path" ]]; then
+ echo "Error: -c/--cpython-path is required for updating tests" >&2
+ usage
+fi
+
cpython_path="$cpython_path/Lib/test"
rpython_path="$rpython_path/Lib/test"🤖 Prompt for AI Agents
In @scripts/update_tests.sh around lines 97 - 99, The script constructs
cpython_path and rpython_path without validating that the -c and -r options were
provided, causing values like "/Lib/test"; add a guard after argument parsing to
check that the variables cpython_path and rpython_path are non-empty (and
optionally that the resulting directories exist) and if either is missing print
a clear error/usage message and exit non-zero before the lines that set
cpython_path="$cpython_path/Lib/test" and rpython_path="$rpython_path/Lib/test".
Ensure you reference and validate the same variable names (cpython_path,
rpython_path) and fail-fast so the paths are never built from empty values.
scripts/update_tests.sh
Outdated
| if [[ attempts -gt 10 ]]; then | ||
| echo "Issue annotating $lib" >&2 | ||
| return; | ||
| fi |
There was a problem hiding this comment.
Bug: Missing $ prefix on variable attempts.
[[ attempts -gt 10 ]] compares the literal string "attempts" (always false), not the variable value. The loop will never exit early.
- if [[ attempts -gt 10 ]]; then
+ if [[ $attempts -gt 10 ]]; then
echo "Issue annotating $lib" >&2
return;
fi🤖 Prompt for AI Agents
In @scripts/update_tests.sh around lines 202 - 205, The if condition is
comparing the literal string "attempts" instead of the variable; change the test
to reference the variable (e.g., replace [[ attempts -gt 10 ]] with [[ $attempts
-gt 10 ]] or use arithmetic form (( attempts > 10 ))), so the loop can exit
correctly when the numeric attempts counter exceeds 10; keep the existing echo
and return behavior.
scripts/update_tests.sh
Outdated
| if [[ ${#libraries[@]} -eq 0 ]]; then | ||
| readarray -t libraries <<< $(find ${cpython_path} -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | ||
| fi |
There was a problem hiding this comment.
Quote command substitution and consider portability.
- Per static analysis (SC2046), quote the command substitution to prevent word splitting.
find -printfis GNU-specific and won't work on macOS/BSD. Consider using portable alternatives.
Proposed fix
if [[ ${#libraries[@]} -eq 0 ]]; then
- readarray -t libraries <<< $(find ${cpython_path} -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
+ readarray -t libraries < <(find "${cpython_path}" -type f -print | sed "s|^${cpython_path}/||" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ ${#libraries[@]} -eq 0 ]]; then | |
| readarray -t libraries <<< $(find ${cpython_path} -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | |
| fi | |
| if [[ ${#libraries[@]} -eq 0 ]]; then | |
| readarray -t libraries < <(find "${cpython_path}" -type f -print | sed "s|^${cpython_path}/||" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 292-292: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
In @scripts/update_tests.sh around lines 291 - 293, The unquoted command
substitution can cause word-splitting and the use of find -printf is
GNU-specific; fix by replacing the find -printf pipeline with a portable command
and quoting the substitution: run a subshell that cd's into "${cpython_path}"
and uses portable find (e.g. find . -type f -print) piped to sed 's|^\./||' to
strip the leading ./, then grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")";
finally assign with readarray -t libraries <<< "$(cd "${cpython_path}" && find .
-type f -print | sed 's|^\./||' | grep -vE "$(IFS=\|; echo
"${ignored_libraries[*]}")")" so variables libraries, cpython_path and
ignored_libraries are quoted and portable tools are used.
scripts/update_tests.sh
Outdated
| if [[ ${#libraries[@]} -eq 0 ]]; then | ||
| readarray -t libraries <<< $(find ${rpython_path} -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | ||
| fi |
There was a problem hiding this comment.
Same quoting and portability issues.
Apply the same fixes here for consistency.
if [[ ${#libraries[@]} -eq 0 ]]; then
- readarray -t libraries <<< $(find ${rpython_path} -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
+ readarray -t libraries < <(find "${rpython_path}" -iname "test_*.py" -type f -print | sed "s|^${rpython_path}/||" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ ${#libraries[@]} -eq 0 ]]; then | |
| readarray -t libraries <<< $(find ${rpython_path} -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | |
| fi | |
| if [[ ${#libraries[@]} -eq 0 ]]; then | |
| readarray -t libraries < <(find "${rpython_path}" -iname "test_*.py" -type f -print | sed "s|^${rpython_path}/||" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | |
| fi |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 299-299: Quote this to prevent word splitting.
(SC2046)
…s/formatting issues
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @scripts/update_tests.sh:
- Around line 271-275: The helper function replace_expected_with_skip should
quote the $file argument and preserve any existing skip reason instead of
injecting a generic one; update replace_expected_with_skip to call sed on
"$file" (quoted) and change the substitution to capture the original inline
comment/reason (e.g., the text after the # marker or after "TODO: RUSTPYTHON")
and reuse that captured reason inside the @unittest.skip(...) replacement so the
original reason is preserved while converting @unittest.expectedFailure to
@unittest.skip.
- Around line 201-250: In annotate_lib, avoid combining declaration and
assignment for output (currently local output=$(rustpython ...)) so the
rustpython exit code isn’t masked; instead declare local output first, run
output=$(rustpython $lib 2>&1) and capture its exit status ($?) immediately,
logging and returning on a non-zero catastrophic failure; also harden the retry
loop around attempts by detecting unchanged or oscillating output (e.g., keep
previous_output and if output == previous_output or it alternates between two
states for multiple iterations then abort with an error) so the while loop can’t
hang indefinitely even within the 15-attempt cap.
- Around line 132-138: The block calling lib_updater.py should check its exit
status and fail fast: after running ./scripts/lib_updater.py --from $clib_path
--to $rlib_path -o $rlib_path, test the command's return code and log/exit
(e.g., non-zero -> echo error and exit 1) instead of continuing silently; also
change the annotate conditional to explicitly test [[ $annotate == true ]] and
quote variables when calling annotate_lib (use annotate_lib "$lib" "$rlib_path")
so paths with spaces are handled safely.
- Around line 56-95: Argument parsing lacks validation: ensure each option that
does "shift 2" (flags -c/--cpython-path, -r/--rpython-path, -t/--timeout,
-j/--jobs) first checks that a following value ($2) exists and is not another
flag before shifting; after the while loop validate required variables (if
check_skip_flag is false require cpython_path, always require rpython_path),
verify that cpython_path and rpython_path point to existing directories, and
validate timeout and num_jobs are positive integers; update the case branches
for cpython_path/rpython_path/timeout/num_jobs to error and exit when $2
missing, and add post-parsing checks for existence and integer format for
timeout/num_jobs before proceeding.
🧹 Nitpick comments (5)
scripts/update_tests.sh (5)
70-73: Exit code inconsistency for--help.When
--helpis explicitly requested, it's conventional to exit with code 0 (success) rather than 1. Currently, both invalid usage and--helpexit with code 1.Suggested fix
-h|--help) usage - exit 1 + exit 0 ;;Also update the
usage()function to accept an optional exit code:usage() { cat >&2 <<EOF ... EOF exit "${1:-1}" }
98-98: Unused variableRUSTPYTHON_CANONICAL_SKIP_RE.This constant is defined but never referenced in the script. Either remove it or add a comment explaining its intended future use.
RUSTPYTHON_POSSIBLE_SKIP_RE="@unittest.skip.*\([\"']TODO:\s*RUSTPYTHON.*[\"']\)" -RUSTPYTHON_CANONICAL_SKIP_RE="@unittest.skip\('TODO: RUSTPYTHON; .*'\)" RUSTPYTHON_CANONICAL_EX_FAILURE="@unittest.expectedFailure # TODO: RUSTPYTHON"
258-263: Potentialsedpattern injection with special characters.The
$linevariable is interpolated directly into thesedreplacement string. If$linecontains&or\,sedwill interpret them specially (&= matched text,\n= backreference). While your current decorators don't contain these, this could cause issues if the pattern evolves.Additionally, if
$testcontains regex metacharacters (unlikely for Python function names but possible), the pattern could break.Consider escaping special characters or using a different approach:
add_above_test() { local file=$1 local test=$2 local line=$3 # Escape sed special chars in replacement local escaped_line="${line//\\/\\\\}" escaped_line="${escaped_line//&/\\&}" sed -i "s/^\([[:space:]]*\)def $test(/\1$escaped_line\n\1def $test(/" "$file" }
277-281: Portability concern:grep -Pis not universally available.The
-P(Perl regex) flag is a GNU grep extension and is not available on macOS/BSD systems by default. This limits the script's portability.Consider using
grep -Ewith a different pattern orpcregrepif Perl regex is required. Also quote variables:already_failed() { - file=$1 - test_name=$2 - grep -qPz "$RUSTPYTHON_CANONICAL_EX_FAILURE_RE\n\s*def\s+${test_name}\(" $file + local file=$1 + local test_name=$2 + grep -qPz "$RUSTPYTHON_CANONICAL_EX_FAILURE_RE\n\s*def\s+${test_name}\(" "$file" }For better portability, consider using
awkorsedfor multiline matching.
104-112: Parallel execution lacks error propagation.The semaphore pattern works but has limitations:
- Race condition between checking job count and spawning new job
- Background job failures are not captured -
waitdoesn't propagate errors- If a job crashes, the script continues without notice
For a tool that's explicitly "not meant to be used as a standalone updater" requiring manual review, this is acceptable but worth noting.
For more robust error handling in Bash 4.3+:
update_tests() { local libraries=("$@") local pids=() for lib in "${libraries[@]}"; do sem update_test "$lib" & pids+=($!) done local failed=0 for pid in "${pids[@]}"; do wait "$pid" || ((failed++)) done if ((failed > 0)); then echo "Warning: $failed update job(s) failed" >&2 fi }Also applies to: 252-256
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/update_tests.sh
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to Lib/** : In the `Lib/` directory, when modifications are necessary, add a `# TODO: RUSTPYTHON` comment or use `unittest.skip('TODO: RustPython <reason>')` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
📚 Learning: 2025-08-30T14:40:05.858Z
Learnt from: ShaharNaveh
Repo: RustPython/RustPython PR: 6089
File: scripts/lib_updater.py:292-297
Timestamp: 2025-08-30T14:40:05.858Z
Learning: In scripts/lib_updater.py, the --inplace flag intentionally writes to orig_file (not remote_file) even though patches are applied to remote_file content. This workflow allows updating the original RustPython test file with patches applied to new upstream CPython content.
Applied to files:
scripts/update_tests.sh
📚 Learning: 2025-12-27T14:03:49.034Z
Learnt from: CR
Repo: RustPython/RustPython PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-12-27T14:03:49.034Z
Learning: Applies to Lib/** : In the `Lib/` directory, when modifications are necessary, add a `# TODO: RUSTPYTHON` comment or use `unittest.skip('TODO: RustPython <reason>')` or `unittest.expectedFailure` with `# TODO: RUSTPYTHON <reason>` comment
Applied to files:
scripts/update_tests.sh
📚 Learning: 2025-09-28T22:22:55.921Z
Learnt from: reactive-firewall
Repo: RustPython/RustPython PR: 6176
File: .github/workflows/Check_Tests.yml:133-141
Timestamp: 2025-09-28T22:22:55.921Z
Learning: In the RustPython project's CI-5974-Test-RustPython-Integration action, the override-rustpython-path input is marked as required but has runtime fallback logic that defaults to RUSTPYTHONPATH environment variable or "Lib" if neither is provided, making explicit specification unnecessary in most cases.
Applied to files:
scripts/update_tests.sh
🪛 Shellcheck (0.11.0)
scripts/update_tests.sh
[warning] 98-98: RUSTPYTHON_CANONICAL_SKIP_RE appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 204-204: Declare and assign separately to avoid masking return values.
(SC2155)
[warning] 304-304: Quote this to prevent word splitting.
(SC2046)
[warning] 312-312: Quote this to prevent word splitting.
(SC2046)
⏰ 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). (1)
- GitHub Check: Check Rust code with clippy
🔇 Additional comments (1)
scripts/update_tests.sh (1)
1-39: Good documentation of usage, examples, and known limitations.The comprehensive help message with examples and clearly documented limitations helps users understand the tool's scope and rough edges. This aligns well with the author's stated intent that "Care needs to be taken to review everything done."
Based on learnings, the use of
@unittest.skip('TODO: RUSTPYTHON...')and@unittest.expectedFailure # TODO: RUSTPYTHONpatterns matches the project conventions for marking RustPython-specific test modifications.
| while [[ $# -gt 0 ]]; do | ||
| case "$1" in | ||
| -c|--cpython-path) | ||
| cpython_path="$2/Lib/test" | ||
| shift 2 | ||
| ;; | ||
| -r|--rpython-path) | ||
| rpython_path="$2/Lib/test" | ||
| shift 2 | ||
| ;; | ||
| -u|--copy-untracked) | ||
| copy_untracked=true | ||
| shift | ||
| ;; | ||
| -h|--help) | ||
| usage | ||
| exit 1 | ||
| ;; | ||
| -s|--update-skipped) | ||
| check_skip_flag=true | ||
| shift | ||
| ;; | ||
| -t|--timeout) | ||
| timeout="$2" | ||
| shift 2 | ||
| ;; | ||
| -a|--annotate) | ||
| annotate=true | ||
| shift | ||
| ;; | ||
| -j|--jobs) | ||
| num_jobs="$2" | ||
| shift 2 | ||
| ;; | ||
| *) | ||
| libraries+=("$1") | ||
| shift | ||
| ;; | ||
| esac | ||
| done |
There was a problem hiding this comment.
Missing validation for required arguments and option values.
Several issues with argument parsing:
- Options like
-t,-j,-c,-rdon't validate that$2exists beforeshift 2, which can silently consume the next flag. - No validation that
cpython_pathis provided when not using--update-skipped. - No validation that paths exist or that
timeout/num_jobsare valid integers.
Suggested validation pattern
-t|--timeout)
+ if [[ -z "$2" || "$2" == -* ]]; then
+ echo "Error: -t requires a numeric value" >&2
+ exit 1
+ fi
timeout="$2"
shift 2
;;Add post-parsing validation:
# After the while loop
if ! $check_skip_flag && [[ -z "$cpython_path" ]]; then
echo "Error: --cpython-path is required for update mode" >&2
exit 1
fi
if [[ -z "$rpython_path" ]]; then
echo "Error: --rpython-path is required" >&2
exit 1
fi
if [[ ! -d "$cpython_path" ]] && ! $check_skip_flag; then
echo "Error: CPython path does not exist: $cpython_path" >&2
exit 1
fi🤖 Prompt for AI Agents
In @scripts/update_tests.sh around lines 56 - 95, Argument parsing lacks
validation: ensure each option that does "shift 2" (flags -c/--cpython-path,
-r/--rpython-path, -t/--timeout, -j/--jobs) first checks that a following value
($2) exists and is not another flag before shifting; after the while loop
validate required variables (if check_skip_flag is false require cpython_path,
always require rpython_path), verify that cpython_path and rpython_path point to
existing directories, and validate timeout and num_jobs are positive integers;
update the case branches for cpython_path/rpython_path/timeout/num_jobs to error
and exit when $2 missing, and add post-parsing checks for existence and integer
format for timeout/num_jobs before proceeding.
| ./scripts/lib_updater.py --from $clib_path --to $rlib_path -o $rlib_path | ||
| fi | ||
|
|
||
|
|
||
| if [[ $annotate && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then | ||
| annotate_lib $lib $rlib_path | ||
| fi |
There was a problem hiding this comment.
Missing error handling and variable quoting.
- No error handling if
lib_updater.pyfails - the script continues silently. - Variables passed to
annotate_libshould be quoted to handle paths with spaces. - The boolean check
$annotateworks but[[ $annotate == true ]]is clearer.
Suggested improvements
else
echo "Using lib_updater to update $lib"
- ./scripts/lib_updater.py --from $clib_path --to $rlib_path -o $rlib_path
+ if ! ./scripts/lib_updater.py --from "$clib_path" --to "$rlib_path" -o "$rlib_path"; then
+ echo "Warning: lib_updater.py failed for $lib" >&2
+ fi
fi
- if [[ $annotate && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then
- annotate_lib $lib $rlib_path
+ if [[ $annotate == true && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then
+ annotate_lib "$lib" "$rlib_path"
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ./scripts/lib_updater.py --from $clib_path --to $rlib_path -o $rlib_path | |
| fi | |
| if [[ $annotate && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then | |
| annotate_lib $lib $rlib_path | |
| fi | |
| if ! ./scripts/lib_updater.py --from "$clib_path" --to "$rlib_path" -o "$rlib_path"; then | |
| echo "Warning: lib_updater.py failed for $lib" >&2 | |
| fi | |
| fi | |
| if [[ $annotate == true && -f "$rlib_path" && $(basename -- "$rlib_path") == test_*.py ]]; then | |
| annotate_lib "$lib" "$rlib_path" | |
| fi |
🤖 Prompt for AI Agents
In @scripts/update_tests.sh around lines 132 - 138, The block calling
lib_updater.py should check its exit status and fail fast: after running
./scripts/lib_updater.py --from $clib_path --to $rlib_path -o $rlib_path, test
the command's return code and log/exit (e.g., non-zero -> echo error and exit 1)
instead of continuing silently; also change the annotate conditional to
explicitly test [[ $annotate == true ]] and quote variables when calling
annotate_lib (use annotate_lib "$lib" "$rlib_path") so paths with spaces are
handled safely.
| annotate_lib() { | ||
| local lib=${1//\//.} | ||
| local rlib_path=$2 | ||
| local output=$(rustpython $lib 2>&1) | ||
|
|
||
| if grep -q "NO TESTS RAN" <<< "$output"; then | ||
| echo "No tests ran in $lib. skipping annotation" | ||
| return | ||
| fi | ||
|
|
||
| echo "Annotating $lib" | ||
|
|
||
| local attempts=0 | ||
| while ! grep -q "Tests result: SUCCESS" <<< "$output"; do | ||
| ((attempts++)) | ||
| # echo "$lib failing, annotating..." | ||
| readarray -t failed_tests <<< "$(echo "$output" | awk '/^(FAIL:|ERROR:)/ {print $2}' | sort -u)" | ||
|
|
||
| # If the test fails/errors, then expectedFailure it | ||
| for test in "${failed_tests[@]}" | ||
| do | ||
| if already_failed $rlib_path $test; then | ||
| replace_expected_with_skip $rlib_path $test | ||
| else | ||
| add_above_test $rlib_path $test "$RUSTPYTHON_CANONICAL_EX_FAILURE" | ||
| fi | ||
| done | ||
|
|
||
| # If the test crashes/hangs, then skip it | ||
| if grep -q "\.\.\.$" <<< "$output"; then | ||
| crashing_test=$(echo "$output" | grep '\.\.\.$' | head -n 1 | awk '{print $1}') | ||
| if grep -q "Timeout" <<< "$output"; then | ||
| hanging=true | ||
| else | ||
| hanging=false | ||
| fi | ||
| apply_skip "$rlib_path" "$crashing_test" $hanging | ||
| fi | ||
|
|
||
| output=$(rustpython $lib 2>&1) | ||
|
|
||
| if [[ $attempts -gt 15 ]]; then | ||
| echo "Issue annotating $lib" >&2 | ||
| return; | ||
| fi | ||
| done | ||
| echo "Successfully updated $lib" | ||
|
|
||
| unset SKIP_BACKUP | ||
| } |
There was a problem hiding this comment.
Declare and assign separately to avoid masking return values.
The combined declaration and assignment on line 204 masks the exit status of rustpython. If rustpython fails catastrophically, you won't detect it.
Also, the 15-attempt limit is reasonable but the logic could still get stuck if tests alternate between different failure modes.
Suggested fix for SC2155
annotate_lib() {
local lib=${1//\//.}
local rlib_path=$2
- local output=$(rustpython $lib 2>&1)
+ local output
+ output=$(rustpython "$lib" 2>&1)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| annotate_lib() { | |
| local lib=${1//\//.} | |
| local rlib_path=$2 | |
| local output=$(rustpython $lib 2>&1) | |
| if grep -q "NO TESTS RAN" <<< "$output"; then | |
| echo "No tests ran in $lib. skipping annotation" | |
| return | |
| fi | |
| echo "Annotating $lib" | |
| local attempts=0 | |
| while ! grep -q "Tests result: SUCCESS" <<< "$output"; do | |
| ((attempts++)) | |
| # echo "$lib failing, annotating..." | |
| readarray -t failed_tests <<< "$(echo "$output" | awk '/^(FAIL:|ERROR:)/ {print $2}' | sort -u)" | |
| # If the test fails/errors, then expectedFailure it | |
| for test in "${failed_tests[@]}" | |
| do | |
| if already_failed $rlib_path $test; then | |
| replace_expected_with_skip $rlib_path $test | |
| else | |
| add_above_test $rlib_path $test "$RUSTPYTHON_CANONICAL_EX_FAILURE" | |
| fi | |
| done | |
| # If the test crashes/hangs, then skip it | |
| if grep -q "\.\.\.$" <<< "$output"; then | |
| crashing_test=$(echo "$output" | grep '\.\.\.$' | head -n 1 | awk '{print $1}') | |
| if grep -q "Timeout" <<< "$output"; then | |
| hanging=true | |
| else | |
| hanging=false | |
| fi | |
| apply_skip "$rlib_path" "$crashing_test" $hanging | |
| fi | |
| output=$(rustpython $lib 2>&1) | |
| if [[ $attempts -gt 15 ]]; then | |
| echo "Issue annotating $lib" >&2 | |
| return; | |
| fi | |
| done | |
| echo "Successfully updated $lib" | |
| unset SKIP_BACKUP | |
| } | |
| annotate_lib() { | |
| local lib=${1//\//.} | |
| local rlib_path=$2 | |
| local output | |
| output=$(rustpython "$lib" 2>&1) | |
| if grep -q "NO TESTS RAN" <<< "$output"; then | |
| echo "No tests ran in $lib. skipping annotation" | |
| return | |
| fi | |
| echo "Annotating $lib" | |
| local attempts=0 | |
| while ! grep -q "Tests result: SUCCESS" <<< "$output"; do | |
| ((attempts++)) | |
| # echo "$lib failing, annotating..." | |
| readarray -t failed_tests <<< "$(echo "$output" | awk '/^(FAIL:|ERROR:)/ {print $2}' | sort -u)" | |
| # If the test fails/errors, then expectedFailure it | |
| for test in "${failed_tests[@]}" | |
| do | |
| if already_failed $rlib_path $test; then | |
| replace_expected_with_skip $rlib_path $test | |
| else | |
| add_above_test $rlib_path $test "$RUSTPYTHON_CANONICAL_EX_FAILURE" | |
| fi | |
| done | |
| # If the test crashes/hangs, then skip it | |
| if grep -q "\.\.\.$" <<< "$output"; then | |
| crashing_test=$(echo "$output" | grep '\.\.\.$' | head -n 1 | awk '{print $1}') | |
| if grep -q "Timeout" <<< "$output"; then | |
| hanging=true | |
| else | |
| hanging=false | |
| fi | |
| apply_skip "$rlib_path" "$crashing_test" $hanging | |
| fi | |
| output=$(rustpython $lib 2>&1) | |
| if [[ $attempts -gt 15 ]]; then | |
| echo "Issue annotating $lib" >&2 | |
| return; | |
| fi | |
| done | |
| echo "Successfully updated $lib" | |
| unset SKIP_BACKUP | |
| } |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 204-204: Declare and assign separately to avoid masking return values.
(SC2155)
🤖 Prompt for AI Agents
In @scripts/update_tests.sh around lines 201 - 250, In annotate_lib, avoid
combining declaration and assignment for output (currently local
output=$(rustpython ...)) so the rustpython exit code isn’t masked; instead
declare local output first, run output=$(rustpython $lib 2>&1) and capture its
exit status ($?) immediately, logging and returning on a non-zero catastrophic
failure; also harden the retry loop around attempts by detecting unchanged or
oscillating output (e.g., keep previous_output and if output == previous_output
or it alternates between two states for multiple iterations then abort with an
error) so the while loop can’t hang indefinitely even within the 15-attempt cap.
| replace_expected_with_skip() { | ||
| file=$1 | ||
| test_name=$2 | ||
| sed -E "/$RUSTPYTHON_CANONICAL_EX_FAILURE_RE/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip\('TODO: RUSTPYTHON'\)/ } }" -i $file | ||
| } |
There was a problem hiding this comment.
Missing variable quoting and lost skip reason.
$fileshould be quoted to handle paths with spaces.- The replacement loses any specific skip reason, converting to a generic message. Consider preserving the reason if one exists.
Suggested fix
replace_expected_with_skip() {
- file=$1
- test_name=$2
- sed -E "/$RUSTPYTHON_CANONICAL_EX_FAILURE_RE/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip\('TODO: RUSTPYTHON'\)/ } }" -i $file
+ local file=$1
+ local test_name=$2
+ sed -E "/$RUSTPYTHON_CANONICAL_EX_FAILURE_RE/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip('TODO: RUSTPYTHON')/ } }" -i "$file"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| replace_expected_with_skip() { | |
| file=$1 | |
| test_name=$2 | |
| sed -E "/$RUSTPYTHON_CANONICAL_EX_FAILURE_RE/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip\('TODO: RUSTPYTHON'\)/ } }" -i $file | |
| } | |
| replace_expected_with_skip() { | |
| local file=$1 | |
| local test_name=$2 | |
| sed -E "/$RUSTPYTHON_CANONICAL_EX_FAILURE_RE/ { N; /\n\s*def $test_name/ { s/^(\s*)@unittest\.expectedFailure\s+# TODO: RUSTPYTHON/\1@unittest.skip('TODO: RUSTPYTHON')/ } }" -i "$file" | |
| } |
🤖 Prompt for AI Agents
In @scripts/update_tests.sh around lines 271 - 275, The helper function
replace_expected_with_skip should quote the $file argument and preserve any
existing skip reason instead of injecting a generic one; update
replace_expected_with_skip to call sed on "$file" (quoted) and change the
substitution to capture the original inline comment/reason (e.g., the text after
the # marker or after "TODO: RUSTPYTHON") and reuse that captured reason inside
the @unittest.skip(...) replacement so the original reason is preserved while
converting @unittest.expectedFailure to @unittest.skip.
| if [[ "${#libraries[@]}" -eq 0 ]]; then | ||
| readarray -t libraries <<< $(find ${cpython_path} -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | ||
| fi | ||
| update_tests "${libraries[@]}" | ||
| else | ||
| echo "Checking Skips" | ||
|
|
||
| # If libraries are not specified, then check all tests | ||
| if [[ ${#libraries[@]} -eq 0 ]]; then | ||
| readarray -t libraries <<< $(find ${rpython_path} -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | ||
| fi | ||
| check_skips "${libraries[@]}" |
There was a problem hiding this comment.
Word splitting issues and empty result handling (SC2046).
The find command output should be properly handled to avoid word splitting issues. Additionally, if find returns no results, the array will contain one empty element rather than being empty.
Suggested fix using process substitution
# If libraries are not specified, then update all tests
if [[ "${#libraries[@]}" -eq 0 ]]; then
- readarray -t libraries <<< $(find ${cpython_path} -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
+ readarray -t libraries < <(find "${cpython_path}" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
+ # Remove empty elements
+ libraries=("${libraries[@]}")
fiSame pattern for line 312:
if [[ ${#libraries[@]} -eq 0 ]]; then
- readarray -t libraries <<< $(find ${rpython_path} -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
+ readarray -t libraries < <(find "${rpython_path}" -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")")
fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if [[ "${#libraries[@]}" -eq 0 ]]; then | |
| readarray -t libraries <<< $(find ${cpython_path} -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | |
| fi | |
| update_tests "${libraries[@]}" | |
| else | |
| echo "Checking Skips" | |
| # If libraries are not specified, then check all tests | |
| if [[ ${#libraries[@]} -eq 0 ]]; then | |
| readarray -t libraries <<< $(find ${rpython_path} -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | |
| fi | |
| check_skips "${libraries[@]}" | |
| if [[ "${#libraries[@]}" -eq 0 ]]; then | |
| readarray -t libraries < <(find "${cpython_path}" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | |
| fi | |
| update_tests "${libraries[@]}" | |
| else | |
| echo "Checking Skips" | |
| # If libraries are not specified, then check all tests | |
| if [[ ${#libraries[@]} -eq 0 ]]; then | |
| readarray -t libraries < <(find "${rpython_path}" -iname "test_*.py" -type f -printf "%P\n" | grep -vE "$(IFS=\|; echo "${ignored_libraries[*]}")") | |
| fi | |
| check_skips "${libraries[@]}" |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 304-304: Quote this to prevent word splitting.
(SC2046)
[warning] 312-312: Quote this to prevent word splitting.
(SC2046)
|
I am good for adding automatic unskip, but oppose to automatic skip. The point of It will be also great if the script is a python script, not a bash because we have to support also windows. |
|
To clarify, there are two ways that the skips are applied;
Are these situations ok to annotate a test as skip, or should I prevent it anyways? I can make the change for porting it to Python, but it's probably something I'll tackle later as well as merging |
|
Also, one last thing: Using this, there are a lot of tests that can be changed. Should I make a separate PR for each test/group of tests, or just make one single PR? (So far there are 55 tests that are changed, in reality, there will be less that are true positives) |
We are skipping tests in one of 4 cases (from most common to most rare):
|
Yes! That way we can start benefiting from the tool even before it gets merged. No need to do 1 PR for each test |
Nice, good to know! |
A new feature added by #6410 is the |
Hey!
This is adding a small script to:
A. Make better use of the
lib_updater.pyto better update files en masse (along with automatically marking tests as skip or expectedFailure)B. Test out existing tests which are annotated
@unittest.skipto see if they can be replaced with@unittest.expectedFailureIt is currently still very janky, but it does it's job alright-ish, and while there still is some amount of manual work that needs to be done, it's mainly just checking + undoing actions in git (rather than manual replacing + checking)
I'm probably not going to add too much more (since like I mentioned before it does it's job alright + diminishing returns), but please let me know if it's something of interest to be merged. If so, I’ll likely make some adjustments to make it more platform-agnostic and tidy up the formatting, etc.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.