CI: per-package test logs, parallel runs, and uboot OOM fix#815
CI: per-package test logs, parallel runs, and uboot OOM fix#815mangelajo wants to merge 8 commits into
Conversation
Makefile: When LOGS_DIR is set, pkg-test-% captures stdout/stderr to per-package log files via tee (visible during run), records failures with .failed markers (exit-code based), and test-report prints full logs for failed packages. Local dev behavior is unchanged. CI: Set fail-fast: false so all matrix jobs complete, run tests with -j4 parallelism, suppress log noise with --log-level/--log-cli-level CRITICAL, and upload per-package logs as artifacts (7-day retention). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rpmfile decompresses the entire zstd CPIO payload (572MB) into memory to extract a 1.6MB u-boot.bin. Under parallel test runs this triggers the OOM killer. Use streaming rpm2cpio + cpio on Linux; fall back to rpmfile when those tools are unavailable (macOS). Also adds download timeout (120s), progress logging, and proper error reporting to the fixture. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…).st_size Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| 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.
| from jumpstarter.common.utils import serve | ||
|
|
||
|
|
||
| UBOOT_RPM_URL = "https://kojipkgs.fedoraproject.org/packages/uboot-tools/2025.10/1.fc43/noarch/uboot-images-armv8-2025.10-1.fc43.noarch.rpm" |
There was a problem hiding this comment.
Again, this is not an image we include in the containers, or the product, It's just used to test a real uboot on QEMU and properly validate the uboot driver.
Fix import sorting (ruff I001) and replace pytest.fail() with raise AssertionError to satisfy ty's call-non-callable check. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The except block (network failure) and rpmfile fallback (no rpm2cpio) are not exercised in normal CI runs. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The omit patterns lacked ** prefixes, so test files like package/driver_test.py were measured for coverage. Add ** glob prefixes so conftest.py, test_*.py, *_test.py, *_pb2.py, and *_pb2_grpc.py are excluded at any depth. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| [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.
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