[WIP] Refactor: remove legacy run_example.py and ci.py runners#590
[WIP] Refactor: remove legacy run_example.py and ci.py runners#590doraemonmj wants to merge 1 commit intohw-native-sys:mainfrom
Conversation
All golden.py-based tests have been migrated to pytest @scene_test format, making both legacy runners dead code. - Delete examples/scripts/run_example.py and ci.py - Remove ci.py steps from CI workflow; add --clone-protocol https to all first-attempt pytest calls so PTO-ISA clones via HTTPS in CI - Update conftest.py to pre-clone PTO-ISA when --clone-protocol is non-default, replacing ci.py's pre-clone responsibility - Update ci.sh run_task() to use test_*.py instead of run_example.py - Remove run_example.py fallback from tools/benchmark_rounds.sh - Remove ci.py and run_example.py smoke tests from verify_packaging.sh
![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 transitions the CI and benchmarking workflows to use standalone test_*.py files by removing ci.py and run_example.py. Feedback identifies that skipping tasks should return a success code to prevent unnecessary CI retries and recommends using python3 for better environment compatibility. Additionally, the reviewer noted that the current file discovery logic using find only executes the first test file found, which could lead to reduced test coverage if multiple test files exist in a directory.
| -p "$platform" --clone-protocol "$CLONE_PROTOCOL" "${commit_flag[@]}") | ||
| else | ||
| echo "[${platform}] SKIP: no test_*.py found in $dir" | ||
| return 1 |
There was a problem hiding this comment.
Returning 1 here will cause the task to be marked as a failure in the caller's logic (e.g., line 400 or 457), leading to unnecessary retries and a non-zero exit code for the CI session. Since this is explicitly logged as a SKIP, it should return 0 to allow the CI to continue, or be logged as an ERROR if the absence of a test file is considered a terminal failure for that task. Note that benchmark_rounds.sh correctly returns 0 in this scenario.
| return 1 | |
| return 0 |
| -p "$platform" --clone-protocol "$CLONE_PROTOCOL" "${commit_flag[@]}") | ||
| # Prefer test_*.py if available | ||
| local test_file | ||
| test_file=$(find "$dir" -maxdepth 1 -name 'test_*.py' -print -quit 2>/dev/null || true) |
There was a problem hiding this comment.
Using find ... -print -quit will only identify and execute the first test_*.py file found in the directory. If a test suite is split across multiple files (e.g., test_basic.py and test_advanced.py), this approach will silently skip all but one, leading to a regression in test coverage. Consider iterating over all matching files or using pytest to execute the directory if the environment supports it.
| local test_file | ||
| test_file=$(find "$dir" -maxdepth 1 -name 'test_*.py' -print -quit 2>/dev/null || true) | ||
| if [[ -n "$test_file" ]]; then | ||
| cmd=(env PYTHONDONTWRITEBYTECODE=1 python "$test_file" |
There was a problem hiding this comment.
It is recommended to use python3 explicitly to ensure the correct interpreter is used, especially since benchmark_rounds.sh was updated to use python3 in this PR. This improves robustness across different environments where python might point to an older version or not exist at all.
| cmd=(env PYTHONDONTWRITEBYTECODE=1 python "$test_file" | |
| cmd=(env PYTHONDONTWRITEBYTECODE=1 python3 "$test_file" |
| # Build run command: prefer test_*.py, fall back to run_example.py | ||
| # Build run command using test_*.py | ||
| local test_file | ||
| test_file=$(find "$example_dir" -maxdepth 1 -name 'test_*.py' -print -quit 2>/dev/null || true) |
There was a problem hiding this comment.
Similar to the issue in ci.sh, using find ... -print -quit only picks up the first test file. This can lead to incomplete benchmarking if multiple test_*.py files are present in an example directory.
1 participant
All golden.py-based tests have been migrated to pytest @scene_test format, making both legacy runners dead code.