CI: per-package test logs, parallel runs, and uboot OOM fix#815
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds ChangesCI Test Infrastructure: Log Capture and Parallel Execution
U-Boot Driver Test Fixture Hardening
Coverage Configuration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/driver_test.py (1)
40-46: 💤 Low valueStatic analysis false positive, but consider defensive quoting.
The static analysis tools flagged this as command injection, but this is a false positive—
rpm_pathis constructed entirely from pytest'stmpdir_factory, not from user input or external requests. The path is deterministic and controlled.That said, using
shlex.quote()is a low-effort defensive practice that protects against edge cases (unusual temp paths, future refactoring):🛡️ Defensive improvement
+import shlex + ... if shutil.which("rpm2cpio") and shutil.which("cpio"): subprocess.run( - f"rpm2cpio {rpm_path} | cpio -idm --quiet ./usr/share/uboot/qemu_arm64/u-boot.bin", + f"rpm2cpio {shlex.quote(str(rpm_path))} | cpio -idm --quiet ./usr/share/uboot/qemu_arm64/u-boot.bin", shell=True, cwd=str(tmp_path), check=True, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/driver_test.py` around lines 40 - 46, The subprocess.run command in the conditional block that checks for rpm2cpio and cpio directly interpolates the rpm_path variable into the shell command string without proper quoting, which could be vulnerable to command injection in edge cases. Apply shlex.quote() around the rpm_path variable when constructing the command string in the subprocess.run call to add defensive quoting. Import shlex at the top of the file if not already present, then wrap rpm_path with shlex.quote(str(rpm_path)) in the f-string command to protect against special characters or unusual paths in the temporary directory.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@python/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/driver_test.py`:
- Around line 40-46: The subprocess.run command in the conditional block that
checks for rpm2cpio and cpio directly interpolates the rpm_path variable into
the shell command string without proper quoting, which could be vulnerable to
command injection in edge cases. Apply shlex.quote() around the rpm_path
variable when constructing the command string in the subprocess.run call to add
defensive quoting. Import shlex at the top of the file if not already present,
then wrap rpm_path with shlex.quote(str(rpm_path)) in the f-string command to
protect against special characters or unusual paths in the temporary directory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d5b90d84-67c6-4a0d-b779-cfd4c6cb63fb
📒 Files selected for processing (3)
.github/workflows/python-tests.yamlpython/Makefilepython/packages/jumpstarter-driver-uboot/jumpstarter_driver_uboot/driver_test.py
| working-directory: python | ||
| env: | ||
| PYTEST_ADDOPTS: "--cov-report=xml" | ||
| PYTEST_ADDOPTS: "--cov-report=xml --log-level=CRITICAL --log-cli-level=CRITICAL" |
There was a problem hiding this comment.
This reduces a lot of unnecessary noise on the tests without completely disabling logs.
| for chunk in r.iter_content(chunk_size=8192): | ||
| f.write(chunk) | ||
|
|
||
| with rpmfile.open(tmp_path / "uboot-images-armv8.rpm") as rpm: |
There was a problem hiding this comment.
this was extracting the RPM to RAM, then extracting the file, not very optimal, and in parallelization used a lot of ram.
| if: needs.changes.outputs.should_run == 'true' || github.event_name == 'workflow_dispatch' | ||
| runs-on: ${{ matrix.runs-on }} | ||
| strategy: | ||
| fail-fast: false |
There was a problem hiding this comment.
isnt it likely that an error in one matrix job will also surface on others hence failing fast kinda makes sense.
There was a problem hiding this comment.
yes and no, I wanted to try without it, but may be it's better to leave it on. For example I was having a lot of mac failures and that stopped the linux ones where I could not see if it was going to fail or not as well... or the other way around.
There was a problem hiding this comment.
Putting it back, we should probably discuss that separately, or disable on a need basis.
| [tool.coverage.run] | ||
| branch = true | ||
| omit = ["conftest.py", "test_*.py", "*_test.py", "*_pb2.py", "*_pb2_grpc.py"] | ||
| omit = ["**/conftest.py", "**/test_*.py", "**/*_test.py", "**/*_pb2.py", "**/*_pb2_grpc.py"] |
There was a problem hiding this comment.
Apparently coverage was complaining for my _test.py file increased python surface. Which unveiled that we probably have better coverage than we thought, and that this may need patters to pickup the subpackages test files. I am not sure if each subpackage may need replication of this.
- Add LOGS_DIR support to Makefile: tee-based log capture, .failed marker files, and test-report target for failure summary - Add PYTHONUNBUFFERED=1 for live output through tee pipeline - Upload per-package test logs as GitHub Actions artifacts - Exclude test files from diff-cover check - Install rpm2cpio and cpio in Linux CI dependencies Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rpmfile decompresses the entire 572MB CPIO payload into memory. Use rpm2cpio | cpio for streaming extraction with rpmfile as fallback. Also adds download progress logging and a 120s timeout. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| exit 1 | ||
| fi | ||
| uv run diff-cover $coverage_files --compare-branch=origin/${{ github.base_ref }} --fail-under=80 --exclude '*_pb2.py' '*_pb2_grpc.py' '**/conftest.py' | ||
| uv run diff-cover $coverage_files --compare-branch=origin/${{ github.base_ref }} --fail-under=80 --exclude '*_pb2.py' '*_pb2_grpc.py' '**/conftest.py' '**/test_*.py' '**/*_test.py' |
There was a problem hiding this comment.
We were considering test and test files as part of our coverage, who tests the test problem?
…ter-dev#815) - **Per-package test log capture**: When `LOGS_DIR` is set, each `pkg-test-%` target captures stdout/stderr to a separate `.log` file via `tee` (visible during run). Failures are tracked with `.failed` markers based on exit code. A `test-report` target prints full logs for failed packages. Local dev behavior is unchanged. - **`fail-fast: false`**: All matrix jobs (runner × Python version) run to completion even when one fails. - **Parallel test runs**: `make test -j4` runs package tests concurrently. - **Log noise suppression**: `--log-level=CRITICAL --log-cli-level=CRITICAL` in CI-only `PYTEST_ADDOPTS`. - **Artifact upload**: Per-package logs uploaded as GitHub Actions artifacts (7-day retention). - **uboot test OOM fix**: `rpmfile` decompresses the entire 31MB zstd RPM into 572MB of memory to extract a 1.6MB `u-boot.bin`. Under parallel runs on resource-constrained VMs this triggers the OOM killer. Now uses streaming `rpm2cpio | cpio` on Linux, falling back to `rpmfile` when unavailable. Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
2 participants
Summary
LOGS_DIRis set, eachpkg-test-%target captures stdout/stderr to a separate.logfile viatee(visible during run). Failures are tracked with.failedmarkers based on exit code. Atest-reporttarget prints full logs for failed packages. Local dev behavior is unchanged.fail-fast: false: All matrix jobs (runner × Python version) run to completion even when one fails.make test -j4runs package tests concurrently.--log-level=CRITICAL --log-cli-level=CRITICALin CI-onlyPYTEST_ADDOPTS.rpmfiledecompresses the entire 31MB zstd RPM into 572MB of memory to extract a 1.6MBu-boot.bin. Under parallel runs on resource-constrained VMs this triggers the OOM killer. Now uses streamingrpm2cpio | cpioon Linux, falling back torpmfilewhen unavailable.Test plan
.logfiles appear in uploaded artifactstest-reportsectionmake teststill works withoutLOGS_DIR🤖 Generated with Claude Code