Improve unit test times with isolated parallel workers#225
Conversation
📝 WalkthroughWalkthroughUnit tests now run in parallel with worker-specific temporary workspaces, while E2E tests are divided across deterministic full and representative suites. Coverage paths, artifact cleanup, archive validation, shard selection, and workflow gating are updated for parallel execution. ChangesTest Execution
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant Pytest
participant E2EShardPlugin
GitHubActions->>Pytest: pass suite, shard index, and shard count
Pytest->>E2EShardPlugin: collect E2E tests
E2EShardPlugin->>Pytest: validate assignments and deselect other shards
Pytest->>GitHubActions: execute selected shard
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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)
tests/README.md (1)
21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language identifier to fenced code block.
Specifying a language for the fenced code block improves syntax highlighting and resolves a markdownlint warning.
♻️ Proposed fix
-``` +```bash pytest tests/test_unittests.py -n 2 --dist load --download-weights --log-cli-level=INFO --log-file=out.log --log-file-level=DEBUG</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.In
@tests/README.mdaround lines 21 - 25, Update the fenced command block in the
tests README to specify the bash language identifier, preserving the existing
pytest command unchanged.</details> <!-- cr-comment:v1:46eb3ed9d26b48d1f77dd292 --> _Source: Linters/SAST tools_ </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>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@tests/README.md:
- Around line 21-25: Update the fenced command block in the tests README to
specify the bash language identifier, preserving the existing pytest command
unchanged.</details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `789f4b11-3fad-4515-ba03-2ef80ad207b3` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 01b44ad8dbefbcceaedbcaef48950f2ab04a354a and 649c30a36d666eec02a0cd601fa4f0301967b36b. </details> <details> <summary>📒 Files selected for processing (6)</summary> * `.github/workflows/unittests.yaml` * `requirements-dev.txt` * `tests/README.md` * `tests/conftest.py` * `tests/helper_functions.py` * `tests/test_unittests.py` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
☂️ Python Coverage
Overall Coverage
New FilesNo new covered files... Modified FilesNo covered modified files...
|
Test Results 6 files 6 suites 13m 31s ⏱️ Results for commit 9d3ef11. ♻️ This comment has been updated with latest results. |
4486f07 to
f77bed7
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_unittests.py (1)
101-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude CLI output in assertion failure messages.
When a negative test fails (e.g., the CLI succeeds when it was expected to fail, or fails with an unexpected error code), the assertion message only prints the exit codes. Appending
result.stdoutto these messages will make debugging CI failures significantly easier by revealing the actual error or unexpected output.
tests/test_unittests.py#L101-L103: Append\nOutput:\n{result.stdout}to the assertion message string.tests/test_unittests.py#L150-L152: Append\nOutput:\n{result.stdout}to the assertion message string.tests/test_unittests.py#L196-L198: Append\nOutput:\n{result.stdout}to the assertion message string.tests/test_unittests.py#L238-L240: Append\nOutput:\n{result.stdout}to the assertion message string.🤖 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 `@tests/test_unittests.py` around lines 101 - 103, Append the CLI output to each exit-code assertion failure message in tests/test_unittests.py at lines 101-103, 150-152, 196-198, and 238-240, using result.stdout with the “Output” label. Update the assertion messages associated with the affected test checks while preserving their existing exit-code details.
🤖 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 `@tests/test_unittests.py`:
- Around line 101-103: Append the CLI output to each exit-code assertion failure
message in tests/test_unittests.py at lines 101-103, 150-152, 196-198, and
238-240, using result.stdout with the “Output” label. Update the assertion
messages associated with the affected test checks while preserving their
existing exit-code details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: afc547ca-3750-449a-9767-84840e0372af
⛔ Files ignored due to path filters (1)
media/coverage_badge.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
.github/workflows/end2end_tests.yaml.github/workflows/unittests.yamlrequirements-dev.txttests/README.mdtests/conftest.pytests/e2e_shards.pytests/helper_functions.pytests/test_unittests.py
🚧 Files skipped from review as they are similar to previous changes (4)
- requirements-dev.txt
- tests/helper_functions.py
- .github/workflows/unittests.yaml
- tests/README.md
Add a shared ten-shard assignment while preserving the existing three-shard fallback. Run ten E2E shards per platform and fail explicitly when the public E2E collection drifts from the checked-in assignments.
f338409 to
2aad6cc
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
tests/test_unittests.py (2)
306-308: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
pytestimport.
pytestis already imported at the module level (line 7), making this local import unnecessary.♻️ Proposed fix
def test_e2e_shard_assignment_detects_collection_drift(): import importlib - - import pytest e2e_shards = importlib.import_module("e2e_shards")🤖 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 `@tests/test_unittests.py` around lines 306 - 308, Remove the redundant local pytest import near the affected test code in tests/test_unittests.py, while retaining the existing module-level import. Do not change the test behavior or other imports.
304-310: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
pytestimport.
pytestis already imported at the module level (line 7), making this local import unnecessary.♻️ Proposed fix
def test_e2e_shard_assignment_detects_collection_drift(): import importlib - - import pytest e2e_shards = importlib.import_module("e2e_shards")🤖 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 `@tests/test_unittests.py` around lines 304 - 310, Remove the redundant local pytest import from test_e2e_shard_assignment_detects_collection_drift, relying on the existing module-level import while leaving the test’s other imports and behavior unchanged.
🤖 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.
Inline comments:
In @.github/workflows/end2end_tests.yaml:
- Around line 54-56: Update the google-github-actions/auth step to reference a
reviewed, trusted 40-character commit SHA instead of the mutable `@v2` tag, while
preserving the existing authentication configuration and condition.
- Line 25: Update the E2E workflow matrix’s shard list to contain exactly three
shards, and change the corresponding --e2e-shard-count argument to 3 so both
values match the supported configuration.
---
Nitpick comments:
In `@tests/test_unittests.py`:
- Around line 306-308: Remove the redundant local pytest import near the
affected test code in tests/test_unittests.py, while retaining the existing
module-level import. Do not change the test behavior or other imports.
- Around line 304-310: Remove the redundant local pytest import from
test_e2e_shard_assignment_detects_collection_drift, relying on the existing
module-level import while leaving the test’s other imports and behavior
unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e353ecbe-cf24-431b-ac66-1b8270c2cd17
⛔ Files ignored due to path filters (1)
media/coverage_badge.svgis excluded by!**/*.svg
📒 Files selected for processing (8)
.github/workflows/end2end_tests.yaml.github/workflows/unittests.yamlrequirements-dev.txttests/README.mdtests/conftest.pytests/e2e_shards.pytests/helper_functions.pytests/test_unittests.py
🚧 Files skipped from review as they are similar to previous changes (6)
- requirements-dev.txt
- tests/README.md
- tests/helper_functions.py
- .github/workflows/unittests.yaml
- tests/e2e_shards.py
- tests/conftest.py
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/end2end_tests.yaml (1)
24-24: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign the workflow to the supported E2E shard count.
The job configures ten shards, but the underlying test suite requires exactly three shards and rejects unsupported counts. Update both the matrix and the CLI argument to match the supported count of 3.
.github/workflows/end2end_tests.yaml#L24-L24: Change the shard matrix list to[0, 1, 2]..github/workflows/end2end_tests.yaml#L51-L51: Change the argument--e2e-shard-count 10to--e2e-shard-count 3.🤖 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 @.github/workflows/end2end_tests.yaml at line 24, Update .github/workflows/end2end_tests.yaml at lines 24-24 and 51-51: change the shard matrix to [0, 1, 2] and change the E2E CLI argument from --e2e-shard-count 10 to --e2e-shard-count 3.
🧹 Nitpick comments (2)
.github/workflows/end2end_tests.yaml (1)
31-31: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueDisable credential persistence in checkout.
The
actions/checkoutstep persists the GitHub token in the local git config by default. Since these jobs do not push back to the repository, you can disable this to reduce the risk of credential exposure if a later step is compromised.
.github/workflows/end2end_tests.yaml#L31-L31: Addpersist-credentials: falseto thewithblock..github/workflows/end2end_tests.yaml#L79-L79: Addpersist-credentials: falseto thewithblock.🤖 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 @.github/workflows/end2end_tests.yaml at line 31, Disable checkout credential persistence by adding persist-credentials: false to the with blocks of both actions/checkout steps at .github/workflows/end2end_tests.yaml lines 31-31 and 79-79.Source: Linters/SAST tools
tests/e2e_shards.py (1)
314-317: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant parentheses. The codebase contains several instances of unnecessary wrapping parentheses around variables and strings, which can be safely removed for cleaner syntax.
tests/e2e_shards.py#L314-L317: remove parentheses aroundREPRESENTATIVE_E2E_SHARD_ASSIGNMENTS_BY_COUNT.tests/e2e_shards.py#L342-L348: remove parentheses around thehelpstring.tests/e2e_shards.py#L437-L440: remove parentheses aroundREPRESENTATIVE_PUBLIC_NODEIDS.🤖 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 `@tests/e2e_shards.py` around lines 314 - 317, Remove the redundant wrapping parentheses in tests/e2e_shards.py at lines 314-317 around REPRESENTATIVE_E2E_SHARD_ASSIGNMENTS_BY_COUNT, lines 342-348 around the help string, and lines 437-440 around REPRESENTATIVE_PUBLIC_NODEIDS; preserve the existing values and behavior.
🤖 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.
Duplicate comments:
In @.github/workflows/end2end_tests.yaml:
- Line 24: Update .github/workflows/end2end_tests.yaml at lines 24-24 and 51-51:
change the shard matrix to [0, 1, 2] and change the E2E CLI argument from
--e2e-shard-count 10 to --e2e-shard-count 3.
---
Nitpick comments:
In @.github/workflows/end2end_tests.yaml:
- Line 31: Disable checkout credential persistence by adding
persist-credentials: false to the with blocks of both actions/checkout steps at
.github/workflows/end2end_tests.yaml lines 31-31 and 79-79.
In `@tests/e2e_shards.py`:
- Around line 314-317: Remove the redundant wrapping parentheses in
tests/e2e_shards.py at lines 314-317 around
REPRESENTATIVE_E2E_SHARD_ASSIGNMENTS_BY_COUNT, lines 342-348 around the help
string, and lines 437-440 around REPRESENTATIVE_PUBLIC_NODEIDS; preserve the
existing values and behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34b6bfd5-e1b0-42a9-9b66-2e76ca0e5b16
📒 Files selected for processing (3)
.github/workflows/end2end_tests.yamltests/e2e_shards.pytests/test_unittests.py
3 participants
Purpose
Reduce the CI test critical path by optimizing both the unit-test and E2E workflows.
For unit tests, independent model conversions now run in parallel with two
pytest-xdistworkers. Each worker uses an isolated temporary workspace for downloaded models, subprocess execution, conversion outputs, validation, and cleanup.For E2E tests, Ubuntu retains full coverage of all 101 public cases using ten duration-balanced serial shards. Windows and macOS run a representative 30-case suite using two balanced shards per platform.
The representative suite covers the available CLI conversion routes while avoiding the cost of repeating the complete model matrix on every operating system. Tests that use the same physical model weights remain grouped where applicable, allowing weight reuse without introducing unsafe concurrent access to shared conversion resources.
Specification
Unit tests
pytest-xdistas a development dependency.-n 2--dist loadE2E tests
full: all 101 public E2E casesrepresentative: 30 CLI conversion cases covering the available conversion routes--e2e-suite--e2e-shard-index--e2e-shard-countDependencies & Potential Impact
Adds:
pytest-xdist>=3.6.1The unit-test workflow deliberately uses two workers instead of
-n auto.In the controlled unit-test benchmark, mean peak aggregate process-tree RSS increased from approximately 1253 MiB during serial execution to 2321 MiB with two workers. This measurement may count some shared pages more than once, but it still supports using a fixed worker count to avoid unnecessary memory pressure on CI runners.
The isolated unit-test workspaces can contain the same model in more than one worker when both workers need it. The measured parallel runs contained 11 weight files representing 10 unique models, with
yolov8npresent in both workspaces.E2E sharding increases the number of concurrent CI jobs, but the representative Windows and macOS suites substantially reduce total runner usage compared with running the full suite on every platform.
The latest hosted E2E run completed successfully in 33 minutes and 11 seconds:
The macOS jobs were queued and executed sequentially during this run, making macOS availability the critical path. Queueing may vary depending on shared GitHub Actions capacity. The tested 10/2/2 topology keeps the workflow simple while preserving full Ubuntu coverage and representative cross-platform coverage.
The coverage workflow also updated
media/coverage_badge.svgautomatically.No product code or vendored submodules are modified.
Testing & Validation
Unit-test benchmark
A controlled ABBA comparison was run against the same 27-test conversion suite, using a fresh temporary workspace for every execution:
Results:
Final local validation
git diff --checkpassed.--delete-weights-nowbehavior remains compatible.pytest-xdist 3.6.1.Hosted E2E validation
The successful 10/2/2 hosted E2E run completed in 33 minutes and 11 seconds.
Observed execution times:
All 14 jobs completed successfully.
Deployment Plan
No deployment required.
Rollout:
Rollback:
Monitoring:
AI Usage
Assisted-by: ChatGPT
Summary by CodeRabbit
pytest-xdistto support parallel test execution.